Re: [PATCH proxmox v2 1/4] log: replace per layer filtering by one global filter

["Lukas Wagner" <l.wagner@proxmox.com>]

Hi Thomas, thanks a lot for the patch.

Two notes inline, otherwise the change looks good to me!


On Wed Sep 23, 2026 at 1:57 PM CEST, Thomas Ellmenreich wrote:
> Instead of filtering with the global filter on every layer, provide one
> global filter once, as described here: [0]
>
> This is also in preparation for filters that either have to be cloned or
> cannot be cloned at all, like EnvFilters* [1].
>
> * EnvFilter can be cloned from tracing_subscriber version 0.3.20 onwards.
>
> [0]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/layer/index.html#global-filtering
> [1]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.EnvFilter.html
>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  proxmox-log/src/builder.rs            | 48 +++++++++------------------
>  proxmox-log/src/pve_task_formatter.rs |  2 +-
>  2 files changed, 17 insertions(+), 33 deletions(-)
>
> diff --git a/proxmox-log/src/builder.rs b/proxmox-log/src/builder.rs
> index 6fcf1cc9..3108ea29 100644
> --- a/proxmox-log/src/builder.rs
> +++ b/proxmox-log/src/builder.rs
> @@ -1,8 +1,9 @@
>  use tracing::Level;
>  use tracing::Metadata;
>  use tracing::level_filters::LevelFilter;
> -use tracing_log::{AsLog, LogTracer};
> +use tracing_log::LogTracer;
>  use tracing_subscriber::Layer;
> +use tracing_subscriber::Registry;
>  use tracing_subscriber::layer::Context;
>  use tracing_subscriber::layer::Filter;
>  use tracing_subscriber::layer::SubscriberExt;
> @@ -55,9 +56,7 @@ impl<S> Filter<S> for NoWorkerTask {
>  /// ```
>  pub struct Logger {
>      global_log_level: LevelFilter,
> -    layer: Vec<
> -        Box<dyn tracing_subscriber::Layer<tracing_subscriber::Registry> + Send + Sync + 'static>,
> -    >,
> +    layer: Vec<Box<dyn Layer<Registry> + Send + Sync + 'static>>,

This is fine as a 'cleanup' step as a part of this series, but please rather do this in a separate
commit. Including this in the current commit would be fine if you were
touching these lines anyways, but this is completely unrelated to the
change described in the commit message.

>  }
>  
>  impl Logger {
> @@ -76,11 +75,7 @@ impl Logger {
>      ///
>      /// If the journal cannot be opened, print to stderr instead.
>      pub fn journald(mut self) -> Logger {
> -        self.layer.push(
> -            journald_or_stderr_layer()
> -                .with_filter(self.global_log_level)
> -                .boxed(),
> -        );
> +        self.layer.push(journald_or_stderr_layer().boxed());
>          self
>      }
>  
> @@ -90,12 +85,8 @@ impl Logger {
>      /// no LogContext exists – which means we are not in a PBS workertask – or the level of the
>      /// log message is 'ERROR'.
>      pub fn journald_on_no_workertask(mut self) -> Logger {
> -        self.layer.push(
> -            journald_or_stderr_layer()
> -                .with_filter(NoWorkerTask)
> -                .with_filter(self.global_log_level)
> -                .boxed(),
> -        );
> +        self.layer
> +            .push(journald_or_stderr_layer().with_filter(NoWorkerTask).boxed());
>          self
>      }
>  
> @@ -103,8 +94,7 @@ impl Logger {
>      ///
>      /// Check if a LogContext exists and if it does, print to the corresponding task log file.
>      pub fn tasklog_pbs(mut self) -> Logger {
> -        self.layer
> -            .push(TasklogLayer {}.with_filter(self.global_log_level).boxed());
> +        self.layer.push(TasklogLayer.boxed());
>          self
>      }
>  
> @@ -112,11 +102,7 @@ impl Logger {
>      ///
>      /// Prints all the events to stderr with the compact format (no level, no timestamp).
>      pub fn stderr(mut self) -> Logger {
> -        self.layer.push(
> -            plain_stderr_layer()
> -                .with_filter(self.global_log_level)
> -                .boxed(),
> -        );
> +        self.layer.push(plain_stderr_layer().boxed());
>          self
>      }
>  
> @@ -126,12 +112,8 @@ impl Logger {
>      /// triggered if no workertask could be found (no LogContext exists) or the event level is
>      /// `ERROR`.
>      pub fn stderr_on_no_workertask(mut self) -> Logger {
> -        self.layer.push(
> -            plain_stderr_layer()
> -                .with_filter(NoWorkerTask)
> -                .with_filter(self.global_log_level)
> -                .boxed(),
> -        );
> +        self.layer
> +            .push(plain_stderr_layer().with_filter(NoWorkerTask).boxed());
>          self
>      }
>  
> @@ -141,9 +123,8 @@ impl Logger {
>      /// e.g.: `DEBUG: event message`.
>      pub fn stderr_pve(mut self) -> Logger {
>          let layer = tracing_subscriber::fmt::layer()
> -            .event_format(PveTaskFormatter {})
> +            .event_format(PveTaskFormatter)
>              .with_writer(std::io::stderr)
> -            .with_filter(self.global_log_level)
>              .boxed();
>          self.layer.push(layer);
>          self
> @@ -153,10 +134,13 @@ impl Logger {
>      ///
>      /// Also configures the `LogTracer` which will convert all `log` events to tracing events.
>      pub fn init(self) -> Result<(), anyhow::Error> {
> -        let registry = tracing_subscriber::registry().with(self.layer);
> +        let registry = tracing_subscriber::registry()
> +            .with(self.layer)
> +            .with(self.global_log_level);
> +
>          tracing::subscriber::set_global_default(registry)?;
>  
> -        LogTracer::init_with_filter(self.global_log_level.as_log())?;
> +        LogTracer::init()?;
>          Ok(())
>      }
>  }
> diff --git a/proxmox-log/src/pve_task_formatter.rs b/proxmox-log/src/pve_task_formatter.rs
> index e9866a4b..12bc33c8 100644
> --- a/proxmox-log/src/pve_task_formatter.rs
> +++ b/proxmox-log/src/pve_task_formatter.rs
> @@ -8,7 +8,7 @@ use tracing_subscriber::registry::LookupSpan;
>  /// This custom formatter outputs logs as they are visible in the PVE task log.
>  ///
>  /// e.g.: "DEBUG: sample message"
> -pub struct PveTaskFormatter {}
> +pub struct PveTaskFormatter;
>  

Same here as above, this is unrelated to this commit, this should be in
a cleanup commit up front. Might benefit from a sentence or two in the
commit message explaining that this is a private type, since the
pve_task_formatter is also private. Just from the diff alone it is not
100% clear if this is a breaking API change or not.

>  impl<C, N> FormatEvent<C, N> for PveTaskFormatter
>  where