From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id B36501FF2C6 for ; Wed, 10 Jul 2024 12:58:46 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5B2A57756; Wed, 10 Jul 2024 12:59:09 +0200 (CEST) Mime-Version: 1.0 Date: Wed, 10 Jul 2024 12:59:03 +0200 Message-Id: To: "Proxmox Backup Server development discussion" From: "Max Carrara" X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240709142018.386394-1-g.goller@proxmox.com> In-Reply-To: <20240709142018.386394-1-g.goller@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.028 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [pull.rs, zfs.rs, proxmox-backup-api.rs, mod.rs, directory.rs, apt.rs, acme.rs, worker-task-abort.rs, prune.rs, verify.rs, datastore.rs, proxmox-backup-proxy.rs, drive.rs, restore.rs, certificates.rs, backup.rs, rest.rs, lib.rs] Subject: Re: [pbs-devel] [PATCH proxmox{, -backup} v8 0/4] proxmox-log introduction X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On Tue Jul 9, 2024 at 4:20 PM CEST, Gabriel Goller wrote: > Removed the task_log! (and friends task_warn!, task_debug!, etc.) macro > and introduced the `tracing` crate. We now initiate the tracing crate > using two layer, which are logging to the syslog and the tasklog. > It uses the `tracing-journald` crate and the original `FileLogger`. > > To write to the task logs from the worker threads and tasks, we now > have a task_local logger (and warning counter), which > will get instantiated when a task/thread is created. This means that > when we call `info!` or any other `tracing` log macros, it will get > the file_logger from TLS and write to the file. Building & Testing ------------------ * patches applied fine on the latest master branches of both proxmox and proxmox-backup * rebuilt `proxmox-backup` and installed the new binaries on my PBS test VM * for a quick smoke test, ran a couple of tasks: - GC - sync (local) - backup VM from PVE Nothing out of the ordinary here; the logs work and look just like before, except that under the hood `proxmox-log` was being used. Pretty neat. Code Review ----------- Consider this "proofreading" again, as most things appear to have been addressed in previous versions of this series. Patches 01 and 02 are rather straightforward, the code is easy to follow, even though the whole `task_log!` thing from tokio is something I'm personally not too familiar with. After reading a little into it, it makes total sense, especially after I saw that every worker gets their own `LOGGER` and `WARN_COUNTER` (as you had also pointed out off-list). So, all is fine here. Patches 03 and 04 are quite sizeable, but nevertheless I think that's totally fine in this context, as you're essentially replacing all `task_log!` and related invocations. It's very nice to see a lot of functions and coroutines being slimmed down that way; logging seems much more convenient now. I haven't caught any replacement that looks off; even though there are a lot of changes in patch 03 & 04 in total, they're fairly simple. Also, your code is formatted with `cargo fmt` and `cargo clippy` doesn't complain either; very nice. Conclusion ---------- The series passed a decent smoke test and I haven't found anything to complain about in your code. LGTM. Tested-by: Max Carrara Reviewed-by: Max Carrara > > v8, thanks @Wolfgang: > - remove unnecessary filter on TasklogLayer > we don't need to check the TLS in the filter **and** in `on_event`, one > time is enough > - remove `flog` macro from `FileLogger` > - move string building in TasklogLayer `on_event` into TLS closure > - rebase > > v7, thanks @Lukas: > - run cargofmt > > v6, thanks @Lukas: > - rebase > - reorder imports, inline variables > - introduce `tracing-journald` and throw out `syslog` > - split single layer into two, one for syslog and for tasklog > > v5: > - minor rebase > > v4: > - rebase > - reword commit messages (thanks @Thomas) > - split into multiple patches > > v3, thanks @Sterzy: > - updated debian/control files > - downgraded tracing-log > > v2: > - Rebase onto master > - Split proxmox-backup commit into two > > v1, thanks @Wolfgang, @Lukas: > - Combined syslog and tasklog to single layer > - Infer the logging target from the FileLogger TLS > > RFC v2, thanks @Dominik, @Thomas: > - Remove the 'tasklog = true' attribute and infer the context > - Wrap the worker_thread or worker_task in a span with name > 'worker_task' > - All events in the span with name 'worker_task' get logged to the > file_logger, everything else goes to syslog (Error events go to > both) > - Remove the `Option<>` around the `FileLogger` in TLS > - Clippy fixes > > proxmox: > > Gabriel Goller (2): > proxmox-log: add tracing infrastructure > enable tracing logger, remove task_log macros > > Cargo.toml | 6 ++ > proxmox-log/Cargo.toml | 23 +++++ > proxmox-log/debian/changelog | 5 + > proxmox-log/debian/control | 52 +++++++++++ > proxmox-log/debian/copyright | 18 ++++ > proxmox-log/debian/debcargo.toml | 7 ++ > .../src/file_logger.rs | 34 +++---- > proxmox-log/src/lib.rs | 48 ++++++++++ > proxmox-log/src/tasklog_layer.rs | 58 ++++++++++++ > proxmox-rest-server/Cargo.toml | 2 + > proxmox-rest-server/src/api_config.rs | 3 +- > proxmox-rest-server/src/lib.rs | 3 - > proxmox-rest-server/src/rest.rs | 4 +- > proxmox-rest-server/src/worker_task.rs | 93 ++++++++----------- > proxmox-sys/src/worker_task_context.rs | 47 ---------- > 15 files changed, 278 insertions(+), 125 deletions(-) > create mode 100644 proxmox-log/Cargo.toml > create mode 100644 proxmox-log/debian/changelog > create mode 100644 proxmox-log/debian/control > create mode 100644 proxmox-log/debian/copyright > create mode 100644 proxmox-log/debian/debcargo.toml > rename {proxmox-rest-server => proxmox-log}/src/file_logger.rs (83%) > create mode 100644 proxmox-log/src/lib.rs > create mode 100644 proxmox-log/src/tasklog_layer.rs > > > proxmox-backup: > > Gabriel Goller (2): > switch from task_log! macro to tracing > api: switch from task_log! macro to tracing > > Cargo.toml | 7 + > debian/control | 2 + > pbs-datastore/Cargo.toml | 1 + > pbs-datastore/src/chunk_store.rs | 30 +--- > pbs-datastore/src/datastore.rs | 82 ++++------ > src/api2/admin/datastore.rs | 26 ++-- > src/api2/config/acme.rs | 21 +-- > src/api2/config/datastore.rs | 16 +- > src/api2/config/prune.rs | 14 +- > src/api2/node/apt.rs | 1 + > src/api2/node/certificates.rs | 67 ++++---- > src/api2/node/disks/directory.rs | 13 +- > src/api2/node/disks/mod.rs | 12 +- > src/api2/node/disks/zfs.rs | 31 ++-- > src/api2/node/mod.rs | 11 +- > src/api2/pull.rs | 37 ++--- > src/api2/tape/backup.rs | 75 ++++----- > src/api2/tape/drive.rs | 152 +++++++----------- > src/api2/tape/restore.rs | 259 ++++++++++--------------------- > src/backup/verify.rs | 105 ++++--------- > src/bin/proxmox-backup-api.rs | 13 +- > src/bin/proxmox-backup-proxy.rs | 42 ++--- > src/server/gc_job.rs | 6 +- > src/server/prune_job.rs | 29 ++-- > src/server/pull.rs | 243 +++++++++-------------------- > src/server/realm_sync_job.rs | 44 ++---- > src/server/verify_job.rs | 10 +- > src/tape/drive/mod.rs | 36 ++--- > src/tape/pool_writer/mod.rs | 92 ++++------- > src/tools/disks/mod.rs | 21 +-- > tests/worker-task-abort.rs | 9 +- > 31 files changed, 514 insertions(+), 993 deletions(-) > > > Summary over all repositories: > 46 files changed, 792 insertions(+), 1118 deletions(-) _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel