[RFC datacenter-manager/proxmox v2 0/4] log: allow finegrained control logging levels

[RFC datacenter-manager/proxmox v2 0/4] log: allow finegrained control logging levels

[Thomas Ellmenreich]

The core of this change is a simple reinterpretation of the logging environment
variables that we are already using. Instead of parsing them as a single level
filter [0], they will now be interpreted as env filters [1]. Since the latter
can still parse the former, this change is backwards compatible and will not
break with any logging environment variables that have been set.

Implementation
--------------

As Gabriel Goller proposed in this [2] Bugzilla enhancement, by using
EnvFilters [1], we can have more granular control over the logging happening in
the different Proxmox products. The first two commits do exactly this and can
thus be applied by themselves. The last three add tests as well as utilities to
better communicate log levels to use cases that don't directly integrate with
tracing.

proxmox 1/4:
    A simple reordering of the logging layers. This had to be done since
    EnvFilter is currently not clonable and can thus be applied only once.
    Previously, the global filter was being applied unnecessarily often, on all
    layers and then on the tracing_log layer as well.

proxmox 2/4:
    Actual implementation of EnvFilters.

proxmox 3/4:
    Introduction of tests for our EnvFilter, parser, and constructor. This was
    initially created for me to better understand EnvFilters, but then evolved
    into tests to ensure consistent functionality with our 'default_log_level'.

datacenter-manager 4/4:
    Use proxmox_log::enabled! to check if DEBUG is enabled and use that to
    determine the REST server debug mode.

Different Log Options
---------------------

Combining the different options of the EnvFilter as well as our own default
log level, we have the following cases for logging:

1. Env contains 'simple':
    Meaning that the environment variable contains a simple 'info'... This
    means that that level is taken as the expected one.

2. Env contains 'simple', and 'module':
    Means that the env variable contains a simple log level as well as zero or
    more module specific log levels. As expected the simple level is applied
    as a default and then for the specified modules the defined levels apply.

3. Env only contains 'module':
    If the env only contains a module (or multiple), that is interpreted by
    tracing as disabling the default logging and only enabling logging for the
    module/s

4. Env is empty:
    No logs are printed at all, everything is hidden.

5. Env is not set:
    This is the only time the default comes into play, by being set as the
    default log level.

Open Questions
--------------

- I had the idea that instead of having all users of the proxmox-log crate
  provide their own 'default_log_level' we could define that inside of the
  proxmox-log crate. By doing so, we could have the default change between
  DEBUG and INFO depending on if we are in a normal or release build.

- Initially, I found case 3. of the different logging cases quite confusing and
  would have thought that our 'default_log_level' should apply as default in
  that case. Unfortunately, when setting 'default_log_level' and then applying
  the EnvFilter string, tracing applies the modules as expected, but then also
  overrides the default as '', which means off.

Notes for the Maintainer
------------------------

For all tracing backed logging in Proxmox products to support EnvFilters,
only patches 1 and 2 have to be applied. The 3 Patch only contains tests
which I mostly used for understanding, but do still test our defaulting
behaviour.

Patch 4 is just something I noticed and fixed, but is not necessary to
fix the Bugzilla issue: [2]

Changelog
---------

- RFC v1 -> v2
    * Removed patch 'v1 proxmox 4/5', which introduced a LoggerConfig returned
      by the Logger::init function. Deemed it unnecessary since I replaced the
      usage for it in patch 'v1 datacenter-manager 5/5' with a simple 'enabled!'
      and the idea of using it in the solution for [3] was abandoned.

    * Cleaned up this cover letter


[0]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.LevelFilter.html
[1]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.EnvFilter.html
[2]: https://bugzilla.proxmox.com/show_bug.cgi?id=6081
[3]: https://bugzilla.proxmox.com/show_bug.cgi?id=4646


proxmox:

Thomas Ellmenreich (3):
  log: replace per layer filtering by one global filter
  fix #6081: log: replace simple level filter with env filter
  log: add tests to the logger

 proxmox-log/Cargo.toml                |   2 +-
 proxmox-log/src/builder.rs            | 374 +++++++++++++++++++++++---
 proxmox-log/src/lib.rs                |  16 --
 proxmox-log/src/pve_task_formatter.rs |   2 +-
 4 files changed, 337 insertions(+), 57 deletions(-)


proxmox-datacenter-manager:

Thomas Ellmenreich (1):
  api: set REST server debug level based on actual log level

 server/src/bin/proxmox-datacenter-api/main.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


Summary over all repositories:
  5 files changed, 340 insertions(+), 59 deletions(-)

-- 
Generated by murpp 0.12.0

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

[Thomas Ellmenreich]

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>>,
 }
 
 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;
 
 impl<C, N> FormatEvent<C, N> for PveTaskFormatter
 where
-- 
2.47.3

[PATCH proxmox v2 2/4] fix #6081: log: replace simple level filter with env filter

[Thomas Ellmenreich]

Instead of parsing the contents of the environment variable passed to the
Logger as a simple LevelFilter, parse them as an EnvFilter [0]. This is
backwards compatible, as a simple LevelFilter [1] string is parsable as
an EnvFilter.

[0]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.EnvFilter.html
[1]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.LevelFilter.html

Suggested-by: Gabriel Goller <g.goller@proxmox.com>
Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6081
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 proxmox-log/Cargo.toml     |  2 +-
 proxmox-log/src/builder.rs | 38 ++++++++++++++++++++++++++++++++------
 proxmox-log/src/lib.rs     | 16 ----------------
 3 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/proxmox-log/Cargo.toml b/proxmox-log/Cargo.toml
index 25c36fc4..4abe1b11 100644
--- a/proxmox-log/Cargo.toml
+++ b/proxmox-log/Cargo.toml
@@ -16,7 +16,7 @@ anyhow.workspace = true
 nix.workspace = true
 tracing.workspace = true
 tracing-journald.workspace = true
-tracing-subscriber.workspace = true
+tracing-subscriber = { workspace = true, features = ["env-filter"] }
 tracing-log = { workspace = true, features = ["std"] }
 tokio = { workspace = true, features = ["rt-multi-thread"] }
 
diff --git a/proxmox-log/src/builder.rs b/proxmox-log/src/builder.rs
index 3108ea29..9b545603 100644
--- a/proxmox-log/src/builder.rs
+++ b/proxmox-log/src/builder.rs
@@ -2,6 +2,7 @@ use tracing::Level;
 use tracing::Metadata;
 use tracing::level_filters::LevelFilter;
 use tracing_log::LogTracer;
+use tracing_subscriber::EnvFilter;
 use tracing_subscriber::Layer;
 use tracing_subscriber::Registry;
 use tracing_subscriber::layer::Context;
@@ -9,7 +10,7 @@ use tracing_subscriber::layer::Filter;
 use tracing_subscriber::layer::SubscriberExt;
 
 use crate::{
-    LogContext, get_env_variable, journald_or_stderr_layer, plain_stderr_layer,
+    LogContext, journald_or_stderr_layer, plain_stderr_layer,
     pve_task_formatter::PveTaskFormatter, tasklog_layer::TasklogLayer,
 };
 ///
@@ -55,16 +56,17 @@ impl<S> Filter<S> for NoWorkerTask {
 /// # func().expect("failed to init logger");
 /// ```
 pub struct Logger {
-    global_log_level: LevelFilter,
+    global_log_level: EnvFilter,
     layer: Vec<Box<dyn Layer<Registry> + Send + Sync + 'static>>,
 }
 
 impl Logger {
-    /// Create a new LogBuilder with no layers and a default loglevel retrieved from an env
-    /// variable. If the env variable cannot be retrieved or the content is not parsable, fallback
-    /// to the default_log_level passed.
+    /// Create a new Logger with no layers and a default loglevel retrieved from an env
+    /// variable. If the env variable cannot be retrieved or the content is not parsable,
+    /// fallback to the default_log_level passed.
     pub fn from_env(env_var: &str, default_log_level: LevelFilter) -> Logger {
-        let log_level = get_env_variable(env_var, default_log_level);
+        let var_content = std::env::var(env_var).ok();
+        let log_level = Self::apply_default_log_level(var_content, default_log_level);
         Logger {
             global_log_level: log_level,
             layer: vec![],
@@ -143,4 +145,28 @@ impl Logger {
         LogTracer::init()?;
         Ok(())
     }
+
+    /// If present, tries to parse the `env_filter_str` as a [`EnvFilter`],
+    /// otherwiese falls back to the provided default log level.
+    ///
+    /// The provided default log level is a application default defined by us.
+    /// The user can define their own default as part of the [`EnvFilter`]
+    ///
+    /// * `env_filter_str` - The log settings that would usually be written into
+    ///                      a environment variable and then extracted by [`get_env_variable`]
+    /// * `default_log_level` - Default log level used by the [`Logger`] in
+    ///                         case the log settings are empty.
+    fn apply_default_log_level(
+        env_filter_str: Option<String>,
+        default_log_level: LevelFilter,
+    ) -> EnvFilter {
+        if let Some(env_filter_str) = env_filter_str {
+            match EnvFilter::try_new(env_filter_str) {
+                Ok(filter) => return filter,
+                Err(e) => eprintln!("unable to parse the log env variable: {e:#}"),
+            }
+        }
+        EnvFilter::default().add_directive(default_log_level.into())
+    }
 }
+
diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs
index 2d321f20..32c10e27 100644
--- a/proxmox-log/src/lib.rs
+++ b/proxmox-log/src/lib.rs
@@ -1,7 +1,6 @@
 #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]
 #![deny(unsafe_op_in_unsafe_fn)]
 
-use std::env;
 use std::future::Future;
 use std::sync::{Arc, Mutex};
 
@@ -147,21 +146,6 @@ where
         .with_writer(std::io::stderr)
 }
 
-fn get_env_variable(env_var: &str, default_log_level: LevelFilter) -> LevelFilter {
-    let mut log_level = default_log_level;
-    if let Ok(v) = env::var(env_var) {
-        match v.parse::<LevelFilter>() {
-            Ok(l) => {
-                log_level = l;
-            }
-            Err(e) => {
-                eprintln!("env variable {env_var} found, but parsing failed: {e:?}");
-            }
-        }
-    }
-    log_level
-}
-
 /// Initialize tracing logger that prints to journald or stderr depending on if we are in a pbs
 /// task.
 ///
-- 
2.47.3

[PATCH proxmox v2 3/4] log: add tests to the logger

[Thomas Ellmenreich]

Add tests to ensure consistent functionality when setting the logging
environment variable, as well as global filtering of logs.

Although the EnvFilter itself is not something we should be testing, we are
adding some additional machinery, on top of the extensive functionality
provided by tracing/tracing_subscriber, which does need testing.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 proxmox-log/src/builder.rs | 296 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 291 insertions(+), 5 deletions(-)

diff --git a/proxmox-log/src/builder.rs b/proxmox-log/src/builder.rs
index 9b545603..05da3643 100644
--- a/proxmox-log/src/builder.rs
+++ b/proxmox-log/src/builder.rs
@@ -1,5 +1,6 @@
 use tracing::Level;
 use tracing::Metadata;
+use tracing::Subscriber;
 use tracing::level_filters::LevelFilter;
 use tracing_log::LogTracer;
 use tracing_subscriber::EnvFilter;
@@ -10,8 +11,8 @@ use tracing_subscriber::layer::Filter;
 use tracing_subscriber::layer::SubscriberExt;
 
 use crate::{
-    LogContext, journald_or_stderr_layer, plain_stderr_layer,
-    pve_task_formatter::PveTaskFormatter, tasklog_layer::TasklogLayer,
+    LogContext, journald_or_stderr_layer, plain_stderr_layer, pve_task_formatter::PveTaskFormatter,
+    tasklog_layer::TasklogLayer,
 };
 ///
 /// Filter yielding `true` *outside* of worker tasks, *unless* the level is `ERROR`.
@@ -136,9 +137,7 @@ 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)
-            .with(self.global_log_level);
+        let registry = self.create_subscriber();
 
         tracing::subscriber::set_global_default(registry)?;
 
@@ -146,6 +145,15 @@ impl Logger {
         Ok(())
     }
 
+    /// Creates the subscriber to be used for the Logger
+    ///
+    /// Placed in its own method to allow easier testing of the subscriber
+    fn create_subscriber(self) -> impl Subscriber {
+        tracing_subscriber::registry()
+            .with(self.layer)
+            .with(self.global_log_level)
+    }
+
     /// If present, tries to parse the `env_filter_str` as a [`EnvFilter`],
     /// otherwiese falls back to the provided default log level.
     ///
@@ -170,3 +178,281 @@ impl Logger {
     }
 }
 
+#[cfg(test)]
+mod tests {
+    use std::sync::{Arc, Mutex};
+
+    use tracing::level_filters::LevelFilter;
+    use tracing_log::log;
+    use tracing_subscriber::{Layer, util::SubscriberInitExt};
+
+    use crate::Logger;
+
+    /// Modules created for testing purposes. Specifically, to test filtering
+    /// of logs in different modules.
+    mod test_module {
+        pub mod nested_module {
+            pub fn info(message: &'static str) {
+                tracing_log::log::info!("{message}");
+            }
+        }
+        pub fn info(message: &'static str) {
+            tracing_log::log::info!("{message}");
+        }
+    }
+
+    macro_rules! assert_logs {
+        ($events:expr, [$($expected:expr),* $(,)?]) => {{
+            let events = $events.lock().unwrap();
+
+            assert_eq!(events.as_slice(), [$($expected),*]);
+        }};
+    }
+
+    #[test]
+    fn logger_builder_correctly_applies_filter() {
+        // Arrange
+        let (events, builder) = create_logger(Some("WARN"), LevelFilter::INFO);
+        let _guard = builder.create_subscriber().set_default();
+
+        // Act
+        log::info!("log1");
+        log::warn!("log2");
+
+        // Assert
+        assert_logs!(events, ["log2"]);
+    }
+
+    #[test]
+    fn simple_log_is_registered() {
+        // Arrange
+        let (events, _guard) = setup_logging(None, LevelFilter::TRACE);
+
+        // Act
+        log::info!("log1");
+
+        // Assert
+        assert_logs!(events, ["log1"]);
+    }
+
+    #[test]
+    fn env_filter_is_backwards_compatible() {
+        // Arrange
+        let (events, _guard) = setup_logging(Some("ERROR"), LevelFilter::INFO);
+
+        // Act
+        test_module::info("log1");
+        test_module::nested_module::info("log2");
+        log::error!("log3");
+
+        // Assert
+        assert_logs!(events, ["log3"]);
+    }
+
+    #[test]
+    fn only_module_filter_disables_other_logging() {
+        // Arrange
+        let (events, _guard) = setup_logging(
+            Some("proxmox_log::builder::tests::test_module=INFO"),
+            LevelFilter::INFO,
+        );
+
+        // Act
+        test_module::info("log1");
+        test_module::nested_module::info("log2");
+        log::info!("log3");
+
+        // Assert
+        assert_logs!(events, ["log1", "log2"]);
+    }
+
+    #[test]
+    fn nested_module_filter_correctly_removes_logs_from_nested_module() {
+        // Arrange
+        let (events, _guard) = setup_logging(
+            Some(
+                "proxmox_log::builder::tests::test_module=INFO,proxmox_log::builder::tests::test_module::nested_module=ERROR",
+            ),
+            LevelFilter::INFO,
+        );
+
+        // Act
+        test_module::info("log1");
+        test_module::nested_module::info("log2");
+
+        // Assert
+        assert_logs!(events, ["log1",]);
+    }
+
+    #[test]
+    fn empty_variable_turns_off_all_logging() {
+        // Arrange
+        let (events, _guard) = setup_logging(Some(""), LevelFilter::TRACE);
+
+        // Act
+        log::trace!("log1");
+        log::debug!("log2");
+        log::info!("log3");
+        log::warn!("log4");
+        log::error!("log5");
+
+        // Assert
+        let list = events.lock().unwrap();
+        assert!(list.is_empty());
+    }
+
+    #[test]
+    fn gibberish_filter_allows_use_of_fallback() {
+        // Arrange
+        let (events, _guard) = setup_logging(
+            Some("SNERROR,::_interesting*Üpath::we;HAVE=QUARNING"),
+            LevelFilter::ERROR,
+        );
+
+        // Act
+        log::info!("log1");
+        log::error!("log2");
+
+        // Assert
+        assert_logs!(events, ["log2"]);
+    }
+
+    #[test]
+    fn setting_default_in_variable_is_adopted_as_default() {
+        // Arrange
+        let (events, _guard) = setup_logging(
+            Some("ERROR,proxmox_log::builder::tests::test_module::nested_module=INFO"),
+            LevelFilter::INFO,
+        );
+
+        // Act
+        test_module::info("log1");
+        test_module::nested_module::info("log2");
+        log::error!("log3");
+
+        assert_logs!(events, ["log2", "log3"]);
+    }
+
+    /// Creates a [`Logger`] with a [`crate::builder::tests::TestLayer`]
+    /// inserted so that any logs received can be monitored through the
+    /// returned Vec of strings.
+    /// * `var_filter` - The log settings that would usually be written into
+    ///                  a environment variable and then extracted by the [`Logger`]
+    /// * `default_log_level` - Default log level used by the [`Logger`] in
+    ///                         case the log settings are empty.
+    fn create_logger(
+        var_filter: Option<&'static str>,
+        default_log_level: LevelFilter,
+    ) -> (Arc<Mutex<Vec<String>>>, Logger) {
+        let (events, test_layer) = TestLayer::no_filter();
+        (
+            events,
+            Logger {
+                global_log_level: Logger::apply_default_log_level(
+                    var_filter.map(str::to_string),
+                    default_log_level,
+                ),
+                layer: vec![test_layer.boxed()],
+            },
+        )
+    }
+
+    /// Sets up logging in a way that allows the filter to act, and then
+    /// records every log that is let through in a Vec of strings. Returns
+    /// said Vec of strings as well as a guard that allows tracing to capture
+    /// logs while it's not dropped.
+    /// * `var_filter` - The log settings that would usually be written into
+    ///                  a environment variable and then extracted by the [`Logger`]
+    /// * `default_log_level` - Default log level used by the [`Logger`] in
+    ///                         case the log settings are empty.
+    fn setup_logging(
+        var_filter: Option<&'static str>,
+        default_log_level: LevelFilter,
+    ) -> (Arc<Mutex<Vec<String>>>, tracing::subscriber::DefaultGuard) {
+        use tracing_subscriber::layer::SubscriberExt;
+
+        let filter =
+            Logger::apply_default_log_level(var_filter.map(str::to_string), default_log_level);
+
+        let (events, filter) = TestLayer::new(filter);
+        let guard = tracing_subscriber::Registry::default()
+            .with(filter)
+            .set_default();
+        (events, guard)
+    }
+
+    pub(crate) struct TestLayer {
+        events: Arc<Mutex<Vec<String>>>,
+        filter: Option<tracing_subscriber::EnvFilter>,
+    }
+
+    impl TestLayer {
+        pub(crate) fn no_filter() -> (Arc<Mutex<Vec<String>>>, Self) {
+            let events = Arc::new(Mutex::new(vec![]));
+            (
+                Arc::clone(&events),
+                Self {
+                    events,
+                    filter: None,
+                },
+            )
+        }
+
+        pub(crate) fn new(
+            filter: tracing_subscriber::EnvFilter,
+        ) -> (Arc<Mutex<Vec<String>>>, Self) {
+            let events = Arc::new(Mutex::new(vec![]));
+            (
+                Arc::clone(&events),
+                Self {
+                    events,
+                    filter: Some(filter),
+                },
+            )
+        }
+    }
+
+    impl<S> tracing_subscriber::Layer<S> for TestLayer
+    where
+        S: tracing::Subscriber,
+    {
+        /// adds events to the Vec of Strings
+        fn on_event(
+            &self,
+            event: &tracing::Event<'_>,
+            _: tracing_subscriber::layer::Context<'_, S>,
+        ) {
+            struct Visitor(Option<String>);
+
+            impl tracing::field::Visit for Visitor {
+                fn record_debug(
+                    &mut self,
+                    field: &tracing::field::Field,
+                    value: &dyn std::fmt::Debug,
+                ) {
+                    if field.name() == "message" {
+                        self.0 = Some(format!("{value:?}"));
+                    }
+                }
+            }
+
+            let mut visitor = Visitor(None);
+            event.record(&mut visitor);
+            let message = visitor.0.unwrap_or_default();
+
+            self.events.lock().unwrap().push(message);
+        }
+
+        /// applies the filter to the logs
+        fn enabled(
+            &self,
+            metadata: &tracing::Metadata<'_>,
+            ctx: tracing_subscriber::layer::Context<'_, S>,
+        ) -> bool {
+            self.filter
+                .as_ref()
+                .map(|filter| filter.enabled(metadata, ctx))
+                .unwrap_or(true)
+        }
+    }
+}
-- 
2.47.3

[PATCH datacenter-manager v2 4/4] api: set REST server debug level based on actual log level

[Thomas Ellmenreich]

Use the configuration returned by the initialised logger to decide whether to
activate debugging for the REST server.

Previously, whether the REST server was in debug mode would only be determined
by the existence of the PROXMOX_DEBUG variable, regardless of its value. Now,
if the environment variable is set to a less verbose level than 'debug', the
REST server will not activate its debug level.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 server/src/bin/proxmox-datacenter-api/main.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/server/src/bin/proxmox-datacenter-api/main.rs b/server/src/bin/proxmox-datacenter-api/main.rs
index 57aa6e22..2fc84c1d 100644
--- a/server/src/bin/proxmox-datacenter-api/main.rs
+++ b/server/src/bin/proxmox-datacenter-api/main.rs
@@ -17,6 +17,7 @@ use tracing::level_filters::LevelFilter;
 use url::form_urlencoded;
 
 use proxmox_lang::try_block;
+use proxmox_log::Level;
 use proxmox_rest_server::{ApiConfig, RestEnvironment, RestServer};
 use proxmox_router::{RpcEnvironment, RpcEnvironmentType};
 use proxmox_sys::fs::CreateOptions;
@@ -46,13 +47,13 @@ fn main() -> Result<(), Error> {
 
     server::env::sanitize_environment_vars();
 
-    let debug = std::env::var("PROXMOX_DEBUG").is_ok();
-
     proxmox_log::Logger::from_env("PROXMOX_DEBUG", LevelFilter::INFO)
         .journald_on_no_workertask()
         .tasklog_pbs()
         .init()?;
 
+    let debug = proxmox_log::enabled!(Level::DEBUG);
+
     if std::env::args().nth(1).is_some() {
         bail!("unexpected command line parameters");
     }
-- 
2.47.3

Re: [RFC datacenter-manager/proxmox v2 0/4] log: allow finegrained control logging levels

[Lukas Wagner]

On Wed Sep 23, 2026 at 1:57 PM CEST, Thomas Ellmenreich wrote:
> The core of this change is a simple reinterpretation of the logging environment
> variables that we are already using. Instead of parsing them as a single level
> filter [0], they will now be interpreted as env filters [1]. Since the latter
> can still parse the former, this change is backwards compatible and will not
> break with any logging environment variables that have been set.
>

Thanks for these patches, looking very good so far. I only had very
minor complaints -- see the patch replies for these.

Tested-by: Lukas Wagner <l.wagner@proxmox.com>

With the minor comments addressed and the diff otherwise unchanged:

Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>

>
> Different Log Options
> ---------------------
>
> Combining the different options of the EnvFilter as well as our own default
> log level, we have the following cases for logging:
>
> 1. Env contains 'simple':
>     Meaning that the environment variable contains a simple 'info'... This
>     means that that level is taken as the expected one.
>
> 2. Env contains 'simple', and 'module':
>     Means that the env variable contains a simple log level as well as zero or
>     more module specific log levels. As expected the simple level is applied
>     as a default and then for the specified modules the defined levels apply.
>
> 3. Env only contains 'module':
>     If the env only contains a module (or multiple), that is interpreted by
>     tracing as disabling the default logging and only enabling logging for the
>     module/s
>
> 4. Env is empty:
>     No logs are printed at all, everything is hidden.
>
> 5. Env is not set:
>     This is the only time the default comes into play, by being set as the
>     default log level.
>
> Open Questions
> --------------
>
> - I had the idea that instead of having all users of the proxmox-log crate
>   provide their own 'default_log_level' we could define that inside of the
>   proxmox-log crate. By doing so, we could have the default change between
>   DEBUG and INFO depending on if we are in a normal or release build.

I think keeping the default log level inside the application code is
fine, to me this seems to be inherently application-specific, even if we
are going to use the same default for all rust-based product code for
now.

I'm also not sure if we'd want to automatically switch to DEBUG for
debug builds, there are some crates in our dependency graph that are
*very* noisy at the debug level. E.g.:

Oct 02 09:55:10 pdm proxmox-datacenter-api[1451]: reuse idle connection for ("https", xxx.xxx.xxx.xxx:8006)
Oct 02 09:55:10 pdm proxmox-datacenter-api[1451]: reuse idle connection for ("https", xxx.xxx.xxx.xxx:8006)
Oct 02 09:55:10 pdm proxmox-datacenter-api[1451]: pooling idle connection for ("https", xxx.xxx.xxx.xxx:8006)
Oct 02 09:55:10 pdm proxmox-datacenter-api[1451]: reuse idle connection for ("https", xxx.xxx.xxx.xxx:8006)
Oct 02 09:55:10 pdm proxmox-datacenter-api[1451]: reuse idle connection for ("https", xxx.xxx.xxx.xxx:8006)
Oct 02 09:55:10 pdm proxmox-datacenter-api[1451]: reuse idle connection for ("https", xxx.xxx.xxx.xxx:8006)
Oct 02 09:55:10 pdm proxmox-datacenter-api[1451]: pooling idle connection for ("https", xxx.xxx.xxx.xxx:8006)
Oct 02 09:55:10 pdm proxmox-datacenter-api[1451]: reuse idle connection for ("https", xxx.xxx.xxx.xxx:8006)
Oct 02 09:55:10 pdm proxmox-datacenter-api[1451]: connecting to xxx.xxx.xxx.xxx:8007


>
> - Initially, I found case 3. of the different logging cases quite confusing and
>   would have thought that our 'default_log_level' should apply as default in
>   that case. Unfortunately, when setting 'default_log_level' and then applying
>   the EnvFilter string, tracing applies the modules as expected, but then also
>   overrides the default as '', which means off.

Indeed a bit confusing, but since this is (mostly) a
developer/supporter feature, as long at this behavior is documented
somewhere, it should be manageable.

>
> Notes for the Maintainer
> ------------------------
>
> For all tracing backed logging in Proxmox products to support EnvFilters,
> only patches 1 and 2 have to be applied. The 3 Patch only contains tests
> which I mostly used for understanding, but do still test our defaulting
> behaviour.
>
> Patch 4 is just something I noticed and fixed, but is not necessary to
> fix the Bugzilla issue: [2]
>

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

[Lukas Wagner]

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

Re: [PATCH proxmox v2 2/4] fix #6081: log: replace simple level filter with env filter

[Lukas Wagner]

On Wed Sep 23, 2026 at 1:58 PM CEST, Thomas Ellmenreich wrote:
> Instead of parsing the contents of the environment variable passed to the
> Logger as a simple LevelFilter, parse them as an EnvFilter [0]. This is
> backwards compatible, as a simple LevelFilter [1] string is parsable as
> an EnvFilter.
>

Looks good to me, one small note inline.

> +
> +    /// If present, tries to parse the `env_filter_str` as a [`EnvFilter`],
> +    /// otherwiese falls back to the provided default log level.
> +    ///
> +    /// The provided default log level is a application default defined by us.
> +    /// The user can define their own default as part of the [`EnvFilter`]
> +    ///
> +    /// * `env_filter_str` - The log settings that would usually be written into
> +    ///                      a environment variable and then extracted by [`get_env_variable`]
> +    /// * `default_log_level` - Default log level used by the [`Logger`] in
> +    ///                         case the log settings are empty.

Thanks for the extensive documentation!

This produceds 2 clippy warnings (warning: doc list item overindented),
please fix these for the next iteration of this series.

> +    fn apply_default_log_level(
> +        env_filter_str: Option<String>,
> +        default_log_level: LevelFilter,
> +    ) -> EnvFilter {
> +        if let Some(env_filter_str) = env_filter_str {
> +            match EnvFilter::try_new(env_filter_str) {
> +                Ok(filter) => return filter,
> +                Err(e) => eprintln!("unable to parse the log env variable: {e:#}"),
> +            }
> +        }
> +        EnvFilter::default().add_directive(default_log_level.into())
> +    }
>  }
> +
> diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs
> index 2d321f20..32c10e27 100644
> --- a/proxmox-log/src/lib.rs
> +++ b/proxmox-log/src/lib.rs
> @@ -1,7 +1,6 @@
>  #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]
>  #![deny(unsafe_op_in_unsafe_fn)]
>  
> -use std::env;
>  use std::future::Future;
>  use std::sync::{Arc, Mutex};
>  
> @@ -147,21 +146,6 @@ where
>          .with_writer(std::io::stderr)
>  }
>  
> -fn get_env_variable(env_var: &str, default_log_level: LevelFilter) -> LevelFilter {
> -    let mut log_level = default_log_level;
> -    if let Ok(v) = env::var(env_var) {
> -        match v.parse::<LevelFilter>() {
> -            Ok(l) => {
> -                log_level = l;
> -            }
> -            Err(e) => {
> -                eprintln!("env variable {env_var} found, but parsing failed: {e:?}");
> -            }
> -        }
> -    }
> -    log_level
> -}
> -
>  /// Initialize tracing logger that prints to journald or stderr depending on if we are in a pbs
>  /// task.
>  ///

Re: [PATCH proxmox v2 3/4] log: add tests to the logger

[Lukas Wagner]

On Wed Sep 23, 2026 at 1:58 PM CEST, Thomas Ellmenreich wrote:
> Add tests to ensure consistent functionality when setting the logging
> environment variable, as well as global filtering of logs.
>
> Although the EnvFilter itself is not something we should be testing, we are
> adding some additional machinery, on top of the extensive functionality
> provided by tracing/tracing_subscriber, which does need testing.
>

[...]


>      /// If present, tries to parse the `env_filter_str` as a [`EnvFilter`],
>      /// otherwiese falls back to the provided default log level.
>      ///
> @@ -170,3 +178,281 @@ impl Logger {
>      }
>  }

Thanks a lot for writing tests for this!

For completeness, you might also change some of the tests (or add 1-2
new ones) to emit native tracing events (e.g. tracing::info!), right now
you only emit log events (either directly through the `log` crate or
through `tracing_log::log`). The risk that one works and the other does
not is minimal, but while at it it certainly does not hurt.

Also please check `cargo clippy --tests`, there are some minor warnings
here (also about overindented documentation).

>  
> +#[cfg(test)]
> +mod tests {
> +    use std::sync::{Arc, Mutex};
> +
> +    use tracing::level_filters::LevelFilter;
> +    use tracing_log::log;
> +    use tracing_subscriber::{Layer, util::SubscriberInitExt};

Re: [RFC datacenter-manager/proxmox v2 0/4] log: allow finegrained control logging levels

[Thomas Ellmenreich]

Thank you very much for your review. I will address the changes in the
next version. I have one question below:

On Fri Oct 2, 2026 at 10:10 AM CEST, Lukas Wagner wrote:

[snip]

>> - Initially, I found case 3. of the different logging cases quite confusing and
>>   would have thought that our 'default_log_level' should apply as default in
>>   that case. Unfortunately, when setting 'default_log_level' and then applying
>>   the EnvFilter string, tracing applies the modules as expected, but then also
>>   overrides the default as '', which means off.
>
> Indeed a bit confusing, but since this is (mostly) a
> developer/supporter feature, as long at this behavior is documented
> somewhere, it should be manageable.

Since you mentioned it, should I add some documentation somewhere
explaining how to use these env filters? If so, where would be the best
place for it?

[snip]

Re: [RFC datacenter-manager/proxmox v2 0/4] log: allow finegrained control logging levels

[Lukas Wagner]

On Fri Oct 2, 2026 at 11:34 AM CEST, Thomas Ellmenreich wrote:
> Thank you very much for your review. I will address the changes in the
> next version. I have one question below:
>
> On Fri Oct 2, 2026 at 10:10 AM CEST, Lukas Wagner wrote:
>
> [snip]
>
>>> - Initially, I found case 3. of the different logging cases quite confusing and
>>>   would have thought that our 'default_log_level' should apply as default in
>>>   that case. Unfortunately, when setting 'default_log_level' and then applying
>>>   the EnvFilter string, tracing applies the modules as expected, but then also
>>>   overrides the default as '', which means off.
>>
>> Indeed a bit confusing, but since this is (mostly) a
>> developer/supporter feature, as long at this behavior is documented
>> somewhere, it should be manageable.
>
> Since you mentioned it, should I add some documentation somewhere
> explaining how to use these env filters? If so, where would be the best
> place for it?

Since as I mentioned this is (mostly) a developer-feature, I think adding
a small section about it in the 'dev-docs' (this is a folder in the PDM
repo) would be a good start.

It of course still could also make sense to mention in the user-facing
product documentation, but as we have briefly discussed off-list, module
paths are inherently unstable, so it's somewhat pointless to provide
examples there. Familiarity with the code structure is required to
properly use this feature.

So for now, I'd just add it as developer documentation.

I've noticed that PDM and PBS use different conventions for the
environment variable, in PDM we use PROXMOX_DEBUG, while in PBS we use
PBS_LOG -- maybe that's also something where we should find a common
style to use. Just thinking out aloud here, no need to touch this as
part of this series, of course.

>
> [snip]

Re: [RFC datacenter-manager/proxmox v2 0/4] log: allow finegrained control logging levels

[Thomas Ellmenreich]

On Fri Oct 2, 2026 at 11:44 AM CEST, Lukas Wagner wrote:
> On Fri Oct 2, 2026 at 11:34 AM CEST, Thomas Ellmenreich wrote:
>> Thank you very much for your review. I will address the changes in the
>> next version. I have one question below:
>>
>> On Fri Oct 2, 2026 at 10:10 AM CEST, Lukas Wagner wrote:
>>
>> [snip]
>>
>>>> - Initially, I found case 3. of the different logging cases quite confusing and
>>>>   would have thought that our 'default_log_level' should apply as default in
>>>>   that case. Unfortunately, when setting 'default_log_level' and then applying
>>>>   the EnvFilter string, tracing applies the modules as expected, but then also
>>>>   overrides the default as '', which means off.
>>>
>>> Indeed a bit confusing, but since this is (mostly) a
>>> developer/supporter feature, as long at this behavior is documented
>>> somewhere, it should be manageable.
>>
>> Since you mentioned it, should I add some documentation somewhere
>> explaining how to use these env filters? If so, where would be the best
>> place for it?
>
> Since as I mentioned this is (mostly) a developer-feature, I think adding
> a small section about it in the 'dev-docs' (this is a folder in the PDM
> repo) would be a good start.
>
> It of course still could also make sense to mention in the user-facing
> product documentation, but as we have briefly discussed off-list, module
> paths are inherently unstable, so it's somewhat pointless to provide
> examples there. Familiarity with the code structure is required to
> properly use this feature.

Yes, I totally understand. I was also thinking of adding it to the developer
focus documentation for the reasons you mentioned.

> So for now, I'd just add it as developer documentation.
>
> I've noticed that PDM and PBS use different conventions for the
> environment variable, in PDM we use PROXMOX_DEBUG, while in PBS we use
> PBS_LOG -- maybe that's also something where we should find a common
> style to use. Just thinking out aloud here, no need to touch this as
> part of this series, of course.

I was actually wondering about this when creating the series. In my mind, all
log level variables should end with '_LOG', but we can't really replace
'PROXMOX_DEBUG' since it is already in use. Could we also support a 'PDM_LOG'
variable alongside it, and then first deprecate and later remove the old one?
(Not for this series, but just as a thought)

>>
>> [snip]

Re: [RFC datacenter-manager/proxmox v2 0/4] log: allow finegrained control logging levels

[Lukas Wagner]

On Fri Oct 2, 2026 at 12:00 PM CEST, Thomas Ellmenreich wrote:
> On Fri Oct 2, 2026 at 11:44 AM CEST, Lukas Wagner wrote:
>> On Fri Oct 2, 2026 at 11:34 AM CEST, Thomas Ellmenreich wrote:
>>> Thank you very much for your review. I will address the changes in the
>>> next version. I have one question below:
>>>
>>> On Fri Oct 2, 2026 at 10:10 AM CEST, Lukas Wagner wrote:
>>>
>>> [snip]
>>>
>>>>> - Initially, I found case 3. of the different logging cases quite confusing and
>>>>>   would have thought that our 'default_log_level' should apply as default in
>>>>>   that case. Unfortunately, when setting 'default_log_level' and then applying
>>>>>   the EnvFilter string, tracing applies the modules as expected, but then also
>>>>>   overrides the default as '', which means off.
>>>>
>>>> Indeed a bit confusing, but since this is (mostly) a
>>>> developer/supporter feature, as long at this behavior is documented
>>>> somewhere, it should be manageable.
>>>
>>> Since you mentioned it, should I add some documentation somewhere
>>> explaining how to use these env filters? If so, where would be the best
>>> place for it?
>>
>> Since as I mentioned this is (mostly) a developer-feature, I think adding
>> a small section about it in the 'dev-docs' (this is a folder in the PDM
>> repo) would be a good start.
>>
>> It of course still could also make sense to mention in the user-facing
>> product documentation, but as we have briefly discussed off-list, module
>> paths are inherently unstable, so it's somewhat pointless to provide
>> examples there. Familiarity with the code structure is required to
>> properly use this feature.
>
> Yes, I totally understand. I was also thinking of adding it to the developer
> focus documentation for the reasons you mentioned.
>
>> So for now, I'd just add it as developer documentation.
>>
>> I've noticed that PDM and PBS use different conventions for the
>> environment variable, in PDM we use PROXMOX_DEBUG, while in PBS we use
>> PBS_LOG -- maybe that's also something where we should find a common
>> style to use. Just thinking out aloud here, no need to touch this as
>> part of this series, of course.
>
> I was actually wondering about this when creating the series. In my mind, all
> log level variables should end with '_LOG', but we can't really replace
> 'PROXMOX_DEBUG' since it is already in use. Could we also support a 'PDM_LOG'
> variable alongside it, and then first deprecate and later remove the old one?
> (Not for this series, but just as a thought)


I grepped the user-facing documentation and did a quick google search,
found no examples of the PROXMOX_DEBUG variable being described, so I
*think* we might get away with just changing the variable.

The approach you described is of course the 'cleaner' approach, but I
guess that would require API changes in proxmox-log, not sure if that is
worth it.

What we would do is check on startup if the old variable is set, and if
that is the case, we show a warning that guides the user to use the new
one. A user caring about detailed logging would then hopefully notice
this since they check the logs anyway.

Probably best to get one or two other opinions on this before we change
this. It's not super urgent anyway, this is also something we could do
at the next major version (PDM 1.x -> PDM 2.x), then it would simply be
a breaking change that is mentioned in the release notes, as usual.

>
>>>
>>> [snip]