* [RFC proxmox-backup 0/2] backup log: reduce logging during backup
@ 2026-09-23 10:27 Thomas Ellmenreich
2026-09-23 10:27 ` [PATCH proxmox-backup 1/2] backup writer: add constructor for BackupWriterOptions Thomas Ellmenreich
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Thomas Ellmenreich @ 2026-09-23 10:27 UTC (permalink / raw)
To: pbs-devel; +Cc: Thomas Ellmenreich
There are a few different forum posts discussing the excessive default logging
of the proxmox-backup-client and its server counterpart: [0], [1]. A lot of the
heavy lifting has been done by Gabriel Goller as mentioned here [0] this just
addresses the final client side setting.
The final implementation just uses proxmox_log::enabled! to determine the log
settings and set the debug flag accordingly. By doing so, in all normal cases,
where INFO is the default verbosity level the following logs should NOT be
displayed anymore:
- 'download chunk "...' [0]
- 'GET /chunk' [0]
- 'successfully added chunk' [1]
Originally, I was planning on implementing this in a way more complex way, but
then ended up choosing this one for now. The current method is used in a few
other places around the codebase and this series would definitely work
perfectly as is.
That said, I have considered two other options:
Proper Implementation
---------------------
The current implementation sets the debug flag on the client, which then passes
it to the server. The server then has a mechanism, separate from tracing, to
decide if to issue 'debug' or 'info' logs.
My initial idea was to replace the existing debug flag on all API endpoints
with an EnvFilter [3] string, which would then be used to change the local
filtering behaviour to match the one on the client. [*]
Aside from the question if we really want the client to control server logging
(also in the current impl), this implementation would be quite complex and
would require a version of this series: [2] to be applied first.
[*]: This would be done by layering a scoped subscriber over the global one. The
new subscriber would only affect logs in the current function while the
guard has not been dropped.
Deleting the serverside
-----------------------
To be quite frank, I find that deleting this functionality altogether might be
the best option. A person that would look at the logs on serverside also has
the access to change the serverside loglevel. Additionally, the aforementioned
Series [3] would then allow very fine grained control of the log settings.
As a note, with the current setup, one can set the server side log level to
TRACE, but if the client doesn't enable the debug flag, none of these logs will
be printed. So by deleting the flag, the custom log filtering mechanism on
serverside can also be deleted.
[0]: https://forum.proxmox.com/threads/how-to-stop-syncjob-flooting-syslog.118894/#post-546081
[1]: https://forum.proxmox.com/threads/enhancement-suggestion-log-levels.130080/
[2]: https://lore.proxmox.com/pdm-devel/20260921095210.229315-2-t.ellmenreich@proxmox.com/T/#t
[3]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.EnvFilter.html
proxmox-backup:
Thomas Ellmenreich (2):
backup writer: add constructor for BackupWriterOptions
fix #4646: backup writer: base debug flag on log level
examples/upload-speed.rs | 15 +++++++--------
pbs-client/src/backup_writer.rs | 21 +++++++++++++++++++++
proxmox-backup-client/src/benchmark.rs | 15 +++++++--------
proxmox-backup-client/src/main.rs | 15 +++++++--------
src/server/push.rs | 17 ++++++++---------
5 files changed, 50 insertions(+), 33 deletions(-)
Summary over all repositories:
5 files changed, 50 insertions(+), 33 deletions(-)
--
Generated by murpp 0.12.0
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH proxmox-backup 1/2] backup writer: add constructor for BackupWriterOptions 2026-09-23 10:27 [RFC proxmox-backup 0/2] backup log: reduce logging during backup Thomas Ellmenreich @ 2026-09-23 10:27 ` Thomas Ellmenreich 2026-09-24 16:06 ` Christian Ebner 2026-09-23 10:27 ` [PATCH proxmox-backup 2/2] fix #4646: backup writer: base debug flag on log level Thomas Ellmenreich 2026-09-24 16:00 ` [RFC proxmox-backup 0/2] backup log: reduce logging during backup Christian Ebner 2 siblings, 1 reply; 5+ messages in thread From: Thomas Ellmenreich @ 2026-09-23 10:27 UTC (permalink / raw) To: pbs-devel; +Cc: Thomas Ellmenreich Instead of creating BackupWriterOptions directly, use a new function for it. The goal is to be able to remove values from the constructor and provide default values for them. Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com> --- examples/upload-speed.rs | 16 ++++++++-------- pbs-client/src/backup_writer.rs | 22 ++++++++++++++++++++++ proxmox-backup-client/src/benchmark.rs | 16 ++++++++-------- proxmox-backup-client/src/main.rs | 16 ++++++++-------- src/server/push.rs | 18 +++++++++--------- 5 files changed, 55 insertions(+), 33 deletions(-) diff --git a/examples/upload-speed.rs b/examples/upload-speed.rs index 5a63e0f09..a0b742854 100644 --- a/examples/upload-speed.rs +++ b/examples/upload-speed.rs @@ -19,15 +19,15 @@ async fn upload_speed() -> Result<f64, Error> { let client = BackupWriter::start( &client, - BackupWriterOptions { + BackupWriterOptions::new( datastore, - ns: &BackupNamespace::root(), - backup: &(BackupType::Host, "speedtest".to_string(), backup_time).into(), - crypt_config: None, - debug: false, - benchmark: true, - no_cache: false, - }, + &BackupNamespace::root(), + &(BackupType::Host, "speedtest".to_string(), backup_time).into(), + None, + false, + true, + false, + ), ) .await?; diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs index 7f7535746..7c3cb21b7 100644 --- a/pbs-client/src/backup_writer.rs +++ b/pbs-client/src/backup_writer.rs @@ -100,6 +100,28 @@ pub struct BackupWriterOptions<'a> { pub no_cache: bool, } +impl<'a> BackupWriterOptions<'a> { + pub fn new( + datastore: &'a str, + ns: &'a BackupNamespace, + backup: &'a BackupDir, + crypt_config: Option<Arc<CryptConfig>>, + debug: bool, + benchmark: bool, + no_cache: bool, + ) -> Self { + Self { + datastore, + ns, + backup, + crypt_config, + debug, + benchmark, + no_cache, + } + } +} + impl BackupWriter { fn new(h2: H2Client, abort: AbortHandle, crypt_config: Option<Arc<CryptConfig>>) -> Arc<Self> { Arc::new(Self { diff --git a/proxmox-backup-client/src/benchmark.rs b/proxmox-backup-client/src/benchmark.rs index a937bacb9..af9113ecb 100644 --- a/proxmox-backup-client/src/benchmark.rs +++ b/proxmox-backup-client/src/benchmark.rs @@ -238,15 +238,15 @@ async fn test_upload_speed( log::debug!("Connecting to backup server"); let client = BackupWriter::start( &client, - BackupWriterOptions { - datastore: repo.store(), - ns: &BackupNamespace::root(), - backup: &(BackupType::Host, "benchmark".to_string(), backup_time).into(), - crypt_config: crypt_config.clone(), - debug: false, - benchmark: true, + BackupWriterOptions::new( + repo.store(), + &BackupNamespace::root(), + &(BackupType::Host, "benchmark".to_string(), backup_time).into(), + crypt_config.clone(), + false, + true, no_cache, - }, + ), ) .await?; diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs index 5238a75ea..3aaaac39e 100644 --- a/proxmox-backup-client/src/main.rs +++ b/proxmox-backup-client/src/main.rs @@ -1046,15 +1046,15 @@ async fn create_backup( let client = BackupWriter::start( &http_client, - BackupWriterOptions { - datastore: repo.store(), - ns: &backup_ns, - backup: &snapshot, - crypt_config: crypt_config.clone(), - debug: true, - benchmark: false, + BackupWriterOptions::new( + repo.store(), + &backup_ns, + &snapshot, + crypt_config.clone(), + true, + false, no_cache, - }, + ), ) .await?; diff --git a/src/server/push.rs b/src/server/push.rs index 487fd91e3..665c77a8d 100644 --- a/src/server/push.rs +++ b/src/server/push.rs @@ -1132,17 +1132,17 @@ pub(crate) async fn push_snapshot( // Writer instance locks the snapshot on the remote side let backup_writer = BackupWriter::start( ¶ms.target.client, - BackupWriterOptions { - datastore: params.target.repo.store(), - ns: &target_ns, - backup: snapshot, - crypt_config: encrypt_using_key + BackupWriterOptions::new( + params.target.repo.store(), + &target_ns, + snapshot, + encrypt_using_key .as_ref() .map(|(_id, conf)| Arc::clone(conf)), - debug: false, - benchmark: false, - no_cache: false, - }, + false, + false, + false, + ), ) .await .with_context(|| prefix.to_string())?; -- 2.47.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH proxmox-backup 1/2] backup writer: add constructor for BackupWriterOptions 2026-09-23 10:27 ` [PATCH proxmox-backup 1/2] backup writer: add constructor for BackupWriterOptions Thomas Ellmenreich @ 2026-09-24 16:06 ` Christian Ebner 0 siblings, 0 replies; 5+ messages in thread From: Christian Ebner @ 2026-09-24 16:06 UTC (permalink / raw) To: Thomas Ellmenreich, pbs-devel On 9/23/26 12:28 PM, Thomas Ellmenreich wrote: > Instead of creating BackupWriterOptions directly, use a new function for it. > The goal is to be able to remove values from the constructor and provide > default values for them. Might make sense to use a builder like pattern [0] here instead and set sensible defaults? Would especially setting the flags be more verbose without needing to lookup the parameter definition and order of new(). [0] https://www.lurklurk.org/effective-rust/builders.html > Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com> > --- > examples/upload-speed.rs | 16 ++++++++-------- > pbs-client/src/backup_writer.rs | 22 ++++++++++++++++++++++ > proxmox-backup-client/src/benchmark.rs | 16 ++++++++-------- > proxmox-backup-client/src/main.rs | 16 ++++++++-------- > src/server/push.rs | 18 +++++++++--------- > 5 files changed, 55 insertions(+), 33 deletions(-) > > diff --git a/examples/upload-speed.rs b/examples/upload-speed.rs > index 5a63e0f09..a0b742854 100644 > --- a/examples/upload-speed.rs > +++ b/examples/upload-speed.rs > @@ -19,15 +19,15 @@ async fn upload_speed() -> Result<f64, Error> { > > let client = BackupWriter::start( > &client, > - BackupWriterOptions { > + BackupWriterOptions::new( > datastore, > - ns: &BackupNamespace::root(), > - backup: &(BackupType::Host, "speedtest".to_string(), backup_time).into(), > - crypt_config: None, > - debug: false, > - benchmark: true, > - no_cache: false, > - }, > + &BackupNamespace::root(), > + &(BackupType::Host, "speedtest".to_string(), backup_time).into(), > + None, > + false, > + true, > + false, > + ), > ) > .await?; > > diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs > index 7f7535746..7c3cb21b7 100644 > --- a/pbs-client/src/backup_writer.rs > +++ b/pbs-client/src/backup_writer.rs > @@ -100,6 +100,28 @@ pub struct BackupWriterOptions<'a> { > pub no_cache: bool, > } > > +impl<'a> BackupWriterOptions<'a> { > + pub fn new( > + datastore: &'a str, > + ns: &'a BackupNamespace, > + backup: &'a BackupDir, > + crypt_config: Option<Arc<CryptConfig>>, > + debug: bool, > + benchmark: bool, > + no_cache: bool, > + ) -> Self { > + Self { > + datastore, > + ns, > + backup, > + crypt_config, > + debug, > + benchmark, > + no_cache, > + } > + } > +} > + > impl BackupWriter { > fn new(h2: H2Client, abort: AbortHandle, crypt_config: Option<Arc<CryptConfig>>) -> Arc<Self> { > Arc::new(Self { > diff --git a/proxmox-backup-client/src/benchmark.rs b/proxmox-backup-client/src/benchmark.rs > index a937bacb9..af9113ecb 100644 > --- a/proxmox-backup-client/src/benchmark.rs > +++ b/proxmox-backup-client/src/benchmark.rs > @@ -238,15 +238,15 @@ async fn test_upload_speed( > log::debug!("Connecting to backup server"); > let client = BackupWriter::start( > &client, > - BackupWriterOptions { > - datastore: repo.store(), > - ns: &BackupNamespace::root(), > - backup: &(BackupType::Host, "benchmark".to_string(), backup_time).into(), > - crypt_config: crypt_config.clone(), > - debug: false, > - benchmark: true, > + BackupWriterOptions::new( > + repo.store(), > + &BackupNamespace::root(), > + &(BackupType::Host, "benchmark".to_string(), backup_time).into(), > + crypt_config.clone(), > + false, > + true, > no_cache, > - }, > + ), > ) > .await?; > > diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs > index 5238a75ea..3aaaac39e 100644 > --- a/proxmox-backup-client/src/main.rs > +++ b/proxmox-backup-client/src/main.rs > @@ -1046,15 +1046,15 @@ async fn create_backup( > > let client = BackupWriter::start( > &http_client, > - BackupWriterOptions { > - datastore: repo.store(), > - ns: &backup_ns, > - backup: &snapshot, > - crypt_config: crypt_config.clone(), > - debug: true, > - benchmark: false, > + BackupWriterOptions::new( > + repo.store(), > + &backup_ns, > + &snapshot, > + crypt_config.clone(), > + true, > + false, > no_cache, > - }, > + ), > ) > .await?; > > diff --git a/src/server/push.rs b/src/server/push.rs > index 487fd91e3..665c77a8d 100644 > --- a/src/server/push.rs > +++ b/src/server/push.rs > @@ -1132,17 +1132,17 @@ pub(crate) async fn push_snapshot( > // Writer instance locks the snapshot on the remote side > let backup_writer = BackupWriter::start( > ¶ms.target.client, > - BackupWriterOptions { > - datastore: params.target.repo.store(), > - ns: &target_ns, > - backup: snapshot, > - crypt_config: encrypt_using_key > + BackupWriterOptions::new( > + params.target.repo.store(), > + &target_ns, > + snapshot, > + encrypt_using_key > .as_ref() > .map(|(_id, conf)| Arc::clone(conf)), > - debug: false, > - benchmark: false, > - no_cache: false, > - }, > + false, > + false, > + false, > + ), > ) > .await > .with_context(|| prefix.to_string())?; ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH proxmox-backup 2/2] fix #4646: backup writer: base debug flag on log level 2026-09-23 10:27 [RFC proxmox-backup 0/2] backup log: reduce logging during backup Thomas Ellmenreich 2026-09-23 10:27 ` [PATCH proxmox-backup 1/2] backup writer: add constructor for BackupWriterOptions Thomas Ellmenreich @ 2026-09-23 10:27 ` Thomas Ellmenreich 2026-09-24 16:00 ` [RFC proxmox-backup 0/2] backup log: reduce logging during backup Christian Ebner 2 siblings, 0 replies; 5+ messages in thread From: Thomas Ellmenreich @ 2026-09-23 10:27 UTC (permalink / raw) To: pbs-devel; +Cc: Thomas Ellmenreich Before, the debug flag had to be set manually for each instance of BackupWriterOptions. However, with the 'new' constructor, the parameter has been removed and the flag is now derived from the verbosity of the current log level. Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=4646 Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com> --- examples/upload-speed.rs | 1 - pbs-client/src/backup_writer.rs | 3 +-- proxmox-backup-client/src/benchmark.rs | 1 - proxmox-backup-client/src/main.rs | 1 - src/server/push.rs | 1 - 5 files changed, 1 insertion(+), 6 deletions(-) diff --git a/examples/upload-speed.rs b/examples/upload-speed.rs index a0b742854..851eee1e6 100644 --- a/examples/upload-speed.rs +++ b/examples/upload-speed.rs @@ -24,7 +24,6 @@ async fn upload_speed() -> Result<f64, Error> { &BackupNamespace::root(), &(BackupType::Host, "speedtest".to_string(), backup_time).into(), None, - false, true, false, ), diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs index 7c3cb21b7..7ae12295d 100644 --- a/pbs-client/src/backup_writer.rs +++ b/pbs-client/src/backup_writer.rs @@ -106,7 +106,6 @@ impl<'a> BackupWriterOptions<'a> { ns: &'a BackupNamespace, backup: &'a BackupDir, crypt_config: Option<Arc<CryptConfig>>, - debug: bool, benchmark: bool, no_cache: bool, ) -> Self { @@ -115,7 +114,7 @@ impl<'a> BackupWriterOptions<'a> { ns, backup, crypt_config, - debug, + debug: proxmox_log::enabled!(Level::DEBUG), benchmark, no_cache, } diff --git a/proxmox-backup-client/src/benchmark.rs b/proxmox-backup-client/src/benchmark.rs index af9113ecb..924f825e8 100644 --- a/proxmox-backup-client/src/benchmark.rs +++ b/proxmox-backup-client/src/benchmark.rs @@ -243,7 +243,6 @@ async fn test_upload_speed( &BackupNamespace::root(), &(BackupType::Host, "benchmark".to_string(), backup_time).into(), crypt_config.clone(), - false, true, no_cache, ), diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs index 3aaaac39e..0067d87eb 100644 --- a/proxmox-backup-client/src/main.rs +++ b/proxmox-backup-client/src/main.rs @@ -1051,7 +1051,6 @@ async fn create_backup( &backup_ns, &snapshot, crypt_config.clone(), - true, false, no_cache, ), diff --git a/src/server/push.rs b/src/server/push.rs index 665c77a8d..5067d8033 100644 --- a/src/server/push.rs +++ b/src/server/push.rs @@ -1141,7 +1141,6 @@ pub(crate) async fn push_snapshot( .map(|(_id, conf)| Arc::clone(conf)), false, false, - false, ), ) .await -- 2.47.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC proxmox-backup 0/2] backup log: reduce logging during backup 2026-09-23 10:27 [RFC proxmox-backup 0/2] backup log: reduce logging during backup Thomas Ellmenreich 2026-09-23 10:27 ` [PATCH proxmox-backup 1/2] backup writer: add constructor for BackupWriterOptions Thomas Ellmenreich 2026-09-23 10:27 ` [PATCH proxmox-backup 2/2] fix #4646: backup writer: base debug flag on log level Thomas Ellmenreich @ 2026-09-24 16:00 ` Christian Ebner 2 siblings, 0 replies; 5+ messages in thread From: Christian Ebner @ 2026-09-24 16:00 UTC (permalink / raw) To: Thomas Ellmenreich, pbs-devel Thanks for tackling this log standing issue! On 9/23/26 12:28 PM, Thomas Ellmenreich wrote: > There are a few different forum posts discussing the excessive default logging > of the proxmox-backup-client and its server counterpart: [0], [1]. A lot of the > heavy lifting has been done by Gabriel Goller as mentioned here [0] this just > addresses the final client side setting. > > The final implementation just uses proxmox_log::enabled! to determine the log > settings and set the debug flag accordingly. By doing so, in all normal cases, > where INFO is the default verbosity level the following logs should NOT be > displayed anymore: > > - 'download chunk "...' [0] > - 'GET /chunk' [0] > - 'successfully added chunk' [1] > > Originally, I was planning on implementing this in a way more complex way, but > then ended up choosing this one for now. The current method is used in a few > other places around the codebase and this series would definitely work > perfectly as is. The current implementation does however only cover the logging for backup writers? Backup readers and other tasks would still potentially produce large logs. So this does not fully fix the issue as raised in the bug-tracker. E.g. for pull sync jobs logging is enabled on the remote source side via the final boolean flag [0]. And other readers might set this as well. The main issue is that server-side and client-side logs are intertwined as described here [1] and [2]. Given that, I think it could make sense to use a lower effort approach as layered out by your patches for the time being, deprecate the client side flag with the next major release and drop it fully on the subsequent major release. At that point a properly decoupled logging with tracing subscribers can be put in place. What's other developers opinion on this? [0] https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=src/server/sync.rs;h=11f30d318b2d64c7dd988cbd0ccfd8fc6998b36a;hb=HEAD#l500 [1] https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=src/api2/backup/environment.rs;h=be70d74f828e33b0020d4b7b5825cf98af77901d;hb=HEAD#l907 [2] https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=src/api2/reader/environment.rs;h=e98a001b83ab80122e2f6f939237d5416c0fcd17;hb=HEAD#l59 > That said, I have considered two other options: > > Proper Implementation > --------------------- > > The current implementation sets the debug flag on the client, which then passes > it to the server. The server then has a mechanism, separate from tracing, to > decide if to issue 'debug' or 'info' logs. > > My initial idea was to replace the existing debug flag on all API endpoints > with an EnvFilter [3] string, which would then be used to change the local > filtering behaviour to match the one on the client. [*] > > Aside from the question if we really want the client to control server logging > (also in the current impl), this implementation would be quite complex and > would require a version of this series: [2] to be applied first. > > [*]: This would be done by layering a scoped subscriber over the global one. The > new subscriber would only affect logs in the current function while the > guard has not been dropped. > > Deleting the serverside > ----------------------- > > To be quite frank, I find that deleting this functionality altogether might be > the best option. A person that would look at the logs on serverside also has > the access to change the serverside loglevel. Additionally, the aforementioned > Series [3] would then allow very fine grained control of the log settings. > > As a note, with the current setup, one can set the server side log level to > TRACE, but if the client doesn't enable the debug flag, none of these logs will > be printed. So by deleting the flag, the custom log filtering mechanism on > serverside can also be deleted. > > [0]: https://forum.proxmox.com/threads/how-to-stop-syncjob-flooting-syslog.118894/#post-546081 > [1]: https://forum.proxmox.com/threads/enhancement-suggestion-log-levels.130080/ > [2]: https://lore.proxmox.com/pdm-devel/20260921095210.229315-2-t.ellmenreich@proxmox.com/T/#t > [3]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.EnvFilter.html > > > proxmox-backup: > > Thomas Ellmenreich (2): > backup writer: add constructor for BackupWriterOptions > fix #4646: backup writer: base debug flag on log level > > examples/upload-speed.rs | 15 +++++++-------- > pbs-client/src/backup_writer.rs | 21 +++++++++++++++++++++ > proxmox-backup-client/src/benchmark.rs | 15 +++++++-------- > proxmox-backup-client/src/main.rs | 15 +++++++-------- > src/server/push.rs | 17 ++++++++--------- > 5 files changed, 50 insertions(+), 33 deletions(-) > > > Summary over all repositories: > 5 files changed, 50 insertions(+), 33 deletions(-) > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-09-24 16:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-23 10:27 [RFC proxmox-backup 0/2] backup log: reduce logging during backup Thomas Ellmenreich 2026-09-23 10:27 ` [PATCH proxmox-backup 1/2] backup writer: add constructor for BackupWriterOptions Thomas Ellmenreich 2026-09-24 16:06 ` Christian Ebner 2026-09-23 10:27 ` [PATCH proxmox-backup 2/2] fix #4646: backup writer: base debug flag on log level Thomas Ellmenreich 2026-09-24 16:00 ` [RFC proxmox-backup 0/2] backup log: reduce logging during backup Christian Ebner
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.