* [pbs-devel] [PATCH v2] add tracing init_cli_logger and deprecate old one @ 2024-08-29 13:40 Gabriel Goller 2024-08-29 13:40 ` [pbs-devel] [PATCH proxmox-backup v2 1/2] move client binaries to tracing Gabriel Goller 2024-08-30 11:58 ` [pbs-devel] applied: [PATCH v2] add tracing init_cli_logger and deprecate old one Wolfgang Bumiller 0 siblings, 2 replies; 9+ messages in thread From: Gabriel Goller @ 2024-08-29 13:40 UTC (permalink / raw) To: pbs-devel Deprecate the proxmox-router init_cli_logger function used in client binaries such as `proxmox-backup-client`, `proxmox-backup-manager`, 'pxar', etc... Add a new init_cli_logger function that uses tracing instead of env_logger. It checks if the task is in a workertask and prints the message either to stdout or to the tasklog (this is neccessary for commands in proxmox-backup-manager that call api handlers that start workerthreads from the client). Signed-off-by: Gabriel Goller <g.goller@proxmox.com> --- v2, thanks @Wolfgang: - order imports - don't delete old function, but deprecate - add stderr warning when log level is not parsable proxmox-log/src/lib.rs | 42 ++++++++++++++++++++++++++++++++++- proxmox-router/src/cli/mod.rs | 1 + 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs index 796d10145b9e..bcde5495dead 100644 --- a/proxmox-log/src/lib.rs +++ b/proxmox-log/src/lib.rs @@ -8,7 +8,7 @@ use std::sync::{Arc, Mutex}; use tokio::task::futures::TaskLocalFuture; use tracing::Level; use tracing_log::{AsLog, LogTracer}; -use tracing_subscriber::filter::{filter_fn, LevelFilter}; +use tracing_subscriber::filter::filter_fn; use tracing_subscriber::prelude::*; use tasklog_layer::TasklogLayer; @@ -32,6 +32,7 @@ pub use tracing::trace; pub use tracing::trace_span; pub use tracing::warn; pub use tracing::warn_span; +pub use tracing_subscriber::filter::LevelFilter; tokio::task_local! { static LOG_CONTEXT: LogContext; @@ -125,3 +126,42 @@ impl LogContext { &self.logger } } + +/// Initialize default logger for CLI binaries +pub fn init_cli_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 PBS_LOG found, but parsing failed: {e:?}"); + } + } + } + + let format = tracing_subscriber::fmt::format() + .with_level(false) + .without_time() + .with_target(false) + .compact(); + + let registry = tracing_subscriber::registry() + .with( + tracing_subscriber::fmt::layer() + .event_format(format) + .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(()) +} diff --git a/proxmox-router/src/cli/mod.rs b/proxmox-router/src/cli/mod.rs index 3cc41ab3ea94..2b5a69c83dce 100644 --- a/proxmox-router/src/cli/mod.rs +++ b/proxmox-router/src/cli/mod.rs @@ -58,6 +58,7 @@ pub use readline::*; pub type CompletionFunction = fn(&str, &HashMap<String, String>) -> Vec<String>; /// Initialize default logger for CLI binaries +#[deprecated = "use proxmox_log::init_cli_logger instead"] pub fn init_cli_logger(env_var_name: &str, default_log_level: &str) { env_logger::Builder::from_env( env_logger::Env::new().filter_or(env_var_name, default_log_level), -- 2.39.2 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 1/2] move client binaries to tracing 2024-08-29 13:40 [pbs-devel] [PATCH v2] add tracing init_cli_logger and deprecate old one Gabriel Goller @ 2024-08-29 13:40 ` Gabriel Goller 2024-08-29 13:41 ` Gabriel Goller 2024-08-30 11:58 ` [pbs-devel] applied: [PATCH v2] add tracing init_cli_logger and deprecate old one Wolfgang Bumiller 1 sibling, 1 reply; 9+ messages in thread From: Gabriel Goller @ 2024-08-29 13:40 UTC (permalink / raw) To: pbs-devel Add tracing logger to all client binaries and remove env_logger. The reason for this change is twofold: our migration to tracing, and the behavior when the client calls an api handler directly. Currently the proxmox-backup-manager calls the api handlers directly for some commands. This results in no output (on console and task log), as no tracing logger is instantiated. Signed-off-by: Gabriel Goller <g.goller@proxmox.com> --- pbs-tape/Cargo.toml | 1 + pbs-tape/src/bin/pmt.rs | 3 ++- pbs-tape/src/bin/pmtx.rs | 3 ++- proxmox-backup-client/Cargo.toml | 1 + proxmox-backup-client/src/main.rs | 3 ++- proxmox-file-restore/Cargo.toml | 1 + proxmox-file-restore/src/main.rs | 11 ++++++----- pxar-bin/Cargo.toml | 1 + pxar-bin/src/main.rs | 3 ++- src/bin/proxmox-backup-debug.rs | 5 +++-- src/bin/proxmox-backup-manager.rs | 3 ++- src/bin/proxmox-tape.rs | 3 ++- src/bin/sg-tape-cmd.rs | 3 ++- 13 files changed, 27 insertions(+), 14 deletions(-) diff --git a/pbs-tape/Cargo.toml b/pbs-tape/Cargo.toml index 142bbacd7daf..4f153fedad8f 100644 --- a/pbs-tape/Cargo.toml +++ b/pbs-tape/Cargo.toml @@ -23,6 +23,7 @@ udev.workspace = true proxmox-io.workspace = true proxmox-lang.workspace=true +proxmox-log.workspace=true proxmox-sys.workspace = true proxmox-time.workspace = true proxmox-uuid.workspace = true diff --git a/pbs-tape/src/bin/pmt.rs b/pbs-tape/src/bin/pmt.rs index 4a5e08e5ea48..9e39dbe16b2f 100644 --- a/pbs-tape/src/bin/pmt.rs +++ b/pbs-tape/src/bin/pmt.rs @@ -15,6 +15,7 @@ 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}; @@ -799,7 +800,7 @@ fn options( } fn main() -> Result<(), Error> { - init_cli_logger("PBS_LOG", "info"); + init_cli_logger("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 6f34bc448ad5..303353e6bfd3 100644 --- a/pbs-tape/src/bin/pmtx.rs +++ b/pbs-tape/src/bin/pmtx.rs @@ -16,6 +16,7 @@ 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; @@ -387,7 +388,7 @@ fn scan(param: Value) -> Result<(), Error> { } fn main() -> Result<(), Error> { - init_cli_logger("PBS_LOG", "info"); + init_cli_logger("PBS_LOG", proxmox_log::LevelFilter::INFO)?; let uid = nix::unistd::Uid::current(); diff --git a/proxmox-backup-client/Cargo.toml b/proxmox-backup-client/Cargo.toml index 62dd97f9ce21..a91a4908ba05 100644 --- a/proxmox-backup-client/Cargo.toml +++ b/proxmox-backup-client/Cargo.toml @@ -24,6 +24,7 @@ pxar.workspace = true proxmox-async.workspace = true proxmox-human-byte.workspace = true +proxmox-log.workspace = true proxmox-io.workspace = true proxmox-router = { workspace = true, features = [ "cli" ] } proxmox-schema = { workspace = true, features = [ "api-macro" ] } diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs index 5edb2a824ef9..b59b560e6834 100644 --- a/proxmox-backup-client/src/main.rs +++ b/proxmox-backup-client/src/main.rs @@ -17,6 +17,7 @@ use pathpatterns::{MatchEntry, MatchType, PatternFlag}; use proxmox_async::blocking::TokioWriterAdapter; use proxmox_human_byte::HumanByte; 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}; @@ -1953,7 +1954,7 @@ impl ReadAt for BufferedDynamicReadAt { fn main() { pbs_tools::setup_libc_malloc_opts(); - init_cli_logger("PBS_LOG", "info"); + init_cli_logger("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/Cargo.toml b/proxmox-file-restore/Cargo.toml index f536e38d4afd..8f99ecf9b96e 100644 --- a/proxmox-file-restore/Cargo.toml +++ b/proxmox-file-restore/Cargo.toml @@ -21,6 +21,7 @@ pxar.workspace = true proxmox-async.workspace = true proxmox-compression.workspace = true proxmox-lang.workspace=true +proxmox-log.workspace=true proxmox-router = { workspace = true, features = [ "cli" ] } proxmox-schema = { workspace = true, features = [ "api-macro" ] } proxmox-sys = { workspace = true, features = [ "logrotate" ] } diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs index 69d811fc1f0b..cda4e911c545 100644 --- a/proxmox-file-restore/src/main.rs +++ b/proxmox-file-restore/src/main.rs @@ -9,10 +9,11 @@ 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, init_cli_logger, run_cli_command, CliCommand, CliCommandMap, CliEnvironment, - ColumnConfig, OUTPUT_FORMAT, + get_output_format, run_cli_command, CliCommand, CliCommandMap, CliEnvironment, ColumnConfig, + OUTPUT_FORMAT, }; use proxmox_router::{http_err, HttpError}; use proxmox_schema::api; @@ -645,10 +646,10 @@ where fn main() { let loglevel = match qemu_helper::debug_mode() { - true => "debug", - false => "info", + true => proxmox_log::LevelFilter::DEBUG, + false => proxmox_log::LevelFilter::INFO, }; - init_cli_logger("PBS_LOG", loglevel); + init_cli_logger("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/Cargo.toml b/pxar-bin/Cargo.toml index d33b7174f9f1..64dcf1496b4d 100644 --- a/pxar-bin/Cargo.toml +++ b/pxar-bin/Cargo.toml @@ -21,6 +21,7 @@ pxar.workspace = true proxmox-async.workspace = true proxmox-human-byte.workspace = true +proxmox-log.workspace = true proxmox-router = { workspace = true, features = ["cli", "server"] } proxmox-schema = { workspace = true, features = [ "api-macro" ] } proxmox-sys.workspace = true diff --git a/pxar-bin/src/main.rs b/pxar-bin/src/main.rs index 6549ccf13963..71ce3bf78f52 100644 --- a/pxar-bin/src/main.rs +++ b/pxar-bin/src/main.rs @@ -19,6 +19,7 @@ use pbs_client::pxar::{ use pxar::EntryKind; use proxmox_human_byte::HumanByte; +use proxmox_log::init_cli_logger; use proxmox_router::cli::*; use proxmox_schema::api; @@ -568,7 +569,7 @@ fn dump_archive(archive: String, payload_input: Option<String>) -> Result<(), Er } fn main() { - init_cli_logger("PXAR_LOG", "info"); + init_cli_logger("PXAR_LOG", proxmox_log::LevelFilter::INFO).expect("failed to initiate logger"); let cmd_def = CliCommandMap::new() .insert( diff --git a/src/bin/proxmox-backup-debug.rs b/src/bin/proxmox-backup-debug.rs index a3589c1673dd..35ad11c70e7e 100644 --- a/src/bin/proxmox-backup-debug.rs +++ b/src/bin/proxmox-backup-debug.rs @@ -1,5 +1,6 @@ +use proxmox_log::init_cli_logger; use proxmox_router::{ - cli::{init_cli_logger, run_cli_command, CliCommandMap, CliEnvironment}, + cli::{run_cli_command, CliCommandMap, CliEnvironment}, RpcEnvironment, }; @@ -7,7 +8,7 @@ mod proxmox_backup_debug; use proxmox_backup_debug::*; fn main() { - init_cli_logger("PBS_LOG", "info"); + init_cli_logger("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 385142df70ac..420e96665662 100644 --- a/src/bin/proxmox-backup-manager.rs +++ b/src/bin/proxmox-backup-manager.rs @@ -3,6 +3,7 @@ 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}; @@ -491,7 +492,7 @@ async fn get_versions(verbose: bool, param: Value) -> Result<Value, Error> { } async fn run() -> Result<(), Error> { - init_cli_logger("PBS_LOG", "info"); + init_cli_logger("PBS_LOG", proxmox_log::LevelFilter::INFO)?; proxmox_backup::server::notifications::init()?; let cmd_def = CliCommandMap::new() diff --git a/src/bin/proxmox-tape.rs b/src/bin/proxmox-tape.rs index 83793c346051..8e8584b35637 100644 --- a/src/bin/proxmox-tape.rs +++ b/src/bin/proxmox-tape.rs @@ -5,6 +5,7 @@ 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; @@ -997,7 +998,7 @@ async fn catalog_media(mut param: Value) -> Result<(), Error> { } fn main() { - init_cli_logger("PBS_LOG", "info"); + init_cli_logger("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 56399044d3d5..cd14b660a68a 100644 --- a/src/bin/sg-tape-cmd.rs +++ b/src/bin/sg-tape-cmd.rs @@ -10,6 +10,7 @@ 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; @@ -124,7 +125,7 @@ fn set_encryption( } fn main() -> Result<(), Error> { - init_cli_logger("PBS_LOG", "info"); + init_cli_logger("PBS_LOG", proxmox_log::LevelFilter::INFO)?; // check if we are user root or backup let backup_uid = pbs_config::backup_user()?.uid; -- 2.39.2 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] move client binaries to tracing 2024-08-29 13:40 ` [pbs-devel] [PATCH proxmox-backup v2 1/2] move client binaries to tracing Gabriel Goller @ 2024-08-29 13:41 ` Gabriel Goller 0 siblings, 0 replies; 9+ messages in thread From: Gabriel Goller @ 2024-08-29 13:41 UTC (permalink / raw) To: Proxmox Backup Server development discussion Oops, ignore the '1/2' in the commit header, it's wrong. _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pbs-devel] applied: [PATCH v2] add tracing init_cli_logger and deprecate old one 2024-08-29 13:40 [pbs-devel] [PATCH v2] add tracing init_cli_logger and deprecate old one Gabriel Goller 2024-08-29 13:40 ` [pbs-devel] [PATCH proxmox-backup v2 1/2] move client binaries to tracing Gabriel Goller @ 2024-08-30 11:58 ` Wolfgang Bumiller 2024-09-03 12:10 ` Gabriel Goller 1 sibling, 1 reply; 9+ messages in thread From: Wolfgang Bumiller @ 2024-08-30 11:58 UTC (permalink / raw) To: Gabriel Goller; +Cc: pbs-devel applied both patches, thanks _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] applied: [PATCH v2] add tracing init_cli_logger and deprecate old one 2024-08-30 11:58 ` [pbs-devel] applied: [PATCH v2] add tracing init_cli_logger and deprecate old one Wolfgang Bumiller @ 2024-09-03 12:10 ` Gabriel Goller 2024-09-03 12:21 ` Wolfgang Bumiller 0 siblings, 1 reply; 9+ messages in thread From: Gabriel Goller @ 2024-09-03 12:10 UTC (permalink / raw) To: Wolfgang Bumiller; +Cc: pbs-devel Christian Ebner and I just noticed that previously all the output was on stderr, because the `env_logger` prints everything to stderr per-default [0]. This means that all the console output from proxmox-backup-client, -manager, -debug, -file-restore, pxar, etc. is now on stdout. There is one test failing in pxar-bin, but I have a patch ready for that already. But will this cause more problems down the road? Do we rely somwhere on the stderr output? Should I change the tracing output to stderr as well? [0]: https://docs.rs/env_logger/latest/env_logger/ _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] applied: [PATCH v2] add tracing init_cli_logger and deprecate old one 2024-09-03 12:10 ` Gabriel Goller @ 2024-09-03 12:21 ` Wolfgang Bumiller 2024-09-03 12:27 ` Gabriel Goller 0 siblings, 1 reply; 9+ messages in thread From: Wolfgang Bumiller @ 2024-09-03 12:21 UTC (permalink / raw) To: Gabriel Goller; +Cc: pbs-devel On Tue, Sep 03, 2024 at 02:10:51PM GMT, Gabriel Goller wrote: > Christian Ebner and I just noticed that previously all the output was on > stderr, because the `env_logger` prints everything to stderr > per-default [0]. This means that all the console output from > proxmox-backup-client, -manager, -debug, -file-restore, pxar, etc. is > now on stdout. > > There is one test failing in pxar-bin, but I have a patch ready for that > already. > > But will this cause more problems down the road? Do we rely somwhere on the > stderr output? Should I change the tracing output to stderr as well? > > [0]: https://docs.rs/env_logger/latest/env_logger/ Yeah having logs go to stderr would make sense. CLI tool output that is meant to be "useful" for *tooling* should be printed with `println!()` after all, not "logged". _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] applied: [PATCH v2] add tracing init_cli_logger and deprecate old one 2024-09-03 12:21 ` Wolfgang Bumiller @ 2024-09-03 12:27 ` Gabriel Goller 2024-09-03 12:38 ` Wolfgang Bumiller 0 siblings, 1 reply; 9+ messages in thread From: Gabriel Goller @ 2024-09-03 12:27 UTC (permalink / raw) To: Wolfgang Bumiller; +Cc: pbs-devel On 03.09.2024 14:21, Wolfgang Bumiller wrote: >On Tue, Sep 03, 2024 at 02:10:51PM GMT, Gabriel Goller wrote: >> Christian Ebner and I just noticed that previously all the output was on >> stderr, because the `env_logger` prints everything to stderr >> per-default [0]. This means that all the console output from >> proxmox-backup-client, -manager, -debug, -file-restore, pxar, etc. is >> now on stdout. >> >> There is one test failing in pxar-bin, but I have a patch ready for that >> already. >> >> But will this cause more problems down the road? Do we rely somwhere on the >> stderr output? Should I change the tracing output to stderr as well? >> >> [0]: https://docs.rs/env_logger/latest/env_logger/ > >Yeah having logs go to stderr would make sense. >CLI tool output that is meant to be "useful" for *tooling* should be >printed with `println!()` after all, not "logged". Ok, will send a patch soon! Currently we nearly always log in our cli-tools, but maybe we should start using println? _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] applied: [PATCH v2] add tracing init_cli_logger and deprecate old one 2024-09-03 12:27 ` Gabriel Goller @ 2024-09-03 12:38 ` Wolfgang Bumiller 2024-09-03 14:40 ` Gabriel Goller 0 siblings, 1 reply; 9+ messages in thread From: Wolfgang Bumiller @ 2024-09-03 12:38 UTC (permalink / raw) To: Gabriel Goller; +Cc: pbs-devel On Tue, Sep 03, 2024 at 02:27:24PM GMT, Gabriel Goller wrote: > On 03.09.2024 14:21, Wolfgang Bumiller wrote: > > On Tue, Sep 03, 2024 at 02:10:51PM GMT, Gabriel Goller wrote: > > > Christian Ebner and I just noticed that previously all the output was on > > > stderr, because the `env_logger` prints everything to stderr > > > per-default [0]. This means that all the console output from > > > proxmox-backup-client, -manager, -debug, -file-restore, pxar, etc. is > > > now on stdout. > > > > > > There is one test failing in pxar-bin, but I have a patch ready for that > > > already. > > > > > > But will this cause more problems down the road? Do we rely somwhere on the > > > stderr output? Should I change the tracing output to stderr as well? > > > > > > [0]: https://docs.rs/env_logger/latest/env_logger/ > > > > Yeah having logs go to stderr would make sense. > > CLI tool output that is meant to be "useful" for *tooling* should be > > printed with `println!()` after all, not "logged". > > Ok, will send a patch soon! > Currently we nearly always log in our cli-tools, but maybe we should > start using println? I don't think we mostly log. Most of the time we call some API call and then use `format_and_print_result()` & friends. The point is not to *always* use `println!()` for everything, but for what is to be considered the actual "output" of the command. If there are cases where this is wrong, then yes, this should be corrected. I do wonder, though, whether CLI tools should move the default log level to "error". Most of the "info" level stuff shouldn't matter 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] 9+ messages in thread
* Re: [pbs-devel] applied: [PATCH v2] add tracing init_cli_logger and deprecate old one 2024-09-03 12:38 ` Wolfgang Bumiller @ 2024-09-03 14:40 ` Gabriel Goller 0 siblings, 0 replies; 9+ messages in thread From: Gabriel Goller @ 2024-09-03 14:40 UTC (permalink / raw) To: Wolfgang Bumiller; +Cc: pbs-devel On 03.09.2024 14:38, Wolfgang Bumiller wrote: >On Tue, Sep 03, 2024 at 02:27:24PM GMT, Gabriel Goller wrote: >> On 03.09.2024 14:21, Wolfgang Bumiller wrote: >> > On Tue, Sep 03, 2024 at 02:10:51PM GMT, Gabriel Goller wrote: >> > > Christian Ebner and I just noticed that previously all the output was on >> > > stderr, because the `env_logger` prints everything to stderr >> > > per-default [0]. This means that all the console output from >> > > proxmox-backup-client, -manager, -debug, -file-restore, pxar, etc. is >> > > now on stdout. >> > > >> > > There is one test failing in pxar-bin, but I have a patch ready for that >> > > already. >> > > >> > > But will this cause more problems down the road? Do we rely somwhere on the >> > > stderr output? Should I change the tracing output to stderr as well? >> > > >> > > [0]: https://docs.rs/env_logger/latest/env_logger/ >> > >> > Yeah having logs go to stderr would make sense. >> > CLI tool output that is meant to be "useful" for *tooling* should be >> > printed with `println!()` after all, not "logged". >> >> Ok, will send a patch soon! >> Currently we nearly always log in our cli-tools, but maybe we should >> start using println? > >I don't think we mostly log. Most of the time we call some API call >and then use `format_and_print_result()` & friends. Well, the proxmox-backup-client mostly logs... e.g. `create_backup` (proxmox-backup-client/src/main.rs:741), `restore` (proxmox-backup-client/src/main.rs:1475) and many more. >The point is not to *always* use `println!()` for everything, but for >what is to be considered the actual "output" of the command. Yes, I agree. >If there are cases where this is wrong, then yes, this should be >corrected. > >I do wonder, though, whether CLI tools should move the default log level >to "error". Most of the "info" level stuff shouldn't matter IMO. We could do that and transform all `info` logs to `println`'s (this probably makes sense nearly everywhere). _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-09-03 14:40 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-08-29 13:40 [pbs-devel] [PATCH v2] add tracing init_cli_logger and deprecate old one Gabriel Goller 2024-08-29 13:40 ` [pbs-devel] [PATCH proxmox-backup v2 1/2] move client binaries to tracing Gabriel Goller 2024-08-29 13:41 ` Gabriel Goller 2024-08-30 11:58 ` [pbs-devel] applied: [PATCH v2] add tracing init_cli_logger and deprecate old one Wolfgang Bumiller 2024-09-03 12:10 ` Gabriel Goller 2024-09-03 12:21 ` Wolfgang Bumiller 2024-09-03 12:27 ` Gabriel Goller 2024-09-03 12:38 ` Wolfgang Bumiller 2024-09-03 14:40 ` Gabriel Goller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox