From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 0F21B1FF0B7 for ; Fri, 02 Oct 2026 10:10:15 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id B116C21667; Fri, 02 Oct 2026 10:10:14 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Fri, 02 Oct 2026 10:10:10 +0200 Message-Id: Subject: Re: [PATCH proxmox v2 1/4] log: replace per layer filtering by one global filter From: "Lukas Wagner" To: "Thomas Ellmenreich" , Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260923115815.211083-1-t.ellmenreich@proxmox.com> <20260923115815.211083-2-t.ellmenreich@proxmox.com> In-Reply-To: <20260923115815.211083-2-t.ellmenreich@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790928610503 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.398 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_SHORT 0.001 Use of a URL Shortener for very short URL RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: IGSJ53MBDINZBBTL75IBG2LOCLIMT2CW X-Message-ID-Hash: IGSJ53MBDINZBBTL75IBG2LOCLIMT2CW X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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/i= ndex.html#global-filtering > [1]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/= struct.EnvFilter.html > > Signed-off-by: Thomas Ellmenreich > --- > 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 Filter for NoWorkerTask { > /// ``` > pub struct Logger { > global_log_level: LevelFilter, > - layer: Vec< > - Box = + Send + Sync + 'static>, > - >, > + layer: Vec + Send + Sync + 'static>>, This is fine as a 'cleanup' step as a part of this series, but please rathe= r 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. > } > =20 > 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 > } > =20 > @@ -90,12 +85,8 @@ impl Logger { > /// no LogContext exists =E2=80=93 which means we are not in a PBS w= orkertask =E2=80=93 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).b= oxed()); > self > } > =20 > @@ -103,8 +94,7 @@ impl Logger { > /// > /// Check if a LogContext exists and if it does, print to the corres= ponding task log file. > pub fn tasklog_pbs(mut self) -> Logger { > - self.layer > - .push(TasklogLayer {}.with_filter(self.global_log_level).box= ed()); > + self.layer.push(TasklogLayer.boxed()); > self > } > =20 > @@ -112,11 +102,7 @@ impl Logger { > /// > /// Prints all the events to stderr with the compact format (no leve= l, 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 > } > =20 > @@ -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 > } > =20 > @@ -141,9 +123,8 @@ impl Logger { > /// e.g.: `DEBUG: event message`. > pub fn stderr_pve(mut self) -> Logger { > let layer =3D 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` eve= nts to tracing events. > pub fn init(self) -> Result<(), anyhow::Error> { > - let registry =3D tracing_subscriber::registry().with(self.layer)= ; > + let registry =3D tracing_subscriber::registry() > + .with(self.layer) > + .with(self.global_log_level); > + > tracing::subscriber::set_global_default(registry)?; > =20 > - 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 ta= sk log. > /// > /// e.g.: "DEBUG: sample message" > -pub struct PveTaskFormatter {} > +pub struct PveTaskFormatter; > =20 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 FormatEvent for PveTaskFormatter > where