Re: [pbs-devel] [PATCH proxmox-backup] api: Outsource the logger initialization to the router

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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.