public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Dominik Csapak <d.csapak@proxmox.com>
To: Gabriel Goller <g.goller@proxmox.com>,
	Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>
Cc: "Thomas Lamprecht" <t.lamprecht@proxmox.com>,
	"Fabian Grünbichler" <f.gruenbichler@proxmox.com>
Subject: Re: [pbs-devel] [RFC proxmox-backup 0/2] Tasklog rewrite with tracing
Date: Mon, 23 Oct 2023 11:29:44 +0200	[thread overview]
Message-ID: <030d3735-02c2-4a6e-860f-e8b6521e7e0e@proxmox.com> (raw)
In-Reply-To: <0fd77836-896a-4017-9300-b74be824f346@proxmox.com>

On 10/23/23 11:09, Gabriel Goller wrote:
> On 10/18/23 15:12, Dominik Csapak wrote:
> 
>> hi, a high level comments on this series:
>>
>> you implemented a 'tasklog' = true filter, which is nice, but not filter for the syslog
>> in general we'd want to land things either in the task log (preferred) or in the syslog,
>> not in both (e.g. for some tasks this pollutes the syslog unnecessarily)
>>
> Ok, this should be easy, we just check in the `syslog_layer` if the `task_log` attribute
> is None in the Metadata. Like this we only log to task_log OR syslog.
>> so i'd like to see also a syslog = false (or syslog = true)  and maybe some
>> functionality to enable/disable that for whole block (if that's even possible?)
> As mentioned in the other patch, currently we can't inspect metadata values when enabling/
> disabling a layer :/ .
> What do you mean exactly by 'disabling in a whole block'?


a (theoretical) example:

fn some_function_in_a_worker() {
    ...
    log_to_tasklog("...");
    ...
    if (some_condition) {
       // low level operations+logs that don't belog into task log
       only_log_to_syslog("...");
       ...
       only_log_to_syslog("...");
       ...
    }
}

also having either syslog or tasklog is also not right everywhere, since sometimes
we explicitly want to have it in both the task and syslog (probably?)


> 
> We could enable/disable the filelog layer in specific rust modules (not so useful for us)
> or e.g, create a span on thread/task creation and then let all the logs in that span go to
> the task_log? The problem with that is that we would have to create an specific attribute
> to log to syslog while we are in a worker_task context (e.g. `info!(syslog = true, "to syslog")`)
> and it's not so clear anymore where we write (i.e. I'd have to look at the context to know
> if a `log::info!()` call writes to task_log or syslog).
> 
> IMO having a explicit `tasklog` attribute on every event is good (The name is obviously debatable,
> we could choose something shorter, or create a macro `task_info!` to make it easier to use).

having the 'tasklog' everytime make it harder to use imho, but if it's hidden behind some macro
can be ok.

in general what i'd like to have are three variants of logging:

* task log only
* syslog only
* task log and syslog

while the first and second will get the most use inside and outside a worker context respectively
so that should be most convenient to use (iow. short)

the second and third should also be possible in a worker context, but there it's not so important
that it's short

in my (naive) imagination i would have liked something like this:

info!("foo"); // logs to task log inside worker, syslog outside
info!(syslog = true, "foo"); // logs only to syslog, even in worker context
info!(syslog = true, tasklog = true, "foo"); // log in both task and syslog (if possible)

(the names are just placeholders, not suggestions. also i'm not against using
macros for either like you wrote: task_log!, syslog!, task_and_syslog! (probably not that))

does that make sense? (@Thomas, @Fabian?)





  reply	other threads:[~2023-10-23  9:29 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-11 14:01 Gabriel Goller
2023-10-11 14:01 ` [pbs-devel] [RFC proxmox-backup 1/2] log: removed task_log! macro and moved to tracing Gabriel Goller
2023-10-11 14:01 ` [pbs-devel] [RFC proxmox 2/2] proxmox-log: added tracing infra Gabriel Goller
2023-10-13 12:36   ` Gabriel Goller
2023-10-18 13:26   ` Dominik Csapak
2023-10-23  8:56     ` Gabriel Goller
2023-10-23  9:11       ` Gabriel Goller
2023-10-18 13:12 ` [pbs-devel] [RFC proxmox-backup 0/2] Tasklog rewrite with tracing Dominik Csapak
2023-10-23  9:09   ` Gabriel Goller
2023-10-23  9:29     ` Dominik Csapak [this message]
2023-10-23 11:43       ` Gabriel Goller
2023-10-23 14:33       ` Thomas Lamprecht
2023-10-24 13:44         ` Gabriel Goller
2023-10-25 13:55           ` Gabriel Goller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=030d3735-02c2-4a6e-860f-e8b6521e7e0e@proxmox.com \
    --to=d.csapak@proxmox.com \
    --cc=f.gruenbichler@proxmox.com \
    --cc=g.goller@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=t.lamprecht@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal