* [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper
@ 2024-12-09 10:46 Gabriel Goller
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions Gabriel Goller
` (5 more replies)
0 siblings, 6 replies; 19+ messages in thread
From: Gabriel Goller @ 2024-12-09 10:46 UTC (permalink / raw)
To: pbs-devel
To make all the logging init functions easier to use renamed all and moved to
`init` module. Like this we don't have to use a e.g. 'init_pve_logger' function
in a shared helper, which isn't strictly true, as it can be pbs as well.
The is backwards compatible as the old functions have not been removed and are
only deprecated. This also means the first two `proxmox` patches can already be
applied independently of the other ones.
Changelog:
v2:
- rename `stderr_and_tasklog` to `stderr_or_tasklog` as it's more fitting
- add `journald_and_tasklog` fn for pbs_daily_update
proxmox:
Gabriel Goller (2):
log: rename/move init functions
log: add logger for perlmod crates
proxmox-log/src/init.rs | 143 ++++++++++++++++++++++++++
proxmox-log/src/lib.rs | 87 ++++++++--------
proxmox-log/src/pve_task_formatter.rs | 31 ++++++
3 files changed, 220 insertions(+), 41 deletions(-)
create mode 100644 proxmox-log/src/init.rs
create mode 100644 proxmox-log/src/pve_task_formatter.rs
proxmox-backup:
Gabriel Goller (1):
log: use new init functions
pbs-tape/src/bin/pmt.rs | 3 +--
pbs-tape/src/bin/pmtx.rs | 3 +--
proxmox-backup-client/src/main.rs | 4 ++--
proxmox-file-restore/src/main.rs | 3 +--
pxar-bin/src/main.rs | 5 +++--
src/bin/proxmox-backup-api.rs | 3 +--
src/bin/proxmox-backup-debug.rs | 4 ++--
src/bin/proxmox-backup-manager.rs | 3 +--
src/bin/proxmox-backup-proxy.rs | 3 +--
src/bin/proxmox-tape.rs | 4 ++--
src/bin/sg-tape-cmd.rs | 3 +--
11 files changed, 16 insertions(+), 22 deletions(-)
proxmox-perl-rs:
Gabriel Goller (1):
log: use new init function, print to stderr and journald
common/src/logger.rs | 4 +++-
pmg-rs/Cargo.toml | 1 -
pmg-rs/src/lib.rs | 2 +-
pve-rs/Cargo.toml | 1 -
4 files changed, 4 insertions(+), 4 deletions(-)
Summary over all repositories:
18 files changed, 240 insertions(+), 67 deletions(-)
--
Generated by git-murpp 0.7.1
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2024-12-09 10:46 [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper Gabriel Goller
@ 2024-12-09 10:46 ` Gabriel Goller
2025-02-10 14:37 ` Wolfgang Bumiller
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox v2 2/4] log: add logger for perlmod crates Gabriel Goller
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Gabriel Goller @ 2024-12-09 10:46 UTC (permalink / raw)
To: pbs-devel; +Cc: Lukas Wagner
Rename and move init functions to different module. Make function names
product-independent. Like this we don't have to use e.g. `pve_logger` in
a shared library, which wouldn't be completely true.
Suggested-by: Lukas Wagner <l.wagner@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
proxmox-log/src/init.rs | 116 ++++++++++++++++++++++++++++++++++++++++
proxmox-log/src/lib.rs | 86 +++++++++++++++--------------
2 files changed, 161 insertions(+), 41 deletions(-)
create mode 100644 proxmox-log/src/init.rs
diff --git a/proxmox-log/src/init.rs b/proxmox-log/src/init.rs
new file mode 100644
index 000000000000..67efa9fcc81f
--- /dev/null
+++ b/proxmox-log/src/init.rs
@@ -0,0 +1,116 @@
+use tracing::{level_filters::LevelFilter, Level};
+use tracing_log::{AsLog, LogTracer};
+use tracing_subscriber::{filter::filter_fn, layer::SubscriberExt, Layer};
+
+use crate::{
+ get_env_variable, journald_or_stderr_layer, plain_stderr_layer, tasklog_layer::TasklogLayer, LogContext
+};
+
+/// Inits a new tracing logger that prints to journald 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 journald if we are not in a task, or if the level of the
+/// log is `ERROR`. 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 journald_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(
+ journald_or_stderr_layer()
+ .with_filter(filter_fn(|metadata| {
+ !LogContext::exists() || *metadata.level() == Level::ERROR
+ }))
+ .with_filter(log_level),
+ )
+ .with(TasklogLayer {}.with_filter(log_level));
+
+ tracing::subscriber::set_global_default(registry)?;
+ LogTracer::init_with_filter(log_level.as_log())?;
+ Ok(())
+}
+
+/// 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()))
+ .with_filter(log_level),
+ )
+ .with(TasklogLayer {}.with_filter(log_level));
+
+ tracing::subscriber::set_global_default(registry)?;
+ LogTracer::init_with_filter(log_level.as_log())?;
+ Ok(())
+}
+
+/// Inits a new tracing logger that prints to stderr with the logging level specified in the
+/// environment variable `env_var`.
+///
+/// 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(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(log_level));
+
+ tracing::subscriber::set_global_default(registry)?;
+ LogTracer::init_with_filter(log_level.as_log())?;
+ Ok(())
+}
+
+/// Inits a new tracing logger that prints to journald with the logging level specified in the
+/// environment variable `env_var`.
+///
+/// Prints every message to journald. If journald cannot be opened, every message will land in
+/// stderr. If `env_var` does not exist or doesn't contain a readable log level, the
+/// `default_log_level` will be used.
+pub fn journald(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(journald_or_stderr_layer().with_filter(log_level));
+
+ tracing::subscriber::set_global_default(registry)?;
+ LogTracer::init_with_filter(log_level.as_log())?;
+ Ok(())
+}
+
+/// Inits a new tracing logger that prints to journald and 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 will always write to journald and tasklog, even if we are in a task. If
+/// `env_var` doesn't exist or can't be read, use the `default_log_level`.
+pub fn journald_and_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(journald_or_stderr_layer().with_filter(log_level))
+ .with(TasklogLayer {}.with_filter(log_level));
+
+ tracing::subscriber::set_global_default(registry)?;
+ LogTracer::init_with_filter(log_level.as_log())?;
+ Ok(())
+}
diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs
index 8c74e42b618d..50f63699336f 100644
--- a/proxmox-log/src/lib.rs
+++ b/proxmox-log/src/lib.rs
@@ -5,18 +5,18 @@ use std::env;
use std::future::Future;
use std::sync::{Arc, Mutex};
+use tasklog_layer::TasklogLayer;
use tokio::task::futures::TaskLocalFuture;
use tracing_log::{AsLog, LogTracer};
use tracing_subscriber::filter::filter_fn;
use tracing_subscriber::prelude::*;
-use tasklog_layer::TasklogLayer;
-
mod file_logger;
-pub use file_logger::{FileLogOptions, FileLogger};
-
mod tasklog_layer;
+pub mod init;
+pub use file_logger::{FileLogOptions, FileLogger};
+
pub use tracing::debug;
pub use tracing::debug_span;
pub use tracing::enabled;
@@ -38,36 +38,6 @@ tokio::task_local! {
static LOG_CONTEXT: LogContext;
}
-pub fn init_logger(
- env_var_name: &str,
- default_log_level: LevelFilter,
-) -> Result<(), anyhow::Error> {
- let mut log_level = default_log_level;
- if let Ok(v) = env::var(env_var_name) {
- match v.parse::<LevelFilter>() {
- Ok(l) => {
- log_level = l;
- }
- Err(e) => {
- eprintln!("env variable {env_var_name} found, but parsing failed: {e:?}");
- }
- }
- }
- let registry = tracing_subscriber::registry()
- .with(
- journald_or_stderr_layer()
- .with_filter(filter_fn(|metadata| {
- !LogContext::exists() || *metadata.level() == Level::ERROR
- }))
- .with_filter(log_level),
- )
- .with(TasklogLayer {}.with_filter(log_level));
-
- tracing::subscriber::set_global_default(registry)?;
- LogTracer::init_with_filter(log_level.as_log())?;
- Ok(())
-}
-
/// A file logger and warnings counter which can be used across a scope for separate logging.
/// Mainly used for worker-task logging.
pub struct FileLogState {
@@ -160,22 +130,56 @@ where
.with_writer(std::io::stderr)
}
-/// Initialize default logger for CLI binaries
-pub fn init_cli_logger(
- env_var_name: &str,
- default_log_level: LevelFilter,
-) -> Result<(), anyhow::Error> {
+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_name) {
+ if let Ok(v) = env::var(env_var) {
match v.parse::<LevelFilter>() {
Ok(l) => {
log_level = l;
}
Err(e) => {
- eprintln!("env variable {env_var_name} found, but parsing failed: {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.
+///
+/// Check the (tokio) LogContext and print to either journald or the Tasklog.
+#[deprecated(note = "Use the init::journald_or_tasklog function")]
+pub fn init_logger(
+ env_var_name: &str,
+ default_log_level: LevelFilter,
+) -> Result<(), anyhow::Error> {
+ let log_level = get_env_variable(env_var_name, default_log_level);
+
+ let registry = tracing_subscriber::registry()
+ .with(
+ journald_or_stderr_layer()
+ .with_filter(filter_fn(|metadata| {
+ !LogContext::exists() || *metadata.level() == Level::ERROR
+ }))
+ .with_filter(log_level),
+ )
+ .with(TasklogLayer {}.with_filter(log_level));
+
+ tracing::subscriber::set_global_default(registry)?;
+ LogTracer::init_with_filter(log_level.as_log())?;
+ Ok(())
+}
+
+/// Initialize default tracing logger for CLI binaries.
+///
+/// Prints to stderr and to the tasklog if we are in a pbs workertask.
+#[deprecated(note = "Use the init::stderr_or_tasklog function")]
+pub fn init_cli_logger(
+ env_var_name: &str,
+ default_log_level: LevelFilter,
+) -> Result<(), anyhow::Error> {
+ let log_level = get_env_variable(env_var_name, default_log_level);
let registry = tracing_subscriber::registry()
.with(
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [pbs-devel] [PATCH proxmox v2 2/4] log: add logger for perlmod crates
2024-12-09 10:46 [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper Gabriel Goller
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions Gabriel Goller
@ 2024-12-09 10:46 ` Gabriel Goller
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox-backup v2 3/4] log: use new init functions Gabriel Goller
` (3 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Gabriel Goller @ 2024-12-09 10:46 UTC (permalink / raw)
To: pbs-devel; +Cc: Lukas Wagner
Add logger for perlmod crates that always prints to stderr (with a
specific format, which will end up in the tasklog) and to journald.
Reported-by: Lukas Wagner <l.wagner@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
proxmox-log/src/init.rs | 29 ++++++++++++++++++++++++-
proxmox-log/src/lib.rs | 1 +
proxmox-log/src/pve_task_formatter.rs | 31 +++++++++++++++++++++++++++
3 files changed, 60 insertions(+), 1 deletion(-)
create mode 100644 proxmox-log/src/pve_task_formatter.rs
diff --git a/proxmox-log/src/init.rs b/proxmox-log/src/init.rs
index 67efa9fcc81f..f2edcbec6a13 100644
--- a/proxmox-log/src/init.rs
+++ b/proxmox-log/src/init.rs
@@ -3,7 +3,8 @@ use tracing_log::{AsLog, LogTracer};
use tracing_subscriber::{filter::filter_fn, layer::SubscriberExt, Layer};
use crate::{
- get_env_variable, journald_or_stderr_layer, plain_stderr_layer, tasklog_layer::TasklogLayer, LogContext
+ get_env_variable, journald_or_stderr_layer, plain_stderr_layer,
+ pve_task_formatter::PveTaskFormatter, tasklog_layer::TasklogLayer, LogContext,
};
/// Inits a new tracing logger that prints to journald or tasklog with the logging level specified in the
@@ -94,6 +95,32 @@ pub fn journald(env_var: &str, default_log_level: LevelFilter) -> Result<(), any
Ok(())
}
+/// Inits a new tracing logger that prints to stderr and journald with the logging level specified
+/// in the environment variable `env_var`.
+///
+/// Prints every message to stderr with a pve-specific format and journald (the fallback will be
+/// stderr as well). The output format for stderr will be "DEBUG: ...msg.." (equivalent to the
+/// pve-workertask format). If `env_var` does not exist or doesn't contain a readable log level,
+/// the `default_log_level` will be used.
+pub fn stderr_and_journald_with_pve_format(
+ env_var: &str,
+ default_log_level: LevelFilter,
+) -> Result<(), anyhow::Error> {
+ let log_level = get_env_variable(env_var, default_log_level);
+
+ let stderr_layer = tracing_subscriber::fmt::layer()
+ .event_format(PveTaskFormatter {})
+ .with_writer(std::io::stderr);
+
+ let registry = tracing_subscriber::registry()
+ .with(tracing_journald::layer().ok().with_filter(log_level))
+ .with(stderr_layer.with_filter(log_level));
+
+ tracing::subscriber::set_global_default(registry)?;
+ LogTracer::init_with_filter(log_level.as_log())?;
+ Ok(())
+}
+
/// Inits a new tracing logger that prints to journald and tasklog with the logging level specified in the
/// environment variable `env_var`.
///
diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs
index 50f63699336f..f5e448fb7f79 100644
--- a/proxmox-log/src/lib.rs
+++ b/proxmox-log/src/lib.rs
@@ -12,6 +12,7 @@ use tracing_subscriber::filter::filter_fn;
use tracing_subscriber::prelude::*;
mod file_logger;
+mod pve_task_formatter;
mod tasklog_layer;
pub mod init;
diff --git a/proxmox-log/src/pve_task_formatter.rs b/proxmox-log/src/pve_task_formatter.rs
new file mode 100644
index 000000000000..e9866a4b0869
--- /dev/null
+++ b/proxmox-log/src/pve_task_formatter.rs
@@ -0,0 +1,31 @@
+use std::fmt;
+use tracing::{Event, Subscriber};
+use tracing_subscriber::field::VisitOutput;
+use tracing_subscriber::fmt::format::{DefaultVisitor, Writer};
+use tracing_subscriber::fmt::{FmtContext, FormatEvent, FormatFields};
+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 {}
+
+impl<C, N> FormatEvent<C, N> for PveTaskFormatter
+where
+ C: Subscriber + for<'a> LookupSpan<'a>,
+ N: for<'a> FormatFields<'a> + 'static,
+{
+ fn format_event(
+ &self,
+ _ctx: &FmtContext<'_, C, N>,
+ mut writer: Writer<'_>,
+ event: &Event<'_>,
+ ) -> fmt::Result {
+ write!(writer, "{}: ", event.metadata().level().as_str())?;
+
+ let mut v = DefaultVisitor::new(writer.by_ref(), true);
+ event.record(&mut v);
+ v.finish()?;
+ writer.write_char('\n')
+ }
+}
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 3/4] log: use new init functions
2024-12-09 10:46 [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper Gabriel Goller
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions Gabriel Goller
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox v2 2/4] log: add logger for perlmod crates Gabriel Goller
@ 2024-12-09 10:46 ` Gabriel Goller
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox-perl-rs v2 4/4] log: use new init function, print to stderr and journald Gabriel Goller
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Gabriel Goller @ 2024-12-09 10:46 UTC (permalink / raw)
To: pbs-devel
Use new logger init functions that are more descriptive, less
product-bound.
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
pbs-tape/src/bin/pmt.rs | 3 +--
pbs-tape/src/bin/pmtx.rs | 3 +--
proxmox-backup-client/src/main.rs | 4 ++--
proxmox-file-restore/src/main.rs | 3 +--
pxar-bin/src/main.rs | 5 +++--
src/bin/proxmox-backup-api.rs | 3 +--
src/bin/proxmox-backup-debug.rs | 4 ++--
src/bin/proxmox-backup-manager.rs | 3 +--
src/bin/proxmox-backup-proxy.rs | 3 +--
src/bin/proxmox-tape.rs | 4 ++--
src/bin/sg-tape-cmd.rs | 3 +--
11 files changed, 16 insertions(+), 22 deletions(-)
diff --git a/pbs-tape/src/bin/pmt.rs b/pbs-tape/src/bin/pmt.rs
index 9e39dbe16b2f..eecb0562aca9 100644
--- a/pbs-tape/src/bin/pmt.rs
+++ b/pbs-tape/src/bin/pmt.rs
@@ -15,7 +15,6 @@
use anyhow::{bail, Error};
use serde_json::Value;
-use proxmox_log::init_cli_logger;
use proxmox_router::cli::*;
use proxmox_router::RpcEnvironment;
use proxmox_schema::{api, ArraySchema, IntegerSchema, Schema, StringSchema};
@@ -800,7 +799,7 @@ fn options(
}
fn main() -> Result<(), Error> {
- init_cli_logger("PBS_LOG", proxmox_log::LevelFilter::INFO)?;
+ proxmox_log::init::stderr_or_tasklog("PBS_LOG", proxmox_log::LevelFilter::INFO)?;
let uid = nix::unistd::Uid::current();
diff --git a/pbs-tape/src/bin/pmtx.rs b/pbs-tape/src/bin/pmtx.rs
index 303353e6bfd3..627a2c7b8389 100644
--- a/pbs-tape/src/bin/pmtx.rs
+++ b/pbs-tape/src/bin/pmtx.rs
@@ -16,7 +16,6 @@ use std::fs::File;
use anyhow::{bail, Error};
use serde_json::Value;
-use proxmox_log::init_cli_logger;
use proxmox_router::cli::*;
use proxmox_router::RpcEnvironment;
use proxmox_schema::api;
@@ -388,7 +387,7 @@ fn scan(param: Value) -> Result<(), Error> {
}
fn main() -> Result<(), Error> {
- init_cli_logger("PBS_LOG", proxmox_log::LevelFilter::INFO)?;
+ proxmox_log::init::stderr_or_tasklog("PBS_LOG", proxmox_log::LevelFilter::INFO)?;
let uid = nix::unistd::Uid::current();
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 632a291707c7..1b3f2d584a98 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -16,7 +16,6 @@ use xdg::BaseDirectories;
use pathpatterns::{MatchEntry, MatchType, PatternFlag};
use proxmox_async::blocking::TokioWriterAdapter;
use proxmox_io::StdChannelWriter;
-use proxmox_log::init_cli_logger;
use proxmox_router::{cli::*, ApiMethod, RpcEnvironment};
use proxmox_schema::api;
use proxmox_sys::fs::{file_get_json, image_size, replace_file, CreateOptions};
@@ -1962,7 +1961,8 @@ impl ReadAt for BufferedDynamicReadAt {
fn main() {
pbs_tools::setup_libc_malloc_opts();
- init_cli_logger("PBS_LOG", proxmox_log::LevelFilter::INFO).expect("failed to initiate logger");
+ proxmox_log::init::stderr_or_tasklog("PBS_LOG", proxmox_log::LevelFilter::INFO)
+ .expect("failed to initiate logger");
let backup_cmd_def = CliCommand::new(&API_METHOD_CREATE_BACKUP)
.arg_param(&["backupspec"])
diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
index 572e2d188b42..ae8a5b2f2d7a 100644
--- a/proxmox-file-restore/src/main.rs
+++ b/proxmox-file-restore/src/main.rs
@@ -10,7 +10,6 @@ use serde_json::{json, Value};
use tokio::io::AsyncWriteExt;
use proxmox_compression::zstd::ZstdEncoder;
-use proxmox_log::init_cli_logger;
use proxmox_router::cli::{
complete_file_name, default_table_format_options, format_and_print_result_full,
get_output_format, run_cli_command, CliCommand, CliCommandMap, CliEnvironment, ColumnConfig,
@@ -629,7 +628,7 @@ fn main() {
true => proxmox_log::LevelFilter::DEBUG,
false => proxmox_log::LevelFilter::INFO,
};
- init_cli_logger("PBS_LOG", loglevel).expect("failed to initiate logger");
+ proxmox_log::init::stderr_or_tasklog("PBS_LOG", loglevel).expect("failed to initiate logger");
let list_cmd_def = CliCommand::new(&API_METHOD_LIST)
.arg_param(&["snapshot", "path"])
diff --git a/pxar-bin/src/main.rs b/pxar-bin/src/main.rs
index 7dff1e38c252..2e12226324af 100644
--- a/pxar-bin/src/main.rs
+++ b/pxar-bin/src/main.rs
@@ -22,7 +22,7 @@ use pbs_client::pxar::{
use pxar::EntryKind;
use proxmox_human_byte::HumanByte;
-use proxmox_log::{debug, enabled, error, init_cli_logger, Level};
+use proxmox_log::{debug, enabled, error, Level};
use proxmox_router::cli::*;
use proxmox_schema::api;
@@ -574,7 +574,8 @@ fn dump_archive(archive: String, payload_input: Option<String>) -> Result<(), Er
}
fn main() {
- init_cli_logger("PXAR_LOG", proxmox_log::LevelFilter::INFO).expect("failed to initiate logger");
+ proxmox_log::init::stderr_or_tasklog("PXAR_LOG", proxmox_log::LevelFilter::INFO)
+ .expect("failed to initiate logger");
let cmd_def = CliCommandMap::new()
.insert(
diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
index 7a72d49a434d..388725b1065c 100644
--- a/src/bin/proxmox-backup-api.rs
+++ b/src/bin/proxmox-backup-api.rs
@@ -8,7 +8,6 @@ use hyper::{Body, StatusCode};
use tracing::level_filters::LevelFilter;
use proxmox_lang::try_block;
-use proxmox_log::init_logger;
use proxmox_rest_server::{ApiConfig, RestServer};
use proxmox_router::RpcEnvironmentType;
use proxmox_sys::fs::CreateOptions;
@@ -41,7 +40,7 @@ fn get_index() -> Pin<Box<dyn Future<Output = Response<Body>> + Send>> {
}
async fn run() -> Result<(), Error> {
- init_logger("PBS_LOG", LevelFilter::INFO)?;
+ proxmox_log::init::journald_or_tasklog("PBS_LOG", LevelFilter::INFO)?;
config::create_configdir()?;
diff --git a/src/bin/proxmox-backup-debug.rs b/src/bin/proxmox-backup-debug.rs
index 35ad11c70e7e..4be910dc48f9 100644
--- a/src/bin/proxmox-backup-debug.rs
+++ b/src/bin/proxmox-backup-debug.rs
@@ -1,4 +1,3 @@
-use proxmox_log::init_cli_logger;
use proxmox_router::{
cli::{run_cli_command, CliCommandMap, CliEnvironment},
RpcEnvironment,
@@ -8,7 +7,8 @@ mod proxmox_backup_debug;
use proxmox_backup_debug::*;
fn main() {
- init_cli_logger("PBS_LOG", proxmox_log::LevelFilter::INFO).expect("failed to initiate logger");
+ proxmox_log::init::stderr_or_tasklog("PBS_LOG", proxmox_log::LevelFilter::INFO)
+ .expect("failed to initiate logger");
let cmd_def = CliCommandMap::new()
.insert("inspect", inspect::inspect_commands())
diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index 02ca0d028225..ac240e138591 100644
--- a/src/bin/proxmox-backup-manager.rs
+++ b/src/bin/proxmox-backup-manager.rs
@@ -3,7 +3,6 @@ use std::io::{self, Write};
use std::str::FromStr;
use anyhow::{format_err, Error};
-use proxmox_log::init_cli_logger;
use serde_json::{json, Value};
use proxmox_router::{cli::*, RpcEnvironment};
@@ -618,7 +617,7 @@ async fn get_versions(verbose: bool, param: Value) -> Result<Value, Error> {
}
async fn run() -> Result<(), Error> {
- init_cli_logger("PBS_LOG", proxmox_log::LevelFilter::INFO)?;
+ proxmox_log::init::stderr_or_tasklog("PBS_LOG", proxmox_log::LevelFilter::INFO)?;
proxmox_backup::server::notifications::init()?;
let cmd_def = CliCommandMap::new()
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index ce1be1c0d8d9..9110013e45ec 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -16,7 +16,6 @@ use openssl::ssl::SslAcceptor;
use serde_json::{json, Value};
use proxmox_lang::try_block;
-use proxmox_log::init_logger;
use proxmox_router::{RpcEnvironment, RpcEnvironmentType};
use proxmox_sys::fs::CreateOptions;
use proxmox_sys::logrotate::LogRotate;
@@ -179,7 +178,7 @@ async fn get_index_future(env: RestEnvironment, parts: Parts) -> Response<Body>
}
async fn run() -> Result<(), Error> {
- init_logger("PBS_LOG", LevelFilter::INFO)?;
+ proxmox_log::init::journald_or_tasklog("PBS_LOG", LevelFilter::INFO)?;
proxmox_backup::auth_helpers::setup_auth_context(false);
proxmox_backup::server::notifications::init()?;
diff --git a/src/bin/proxmox-tape.rs b/src/bin/proxmox-tape.rs
index 8e8584b35637..930a4e0b44cc 100644
--- a/src/bin/proxmox-tape.rs
+++ b/src/bin/proxmox-tape.rs
@@ -5,7 +5,6 @@ use serde_json::{json, Value};
use proxmox_human_byte::HumanByte;
use proxmox_io::ReadExt;
-use proxmox_log::init_cli_logger;
use proxmox_router::cli::*;
use proxmox_router::RpcEnvironment;
use proxmox_schema::api;
@@ -998,7 +997,8 @@ async fn catalog_media(mut param: Value) -> Result<(), Error> {
}
fn main() {
- init_cli_logger("PBS_LOG", proxmox_log::LevelFilter::INFO).expect("failed to initiate logger");
+ proxmox_log::init::stderr_or_tasklog("PBS_LOG", proxmox_log::LevelFilter::INFO)
+ .expect("failed to initiate logger");
let cmd_def = CliCommandMap::new()
.insert(
diff --git a/src/bin/sg-tape-cmd.rs b/src/bin/sg-tape-cmd.rs
index cd14b660a68a..933290c7c663 100644
--- a/src/bin/sg-tape-cmd.rs
+++ b/src/bin/sg-tape-cmd.rs
@@ -10,7 +10,6 @@ use pbs_tape::sg_tape::SgTape;
use proxmox_backup::tape::encryption_keys::load_key;
use serde_json::Value;
-use proxmox_log::init_cli_logger;
use proxmox_router::{cli::*, RpcEnvironment};
use proxmox_schema::api;
use proxmox_uuid::Uuid;
@@ -125,7 +124,7 @@ fn set_encryption(
}
fn main() -> Result<(), Error> {
- init_cli_logger("PBS_LOG", proxmox_log::LevelFilter::INFO)?;
+ proxmox_log::init::stderr_or_tasklog("PBS_LOG", proxmox_log::LevelFilter::INFO)?;
// check if we are user root or backup
let backup_uid = pbs_config::backup_user()?.uid;
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [pbs-devel] [PATCH proxmox-perl-rs v2 4/4] log: use new init function, print to stderr and journald
2024-12-09 10:46 [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper Gabriel Goller
` (2 preceding siblings ...)
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox-backup v2 3/4] log: use new init functions Gabriel Goller
@ 2024-12-09 10:46 ` Gabriel Goller
2025-01-09 10:09 ` [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper Lukas Wagner
2025-01-14 8:43 ` Gabriel Goller
5 siblings, 0 replies; 19+ messages in thread
From: Gabriel Goller @ 2024-12-09 10:46 UTC (permalink / raw)
To: pbs-devel; +Cc: Lukas Wagner
Use the new init function. Print the logs to stderr and journald always.
Remove the log dependency.
Suggested-by: Lukas Wagner <l.wagner@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
common/src/logger.rs | 4 +++-
pmg-rs/Cargo.toml | 1 -
pmg-rs/src/lib.rs | 2 +-
pve-rs/Cargo.toml | 1 -
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/common/src/logger.rs b/common/src/logger.rs
index 1c8940ba4588..5edb08a45f39 100644
--- a/common/src/logger.rs
+++ b/common/src/logger.rs
@@ -5,7 +5,9 @@ pub fn init(env_var_name: &str, default_log_level: &str) {
if let Err(e) = default_log_level
.parse()
.map_err(Error::from)
- .and_then(|default_log_level| proxmox_log::init_logger(env_var_name, default_log_level))
+ .and_then(|default_log_level| {
+ proxmox_log::init::stderr_and_journald_with_pve_format(env_var_name, default_log_level)
+ })
{
eprintln!("could not set up env_logger: {e:?}");
}
diff --git a/pmg-rs/Cargo.toml b/pmg-rs/Cargo.toml
index 12526714f060..137434d227ac 100644
--- a/pmg-rs/Cargo.toml
+++ b/pmg-rs/Cargo.toml
@@ -18,7 +18,6 @@ anyhow = "1.0"
hex = "0.4"
http = "0.2.7"
libc = "0.2"
-log = "0.4.17"
nix = "0.26"
openssl = "0.10.40"
serde = "1.0"
diff --git a/pmg-rs/src/lib.rs b/pmg-rs/src/lib.rs
index 3db6966ab263..0ebe2855f8c4 100644
--- a/pmg-rs/src/lib.rs
+++ b/pmg-rs/src/lib.rs
@@ -29,7 +29,7 @@ mod export {
}
pub fn send_updates_available(_updates: &[&APTUpdateInfo]) -> Result<(), Error> {
- log::warn!("update notifications are not implemented for PMG yet");
+ tracing::warn!("update notifications are not implemented for PMG yet");
Ok(())
}
diff --git a/pve-rs/Cargo.toml b/pve-rs/Cargo.toml
index 4b6dec6ff452..8eb7bbdf61eb 100644
--- a/pve-rs/Cargo.toml
+++ b/pve-rs/Cargo.toml
@@ -20,7 +20,6 @@ base64 = "0.13"
hex = "0.4"
http = "0.2.7"
libc = "0.2"
-log = "0.4.17"
nix = "0.26"
openssl = "0.10.40"
serde = "1.0"
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper
2024-12-09 10:46 [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper Gabriel Goller
` (3 preceding siblings ...)
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox-perl-rs v2 4/4] log: use new init function, print to stderr and journald Gabriel Goller
@ 2025-01-09 10:09 ` Lukas Wagner
2025-01-14 8:43 ` Gabriel Goller
5 siblings, 0 replies; 19+ messages in thread
From: Lukas Wagner @ 2025-01-09 10:09 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Gabriel Goller
On 2024-12-09 11:46, Gabriel Goller wrote:
> To make all the logging init functions easier to use renamed all and moved to
> `init` module. Like this we don't have to use a e.g. 'init_pve_logger' function
> in a shared helper, which isn't strictly true, as it can be pbs as well.
>
> The is backwards compatible as the old functions have not been removed and are
> only deprecated. This also means the first two `proxmox` patches can already be
> applied independently of the other ones.
>
Sorry for the delay in taking a look at this.
Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
(tested the pve-rs part, the changes in proxmox-backup only adapt
to the renamed function)
Thanks!
--
- Lukas
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper
2024-12-09 10:46 [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper Gabriel Goller
` (4 preceding siblings ...)
2025-01-09 10:09 ` [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper Lukas Wagner
@ 2025-01-14 8:43 ` Gabriel Goller
5 siblings, 0 replies; 19+ messages in thread
From: Gabriel Goller @ 2025-01-14 8:43 UTC (permalink / raw)
To: pbs-devel
bump
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions Gabriel Goller
@ 2025-02-10 14:37 ` Wolfgang Bumiller
2025-02-10 16:42 ` Gabriel Goller
0 siblings, 1 reply; 19+ messages in thread
From: Wolfgang Bumiller @ 2025-02-10 14:37 UTC (permalink / raw)
To: Gabriel Goller; +Cc: Lukas Wagner, pbs-devel
On Mon, Dec 09, 2024 at 11:46:03AM +0100, Gabriel Goller wrote:
> Rename and move init functions to different module. Make function names
> product-independent. Like this we don't have to use e.g. `pve_logger` in
> a shared library, which wouldn't be completely true.
>
> Suggested-by: Lukas Wagner <l.wagner@proxmox.com>
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> proxmox-log/src/init.rs | 116 ++++++++++++++++++++++++++++++++++++++++
> proxmox-log/src/lib.rs | 86 +++++++++++++++--------------
> 2 files changed, 161 insertions(+), 41 deletions(-)
> create mode 100644 proxmox-log/src/init.rs
>
> diff --git a/proxmox-log/src/init.rs b/proxmox-log/src/init.rs
> new file mode 100644
> index 000000000000..67efa9fcc81f
> --- /dev/null
> +++ b/proxmox-log/src/init.rs
> @@ -0,0 +1,116 @@
> +use tracing::{level_filters::LevelFilter, Level};
> +use tracing_log::{AsLog, LogTracer};
> +use tracing_subscriber::{filter::filter_fn, layer::SubscriberExt, Layer};
> +
> +use crate::{
> + get_env_variable, journald_or_stderr_layer, plain_stderr_layer, tasklog_layer::TasklogLayer, LogContext
> +};
> +
> +/// Inits a new tracing logger that prints to journald 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 journald if we are not in a task, or if the level of the
> +/// log is `ERROR`. 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 journald_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(
> + journald_or_stderr_layer()
> + .with_filter(filter_fn(|metadata| {
> + !LogContext::exists() || *metadata.level() == Level::ERROR
> + }))
> + .with_filter(log_level),
> + )
> + .with(TasklogLayer {}.with_filter(log_level));
> +
> + tracing::subscriber::set_global_default(registry)?;
> + LogTracer::init_with_filter(log_level.as_log())?;
> + Ok(())
> +}
> +
> +/// 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.
If this was an oversight, the functions in `lib.rs` could just call
their recommended `init::` counterparts instead of reimplementing them.
> + .with_filter(log_level),
> + )
> + .with(TasklogLayer {}.with_filter(log_level));
> +
> + tracing::subscriber::set_global_default(registry)?;
> + LogTracer::init_with_filter(log_level.as_log())?;
> + Ok(())
> +}
> +
> +/// Inits a new tracing logger that prints to stderr with the logging level specified in the
> +/// environment variable `env_var`.
> +///
> +/// 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(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(log_level));
> +
> + tracing::subscriber::set_global_default(registry)?;
> + LogTracer::init_with_filter(log_level.as_log())?;
> + Ok(())
> +}
> +
> +/// Inits a new tracing logger that prints to journald with the logging level specified in the
> +/// environment variable `env_var`.
> +///
> +/// Prints every message to journald. If journald cannot be opened, every message will land in
> +/// stderr. If `env_var` does not exist or doesn't contain a readable log level, the
> +/// `default_log_level` will be used.
> +pub fn journald(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(journald_or_stderr_layer().with_filter(log_level));
> +
> + tracing::subscriber::set_global_default(registry)?;
> + LogTracer::init_with_filter(log_level.as_log())?;
> + Ok(())
> +}
> +
> +/// Inits a new tracing logger that prints to journald and 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 will always write to journald and tasklog, even if we are in a task. If
> +/// `env_var` doesn't exist or can't be read, use the `default_log_level`.
> +pub fn journald_and_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(journald_or_stderr_layer().with_filter(log_level))
> + .with(TasklogLayer {}.with_filter(log_level));
> +
> + tracing::subscriber::set_global_default(registry)?;
> + LogTracer::init_with_filter(log_level.as_log())?;
> + Ok(())
> +}
> diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs
> index 8c74e42b618d..50f63699336f 100644
> --- a/proxmox-log/src/lib.rs
> +++ b/proxmox-log/src/lib.rs
> @@ -5,18 +5,18 @@ use std::env;
> use std::future::Future;
> use std::sync::{Arc, Mutex};
>
> +use tasklog_layer::TasklogLayer;
> use tokio::task::futures::TaskLocalFuture;
> use tracing_log::{AsLog, LogTracer};
> use tracing_subscriber::filter::filter_fn;
> use tracing_subscriber::prelude::*;
>
> -use tasklog_layer::TasklogLayer;
> -
> mod file_logger;
> -pub use file_logger::{FileLogOptions, FileLogger};
> -
> mod tasklog_layer;
>
> +pub mod init;
> +pub use file_logger::{FileLogOptions, FileLogger};
> +
> pub use tracing::debug;
> pub use tracing::debug_span;
> pub use tracing::enabled;
> @@ -38,36 +38,6 @@ tokio::task_local! {
> static LOG_CONTEXT: LogContext;
> }
>
> -pub fn init_logger(
> - env_var_name: &str,
> - default_log_level: LevelFilter,
> -) -> Result<(), anyhow::Error> {
> - let mut log_level = default_log_level;
> - if let Ok(v) = env::var(env_var_name) {
> - match v.parse::<LevelFilter>() {
> - Ok(l) => {
> - log_level = l;
> - }
> - Err(e) => {
> - eprintln!("env variable {env_var_name} found, but parsing failed: {e:?}");
> - }
> - }
> - }
> - let registry = tracing_subscriber::registry()
> - .with(
> - journald_or_stderr_layer()
> - .with_filter(filter_fn(|metadata| {
> - !LogContext::exists() || *metadata.level() == Level::ERROR
> - }))
> - .with_filter(log_level),
> - )
> - .with(TasklogLayer {}.with_filter(log_level));
> -
> - tracing::subscriber::set_global_default(registry)?;
> - LogTracer::init_with_filter(log_level.as_log())?;
> - Ok(())
> -}
> -
> /// A file logger and warnings counter which can be used across a scope for separate logging.
> /// Mainly used for worker-task logging.
> pub struct FileLogState {
> @@ -160,22 +130,56 @@ where
> .with_writer(std::io::stderr)
> }
>
> -/// Initialize default logger for CLI binaries
> -pub fn init_cli_logger(
> - env_var_name: &str,
> - default_log_level: LevelFilter,
> -) -> Result<(), anyhow::Error> {
> +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_name) {
> + if let Ok(v) = env::var(env_var) {
> match v.parse::<LevelFilter>() {
> Ok(l) => {
> log_level = l;
> }
> Err(e) => {
> - eprintln!("env variable {env_var_name} found, but parsing failed: {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.
> +///
> +/// Check the (tokio) LogContext and print to either journald or the Tasklog.
> +#[deprecated(note = "Use the init::journald_or_tasklog function")]
> +pub fn init_logger(
> + env_var_name: &str,
> + default_log_level: LevelFilter,
> +) -> Result<(), anyhow::Error> {
> + let log_level = get_env_variable(env_var_name, default_log_level);
> +
> + let registry = tracing_subscriber::registry()
> + .with(
> + journald_or_stderr_layer()
> + .with_filter(filter_fn(|metadata| {
> + !LogContext::exists() || *metadata.level() == Level::ERROR
> + }))
> + .with_filter(log_level),
> + )
> + .with(TasklogLayer {}.with_filter(log_level));
> +
> + tracing::subscriber::set_global_default(registry)?;
> + LogTracer::init_with_filter(log_level.as_log())?;
> + Ok(())
> +}
> +
> +/// Initialize default tracing logger for CLI binaries.
> +///
> +/// Prints to stderr and to the tasklog if we are in a pbs workertask.
> +#[deprecated(note = "Use the init::stderr_or_tasklog function")]
> +pub fn init_cli_logger(
> + env_var_name: &str,
> + default_log_level: LevelFilter,
> +) -> Result<(), anyhow::Error> {
> + let log_level = get_env_variable(env_var_name, default_log_level);
>
> let registry = tracing_subscriber::registry()
> .with(
> --
> 2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2025-02-10 14:37 ` Wolfgang Bumiller
@ 2025-02-10 16:42 ` Gabriel Goller
2025-02-11 9:22 ` Wolfgang Bumiller
0 siblings, 1 reply; 19+ messages in thread
From: Gabriel Goller @ 2025-02-10 16:42 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: Lukas Wagner, pbs-devel
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 :(
>If this was an oversight, the functions in `lib.rs` could just call
>their recommended `init::` counterparts instead of reimplementing them.
Yep, will do that as well!
>> + .with_filter(log_level),
>> + )
>> + .with(TasklogLayer {}.with_filter(log_level));
>> +
>> + tracing::subscriber::set_global_default(registry)?;
>> + LogTracer::init_with_filter(log_level.as_log())?;
>> + Ok(())
>> +}
Thanks for the review!
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2025-02-10 16:42 ` Gabriel Goller
@ 2025-02-11 9:22 ` Wolfgang Bumiller
2025-02-11 9:28 ` Wolfgang Bumiller
2025-02-11 9:31 ` Gabriel Goller
0 siblings, 2 replies; 19+ messages in thread
From: Wolfgang Bumiller @ 2025-02-11 9:22 UTC (permalink / raw)
To: Gabriel Goller; +Cc: Lukas Wagner, pbs-devel
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)...
>
> > If this was an oversight, the functions in `lib.rs` could just call
> > their recommended `init::` counterparts instead of reimplementing them.
>
> Yep, will do that as well!
>
> > > + .with_filter(log_level),
> > > + )
> > > + .with(TasklogLayer {}.with_filter(log_level));
> > > +
> > > + tracing::subscriber::set_global_default(registry)?;
> > > + LogTracer::init_with_filter(log_level.as_log())?;
> > > + Ok(())
> > > +}
>
> Thanks for the review!
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2025-02-11 9:22 ` Wolfgang Bumiller
@ 2025-02-11 9:28 ` Wolfgang Bumiller
2025-02-17 13:08 ` Gabriel Goller
2025-02-11 9:31 ` Gabriel Goller
1 sibling, 1 reply; 19+ messages in thread
From: Wolfgang Bumiller @ 2025-02-11 9:28 UTC (permalink / raw)
To: Gabriel Goller; +Cc: Lukas Wagner, pbs-devel
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.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2025-02-11 9:22 ` Wolfgang Bumiller
2025-02-11 9:28 ` Wolfgang Bumiller
@ 2025-02-11 9:31 ` Gabriel Goller
1 sibling, 0 replies; 19+ messages in thread
From: Gabriel Goller @ 2025-02-11 9:31 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: Lukas Wagner, pbs-devel
On 11.02.2025 10:22, 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.)
Yep, maybe I'll make it more modular in the future...
>Perhaps the journal/tasklog one could be named "init_daemon_log" (or
>just have an alias under that name)...
I'd be fine with an alias, but why 'init_daemon_log' though?
>> > If this was an oversight, the functions in `lib.rs` could just call
>> > their recommended `init::` counterparts instead of reimplementing them.
>>
>> Yep, will do that as well!
>>
>> > > + .with_filter(log_level),
>> > > + )
>> > > + .with(TasklogLayer {}.with_filter(log_level));
>> > > +
>> > > + tracing::subscriber::set_global_default(registry)?;
>> > > + LogTracer::init_with_filter(log_level.as_log())?;
>> > > + Ok(())
>> > > +}
>>
>> Thanks for the review!
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2025-02-11 9:28 ` Wolfgang Bumiller
@ 2025-02-17 13:08 ` Gabriel Goller
2025-02-17 13:38 ` Wolfgang Bumiller
0 siblings, 1 reply; 19+ messages in thread
From: Gabriel Goller @ 2025-02-17 13:08 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: Lukas Wagner, pbs-devel
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2025-02-17 13:08 ` Gabriel Goller
@ 2025-02-17 13:38 ` Wolfgang Bumiller
2025-02-17 14:12 ` Gabriel Goller
0 siblings, 1 reply; 19+ messages in thread
From: Wolfgang Bumiller @ 2025-02-17 13:38 UTC (permalink / raw)
To: Gabriel Goller; +Cc: Lukas Wagner, pbs-devel
On Mon, Feb 17, 2025 at 02:08:03PM +0100, Gabriel Goller wrote:
> 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?
Hmmm...
Those names are a bit long, and still as specific as before, so I'm not
sure we win a lot either way.
I'm wondering - if we really have so many specific cases - do we really
need them implemented in this crate, rather than where they are used?
How many different types of logging layers do we have and where atm?
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2025-02-17 13:38 ` Wolfgang Bumiller
@ 2025-02-17 14:12 ` Gabriel Goller
2025-02-17 14:51 ` Wolfgang Bumiller
0 siblings, 1 reply; 19+ messages in thread
From: Gabriel Goller @ 2025-02-17 14:12 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: Lukas Wagner, pbs-devel
On 17.02.2025 14:38, Wolfgang Bumiller wrote:
>On Mon, Feb 17, 2025 at 02:08:03PM +0100, Gabriel Goller wrote:
>> 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?
>
>Hmmm...
>Those names are a bit long, and still as specific as before, so I'm not
>sure we win a lot either way.
>
>I'm wondering - if we really have so many specific cases - do we really
>need them implemented in this crate, rather than where they are used?
>How many different types of logging layers do we have and where atm?
We currently have:
1) journald (with stderr fallback)
2) stderr
3) stderr (with pve formatting)
4) pbs tasklog
But the problem is we have different combinations and filters as well:
journald and stderr (perlmod), stderr and pbs tasklog (but only when we
are in a tasklog or when the level is error) (pbs-client), etc.
The logging gets initiated in:
* pbs proxy daemon
* pbs api daemon
* pbs client
* pbs tape pmt
* pbs tape pmtx
* pbs proxmox-file-restore
* pbs pxar
* pbs proxmox-backup-debug
* pbs proxmox-backup-manager
* pbs proxmox-backup-tape
* pbs sg-tape-cmd
* pbs proxmox-daily-update
* perlmod
Exposing the builder directly without any helper functions would be fine
as well I reckon. The downside is that the initiation gets more
"complicated", e.g. (the pbs daemon):
LogBuilder::from_env("PBS_LOG", LevelFilter::INFO)
.journald_on_no_tasklog_or_error()
.tasklog().init()?;
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2025-02-17 14:12 ` Gabriel Goller
@ 2025-02-17 14:51 ` Wolfgang Bumiller
2025-02-17 15:21 ` Gabriel Goller
0 siblings, 1 reply; 19+ messages in thread
From: Wolfgang Bumiller @ 2025-02-17 14:51 UTC (permalink / raw)
To: Gabriel Goller; +Cc: Lukas Wagner, pbs-devel
On Mon, Feb 17, 2025 at 03:12:23PM +0100, Gabriel Goller wrote:
> On 17.02.2025 14:38, Wolfgang Bumiller wrote:
> > On Mon, Feb 17, 2025 at 02:08:03PM +0100, Gabriel Goller wrote:
> > > 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?
> >
> > Hmmm...
> > Those names are a bit long, and still as specific as before, so I'm not
> > sure we win a lot either way.
> >
> > I'm wondering - if we really have so many specific cases - do we really
> > need them implemented in this crate, rather than where they are used?
> > How many different types of logging layers do we have and where atm?
>
> We currently have:
> 1) journald (with stderr fallback)
=> builder.journald_logging()
(the fallback could just be implied, or do we ever not want that?)
> 2) stderr
=> builder.stderr_logging()
> 3) stderr (with pve formatting)
=> builder.pve_logging() (#[cfg(feature = "pve")]?)
> 4) pbs tasklog
^ I assume (4) is the "tasklog-or-journald" case?
Not sure where (1) is really the right choice.
Does (4) exist anywhere other than in the API daemons?
=> builder.task_logging()
(the journal-if-not-in-task part could be implied - or do we ever need
to combine the tasklog part differently?)
>
> But the problem is we have different combinations and filters as well:
> journald and stderr (perlmod), stderr and pbs tasklog (but only when we
> are in a tasklog or when the level is error) (pbs-client), etc.
^ Why is "pbs-client" the example here? IMO this sounds like the API
daemons. The pbs-client *CLI* tool certainly should just log to stderr,
and the "crate" otherwise doesn't decide this.
>
> The logging gets initiated in:
(list below is reordered)
> * pbs proxy daemon
> * pbs api daemon
^ API daemons - so those are case (4) - are they different from one
another?
> * pbs proxmox-file-restore
^ The VM side? Not sure what it needs, but seems special-enough to have
its own code, unless we just log to the journal anyway
> * pbs tape pmt
> * pbs tape pmtx
> * pbs client
> * pbs pxar
> * pbs proxmox-backup-debug
> * pbs proxmox-backup-manager
> * pbs proxmox-backup-tape
> * pbs sg-tape-cmd
^ IIRC all of these are CLI tools and should therefore all be case (2) -
although I don't know about how the tape stuff works.
If they do anything else, it would be good to know why and have this
documented either in proxmox-log or in their logging-init functions.
> * pbs proxmox-daily-update
The 'daily-update' may be a special case and could use the journal
directly, but may as well be stderr->journald via its `.service`.
But, yeah, obviously it would make actual tracing/debugging easier with
a proper journald-logger here. So another user of case (1).
> * perlmod
^ For the lack of a better place (as pve, pmg and nftables code probably
all want to share it), having this as a special function in proxmox-log
makes some sense I suppose (but could be feature-guarded).
>
> Exposing the builder directly without any helper functions would be fine
> as well I reckon. The downside is that the initiation gets more
> "complicated", e.g. (the pbs daemon):
>
> LogBuilder::from_env("PBS_LOG", LevelFilter::INFO)
> .journald_on_no_tasklog_or_error()
> .tasklog().init()?;
So yes this is probably okay to have, but I really don't think we need
these huge names - like I said, it doesn't make sense to me that
`.tasklog()` should be combined with anything other than this exact one
other thing anyway, so the middle line there could just be left out IMO.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2025-02-17 14:51 ` Wolfgang Bumiller
@ 2025-02-17 15:21 ` Gabriel Goller
2025-02-18 10:06 ` Wolfgang Bumiller
0 siblings, 1 reply; 19+ messages in thread
From: Gabriel Goller @ 2025-02-17 15:21 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: Lukas Wagner, pbs-devel
>> > > <snip>
>> > >
>> > > 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?
>> >
>> > Hmmm...
>> > Those names are a bit long, and still as specific as before, so I'm not
>> > sure we win a lot either way.
>> >
>> > I'm wondering - if we really have so many specific cases - do we really
>> > need them implemented in this crate, rather than where they are used?
>> > How many different types of logging layers do we have and where atm?
>>
>> We currently have:
>> 1) journald (with stderr fallback)
>
>=> builder.journald_logging()
>
>(the fallback could just be implied, or do we ever not want that?)
true, I don't think we want to fail anywhere if there is no journald
anyway.
>> 2) stderr
>
>=> builder.stderr_logging()
>
>> 3) stderr (with pve formatting)
>
>=> builder.pve_logging() (#[cfg(feature = "pve")]?)
Maybe stderr_pve_logging? I'd like to mention 'stderr' somewhere in the
case we add some different pve-format logging (e.g. pve-tasklog).
>> 4) pbs tasklog
>^ I assume (4) is the "tasklog-or-journald" case?
>Not sure where (1) is really the right choice.
>
>Does (4) exist anywhere other than in the API daemons?
>
>=> builder.task_logging()
>
>(the journal-if-not-in-task part could be implied - or do we ever need
>to combine the tasklog part differently?)
Yes, in the proxmox-backup-manager. There we use stderr for all the
normal logs and the tasklogger for when we a task is started.
>> But the problem is we have different combinations and filters as well:
>> journald and stderr (perlmod), stderr and pbs tasklog (but only when we
>> are in a tasklog or when the level is error) (pbs-client), etc.
>
>^ Why is "pbs-client" the example here? IMO this sounds like the API
>daemons. The pbs-client *CLI* tool certainly should just log to stderr,
>and the "crate" otherwise doesn't decide this.
Oops, my bad I meant the proxmox-backup-manager.
>> The logging gets initiated in:
>
>(list below is reordered)
>
>> * pbs proxy daemon
>> * pbs api daemon
>
>^ API daemons - so those are case (4) - are they different from one
>another?
Nope, this is case 4. (To be exact case 1 + 4, journald + tasklog.)
Also you need to add filters here, this isn't just a "print everything
to journald", we only print to journald when the level is error or we
are *not* in a tasklog.
>> * pbs proxmox-file-restore
>
>^ The VM side? Not sure what it needs, but seems special-enough to have
>its own code, unless we just log to the journal anyway
As far as I remember this one should print to stderr only. (This is
wrong in this patch.)
>> * pbs tape pmt
>> * pbs tape pmtx
>> * pbs client
>> * pbs pxar
>> * pbs proxmox-backup-debug
>> * pbs proxmox-backup-manager
>> * pbs proxmox-backup-tape
>> * pbs sg-tape-cmd
>
>^ IIRC all of these are CLI tools and should therefore all be case (2) -
>although I don't know about how the tape stuff works.
>If they do anything else, it would be good to know why and have this
>documented either in proxmox-log or in their logging-init functions.
All except the proxmox-backup-manager as far as I can see, because that
one starts pbs tasks directly.
>> * pbs proxmox-daily-update
>
>The 'daily-update' may be a special case and could use the journal
>directly, but may as well be stderr->journald via its `.service`.
>But, yeah, obviously it would make actual tracing/debugging easier with
>a proper journald-logger here. So another user of case (1).
Agree.
>> * perlmod
>^ For the lack of a better place (as pve, pmg and nftables code probably
>all want to share it), having this as a special function in proxmox-log
>makes some sense I suppose (but could be feature-guarded).
There is already a shared common::init_logger function in perlmod I think.
>> Exposing the builder directly without any helper functions would be fine
>> as well I reckon. The downside is that the initiation gets more
>> "complicated", e.g. (the pbs daemon):
>>
>> LogBuilder::from_env("PBS_LOG", LevelFilter::INFO)
>> .journald_on_no_tasklog_or_error()
>> .tasklog().init()?;
>
>So yes this is probably okay to have, but I really don't think we need
>these huge names - like I said, it doesn't make sense to me that
>`.tasklog()` should be combined with anything other than this exact one
>other thing anyway, so the middle line there could just be left out IMO.
See above.
Also either we use a helper function which does everything for us (e.g.
init_perlmod_logger, init_journald_and_tasklog) or we use the builder
where we *need* to mention every layer separately. Otherwise you have different
builder functions, some which add a single layer and others that add
multiple layers + magic (which IMO is a mess).
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2025-02-17 15:21 ` Gabriel Goller
@ 2025-02-18 10:06 ` Wolfgang Bumiller
2025-02-18 16:15 ` Gabriel Goller
0 siblings, 1 reply; 19+ messages in thread
From: Wolfgang Bumiller @ 2025-02-18 10:06 UTC (permalink / raw)
To: Gabriel Goller; +Cc: Lukas Wagner, pbs-devel
On Mon, Feb 17, 2025 at 04:21:19PM +0100, Gabriel Goller wrote:
> > > > > <snip>
> > > > >
> > > > > 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?
> > > >
> > > > Hmmm...
> > > > Those names are a bit long, and still as specific as before, so I'm not
> > > > sure we win a lot either way.
> > > >
> > > > I'm wondering - if we really have so many specific cases - do we really
> > > > need them implemented in this crate, rather than where they are used?
> > > > How many different types of logging layers do we have and where atm?
> > >
> > > We currently have:
> > > 1) journald (with stderr fallback)
> >
> > => builder.journald_logging()
> >
> > (the fallback could just be implied, or do we ever not want that?)
>
> true, I don't think we want to fail anywhere if there is no journald
> anyway.
>
> > > 2) stderr
> >
> > => builder.stderr_logging()
> >
> > > 3) stderr (with pve formatting)
> >
> > => builder.pve_logging() (#[cfg(feature = "pve")]?)
>
> Maybe stderr_pve_logging? I'd like to mention 'stderr' somewhere in the
> case we add some different pve-format logging (e.g. pve-tasklog).
Sounds good.
>
> > > 4) pbs tasklog
> > ^ I assume (4) is the "tasklog-or-journald" case?
> > Not sure where (1) is really the right choice.
> >
> > Does (4) exist anywhere other than in the API daemons?
> >
> > => builder.task_logging()
> >
> > (the journal-if-not-in-task part could be implied - or do we ever need
> > to combine the tasklog part differently?)
>
> Yes, in the proxmox-backup-manager. There we use stderr for all the
> normal logs and the tasklogger for when we a task is started.
(I think this could be considered a special case, but if its case is
covered by the builder that's fine.)
>
> > > But the problem is we have different combinations and filters as well:
> > > journald and stderr (perlmod), stderr and pbs tasklog (but only when we
> > > are in a tasklog or when the level is error) (pbs-client), etc.
> >
> > ^ Why is "pbs-client" the example here? IMO this sounds like the API
> > daemons. The pbs-client *CLI* tool certainly should just log to stderr,
> > and the "crate" otherwise doesn't decide this.
>
> Oops, my bad I meant the proxmox-backup-manager.
>
> > > The logging gets initiated in:
> >
> > (list below is reordered)
> >
> > > * pbs proxy daemon
> > > * pbs api daemon
> >
> > ^ API daemons - so those are case (4) - are they different from one
> > another?
>
> Nope, this is case 4. (To be exact case 1 + 4, journald + tasklog.)
> Also you need to add filters here, this isn't just a "print everything
> to journald", we only print to journald when the level is error or we
> are *not* in a tasklog.
I did say 4 ;-)
But okay, the filters are annoying.
>
> > > * pbs proxmox-file-restore
> >
> > ^ The VM side? Not sure what it needs, but seems special-enough to have
> > its own code, unless we just log to the journal anyway
>
> As far as I remember this one should print to stderr only. (This is
> wrong in this patch.)
>
> > > * pbs tape pmt
> > > * pbs tape pmtx
> > > * pbs client
> > > * pbs pxar
> > > * pbs proxmox-backup-debug
> > > * pbs proxmox-backup-manager
> > > * pbs proxmox-backup-tape
> > > * pbs sg-tape-cmd
> >
> > ^ IIRC all of these are CLI tools and should therefore all be case (2) -
> > although I don't know about how the tape stuff works.
> > If they do anything else, it would be good to know why and have this
> > documented either in proxmox-log or in their logging-init functions.
>
> All except the proxmox-backup-manager as far as I can see, because that
> one starts pbs tasks directly.
>
> > > * pbs proxmox-daily-update
> >
> > The 'daily-update' may be a special case and could use the journal
> > directly, but may as well be stderr->journald via its `.service`.
> > But, yeah, obviously it would make actual tracing/debugging easier with
> > a proper journald-logger here. So another user of case (1).
>
> Agree.
>
> > > * perlmod
> > ^ For the lack of a better place (as pve, pmg and nftables code probably
> > all want to share it), having this as a special function in proxmox-log
> > makes some sense I suppose (but could be feature-guarded).
>
> There is already a shared common::init_logger function in perlmod I think.
I don't think the nft firewall code (and potential additional new
rust-only things we might add in the future) can access this. We have a
`proxmox-ve-rs.git` now, but that one won't be used by *pmg*.
Anyway, that part is for later.
>
> > > Exposing the builder directly without any helper functions would be fine
> > > as well I reckon. The downside is that the initiation gets more
> > > "complicated", e.g. (the pbs daemon):
> > >
> > > LogBuilder::from_env("PBS_LOG", LevelFilter::INFO)
> > > .journald_on_no_tasklog_or_error()
> > > .tasklog().init()?;
> >
> > So yes this is probably okay to have, but I really don't think we need
> > these huge names - like I said, it doesn't make sense to me that
> > `.tasklog()` should be combined with anything other than this exact one
> > other thing anyway, so the middle line there could just be left out IMO.
>
> See above.
> Also either we use a helper function which does everything for us (e.g.
> init_perlmod_logger, init_journald_and_tasklog) or we use the builder
> where we *need* to mention every layer separately. Otherwise you have different
> builder functions, some which add a single layer and others that add
> multiple layers + magic (which IMO is a mess).
Yeah. I suppose we can proceed with the builder.
It might be good to have the 4 types above summarised in the builder
type's doc comments.
As for a builder *module* (which you mentioned in an earlier mail) - I
*think* API-wise, the builder type itself could very well live (as in be
re-exported) at the top level, but implementation wise it could be in
its own module for sure, if it makes the code more tidy/organized in
your opinion.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions
2025-02-18 10:06 ` Wolfgang Bumiller
@ 2025-02-18 16:15 ` Gabriel Goller
0 siblings, 0 replies; 19+ messages in thread
From: Gabriel Goller @ 2025-02-18 16:15 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: Lukas Wagner, pbs-devel
>> > > * pbs tape pmt
>> > > * pbs tape pmtx
>> > > * pbs client
>> > > * pbs pxar
>> > > * pbs proxmox-backup-debug
>> > > * pbs proxmox-backup-manager
>> > > * pbs proxmox-backup-tape
>> > > * pbs sg-tape-cmd
>> >
>> > ^ IIRC all of these are CLI tools and should therefore all be case (2) -
>> > although I don't know about how the tape stuff works.
>> > If they do anything else, it would be good to know why and have this
>> > documented either in proxmox-log or in their logging-init functions.
>>
>> All except the proxmox-backup-manager as far as I can see, because that
>> one starts pbs tasks directly.
>>
>> > > * pbs proxmox-daily-update
>> >
>> > The 'daily-update' may be a special case and could use the journal
>> > directly, but may as well be stderr->journald via its `.service`.
>> > But, yeah, obviously it would make actual tracing/debugging easier with
>> > a proper journald-logger here. So another user of case (1).
>>
>> Agree.
Nevermind actually, proxmox-daily-update calls a workertask, so we need
journald and tasklog layer, similar to the daemon's setup.
Sent a new series: https://lore.proxmox.com/pve-devel/20250218161311.558674-1-g.goller@proxmox.com/
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2025-02-18 16:16 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-09 10:46 [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper Gabriel Goller
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox v2 1/4] log: rename/move init functions Gabriel Goller
2025-02-10 14:37 ` Wolfgang Bumiller
2025-02-10 16:42 ` Gabriel Goller
2025-02-11 9:22 ` Wolfgang Bumiller
2025-02-11 9:28 ` Wolfgang Bumiller
2025-02-17 13:08 ` Gabriel Goller
2025-02-17 13:38 ` Wolfgang Bumiller
2025-02-17 14:12 ` Gabriel Goller
2025-02-17 14:51 ` Wolfgang Bumiller
2025-02-17 15:21 ` Gabriel Goller
2025-02-18 10:06 ` Wolfgang Bumiller
2025-02-18 16:15 ` Gabriel Goller
2025-02-11 9:31 ` Gabriel Goller
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox v2 2/4] log: add logger for perlmod crates Gabriel Goller
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox-backup v2 3/4] log: use new init functions Gabriel Goller
2024-12-09 10:46 ` [pbs-devel] [PATCH proxmox-perl-rs v2 4/4] log: use new init function, print to stderr and journald Gabriel Goller
2025-01-09 10:09 ` [pbs-devel] [PATCH proxmox{, -backup, -perl-rs} v2 0/4] Rename/Move logging init helper Lukas Wagner
2025-01-14 8:43 ` Gabriel Goller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox