From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id CD611F5C0 for ; Fri, 29 Sep 2023 13:46:09 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B72BF1FE00 for ; Fri, 29 Sep 2023 13:46:09 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 29 Sep 2023 13:46:08 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 586CE48E2F for ; Fri, 29 Sep 2023 13:46:08 +0200 (CEST) Date: Fri, 29 Sep 2023 13:46:07 +0200 From: Wolfgang Bumiller To: Gabriel Goller Cc: Dominik Csapak , Thomas Lamprecht , Proxmox Backup Server development discussion Message-ID: References: <20230821111938.110298-1-g.goller@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL 0.099 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Sep 2023 11:46:09 -0000 On Fri, Sep 29, 2023 at 01:09:15PM +0200, Gabriel Goller wrote: > > On 9/29/23 11:49, Wolfgang Bumiller wrote: > > On Fri, Sep 29, 2023 at 11:35:06AM +0200, Dominik Csapak wrote: > > > [..] > > On the one hand `proxmox-router` is used for both the API daemons and by > > our schema-based CLI parser, and we already have `cli::init_cli_logger` > > in there. > > On the other hand, there's no guarantee that all daemons will use this > > crate, if they don't need any schema/CLI parsing, but then again this > > can still be initialized specially there... > > > > Basically, I don't specifically object to having a common helper for > > a "this is how our daemons usually do logging" type of deal, but it may > > still make more sense in proxmox-rest-server. > I agree, it does more sense in the proxmox-rest-server crate. > > Regardless of where we put it, for our log refactoring, we'd need this > > to return a logger instance, rather than actually setting the logger, > > because our API daemons will need a *custom* logger to deal with the > > workers, which in turn needs access to the logger created *here*. > > Yeah, we can do that, we will just have to return the `syslog::BasicLogger` > and call `log::set_boxed_logger(..)` in the api/proxy `run()` function. > > Should we also return the max_log_level somehow, maybe in a tupel? > Currently I am already setting it in the `init_syslog_logger` function > using log::set_max_loglevel(..)`. I'm not sure it's worth it, those are basically two somewhat independent things. In fact, the `log::set_...` call will probably also happen in rest-server where we create the custom logger, and I'm not sure a helper just for the syslogger instance is worth much. Maybe you could already create the custom logger, for now simply wrapping the syslog instance and forwarding all the methods from the `Log` trait to that, and then we can build the worker task logging logic on top of that. That way, the function signature basically is the same as it is now, setting up the log level as well, just moved to rest-server and ready to get some more logic added. If you'd like to take a stab at the rest of it: For the worker task logging, as discussed off list, it'll most likely be simplest to use tokio's `TaskLocal` to hold an optional reference to the current task (and maybe some state about where to log to), so that we don't need to pass the worker task around as is currently required to use the `task_log!()` macro. It has a sync version which should work for the threaded task variant. We also need ways to control where to log to and/or explicitly log to only the task log or only the syslog.