Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions

[Gabriel Goller <g.goller@proxmox.com>]

On 11.02.2025 10:28, Wolfgang Bumiller wrote:
>On Tue, Feb 11, 2025 at 10:22:44AM +0100, Wolfgang Bumiller wrote:
>> On Mon, Feb 10, 2025 at 05:42:35PM +0100, Gabriel Goller wrote:
>> > On 10.02.2025 15:37, Wolfgang Bumiller wrote:
>> > > On Mon, Dec 09, 2024 at 11:46:03AM +0100, Gabriel Goller wrote:
>> > > > +/// Inits a new tracing logger that prints to stderr or tasklog with the logging level specified in the
>> > > > +/// environment variable `env_var`.
>> > > > +///
>> > > > +/// This logger is task-aware, which means if we are in a PBS task, we will retrieve the task-file
>> > > > +/// and write to it. We'll only write to stderr if we are not in a task. If `env_var` doesn't exist
>> > > > +/// or can't be read, use the `default_log_level`. The output will be very plain: no ansi, no
>> > > > +/// timestamp, no level, just the message and it's
>> > > > +/// fields.
>> > > > +pub fn stderr_or_tasklog(
>> > > > +    env_var: &str,
>> > > > +    default_log_level: LevelFilter,
>> > > > +) -> Result<(), anyhow::Error> {
>> > > > +    let log_level = get_env_variable(env_var, default_log_level);
>> > > > +
>> > > > +    let registry = tracing_subscriber::registry()
>> > > > +        .with(
>> > > > +            plain_stderr_layer()
>> > > > +                .with_filter(filter_fn(|_metadata| !LogContext::exists()))
>> > >
>> > > ^ This condition misses the `Level::ERROR` comparison while being
>> > > suggested as a replacement for `init_cli_logger` which had it (not
>> > > visible in the patch context lines, but it's there).
>> > > If this is done on purpose, please explain it in the commit message.
>> >
>> > Oops, yeah my bad, this should be
>> >
>> >     !LogContext::exists() || *metadata.level() == Level::ERROR
>> >
>> > What do you think about the rest of the patch? I tried to implement this
>> > with a builder pattern as well, but it turned out to be quite tricky
>> > moving the layers around so I just wrote a ton of functions with long
>> > names :(
>>
>> The rest seems fine.
>> It does look like it should be mostly a builder-pattern thing (as it
>> kind of already is, with the final 2 lines being a kind of `.apply()`,
>> but with the names being showing their intended use, it's fine for an
>> `init` module to have specific common setups like this (`init_cli_…`,
>> `…with_pve_format`, etc.)
>>
>> Perhaps the journal/tasklog one could be named "init_daemon_log" (or
>> just have an alias under that name)...
>
>Sorry, I was reading it backwards, we're getting rid of those names...
>That just goes to show I didn't properly think about this... :-)
>
>Now, first of all, having the "descriptive" names there makes sense.
>With the `pve` specific function we then still have a rather specific
>one.
>Therefore, with a specific `init` module, it would IMO be fine to have
>situation-specific names for common setups.
>
>Unless we can actually come up with a builder-pattern variant.
>Perhaps it would have to be the Layer type rather than the subscriber
>we'd need to turn into a builder, while having its final "apply" create
>the subscriber and register it and initialize the LogTracer.

I cooked up a simple builder type thingy to build layers:


     struct LogBuilder {
         global_log_level: LevelFilter,
         layer: Vec<
             Box<dyn tracing_subscriber::Layer<tracing_subscriber::Registry> + Send + Sync + 'static>,
         >,
     }


And the implementation:

     impl LogBuilder {
         pub fn from_env(env_var: &str, default_log_level: LevelFilter) -> LogBuilder {
             let log_level = get_env_variable(env_var, default_log_level);
             LogBuilder {
                 global_log_level: log_level,
                 layer: vec![],
             }
         }

         pub fn journald_or_stderr(mut self) -> LogBuilder {
             self.layer.push(
                 journald_or_stderr_layer()
                     .with_filter(self.global_log_level)
                     .boxed(),
             );
             self
         }

         pub fn journald_or_stderr_on_logcontext_and_error(mut self) -> LogBuilder {
             self.layer.push(
                 journald_or_stderr_layer()
                     .with_filter(filter_fn(|metadata| {
                         !LogContext::exists() || *metadata.level() == Level::ERROR
                     }))
                     .with_filter(self.global_log_level)
                     .boxed(),
             );
             self
         }

         //...

         pub fn init(self) -> Result<(), anyhow::Error> {
             let registry = tracing_subscriber::registry().with(self.layer);
             tracing::subscriber::set_global_default(registry)?;

             LogTracer::init_with_filter(self.global_log_level.as_log())?;
             Ok(())
         }
     }

We could place this in a new builder module and then have the
product-specific functions (e.g. init_pve_log, init_perlmod_log,
init_pbs_log, etc.) in the init module.

What do you think?


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel