all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal