all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Gabriel Goller <g.goller@proxmox.com>
To: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: pbs-devel@lists.proxmox.com
Subject: Re: [pbs-devel] [RFC proxmox v2 2/2] proxmox-log: added tracing infra
Date: Thu, 2 Nov 2023 15:58:02 +0100	[thread overview]
Message-ID: <f5833873-687a-4480-b8ad-fb9c151a301f@proxmox.com> (raw)
In-Reply-To: <uilsuhdampqxjkl6j4iqdwnsto2ntft2v2774h7xhulcgaq6lh@knus6hopuhyy>

Thanks for the review!

On 11/2/23 14:43, Wolfgang Bumiller wrote:
> [..]
>> +
>> +impl<S: Subscriber> Layer<S> for FilelogLayer {
>> +    fn on_event(&self, event: &Event<'_>, _ctx: Context<'_, S>) {
>> +        let mut buf = String::new();
>> +
>> +        event.record(&mut EventVisitor::new(&mut buf));
> I'd argue the above 2 lines should be part of the closure below,
> otherwise in the error case you just produce a string to throw away.
I Agree!
>> +
>> +        let logger_exists = LOGGER.try_with(|logger| {
>> +            log_to_file(&mut logger.borrow_mut(), event.metadata().level(), buf);
>> +        });
>> +        if let Err(e) = logger_exists {
>> +            error!(
> Is it wise to call log functions from within log handlers? ;-)
Hmm, yes this could be tricky...
How do we want to handle this anyway? We could panic here, but that 
seems a bit
harsh. Logging with `eprintln` is also an option, but a quite useless 
one tbh.
>> [..]
>> +pub struct WorkerTaskFilter {
>> +    in_worker_task: Arc<Mutex<bool>>,
> AFAICT you only have locks which are shortlived to set/clear/check this
> value.
> For such a thing you can use `Arc<AtomicBool>` and get rid of all the
> error handling.
Ooh, this makes it a lot easier, thanks for the heads up!I think we 
should be alright by just using `Ordering::Relaxed` everywhere, right?
>> +}
>> +
>> +impl WorkerTaskFilter {
>> +    pub fn new(in_worker_task: Arc<Mutex<bool>>) -> WorkerTaskFilter {
>> +        WorkerTaskFilter { in_worker_task }
>> +    }
>> +}
>> +
>> +impl<S: Subscriber + for<'a> LookupSpan<'a>> Filter<S> for WorkerTaskFilter {
>> +    fn on_enter(&self, id: &span::Id, ctx: Context<'_, S>) {
>> +        let metadata = ctx.metadata(id);
>> +        if let Some(m) = metadata {
>> +            if m.name() == "worker_task" {
> I'm not so happy with this.
> Now each time we poll a worker task we go through this layer system
> which uses string comparison to know whether we're currently in a worker
> task, for something that is actually rather static in the code.
> I'd much prefer a simply custom `Future` wrapping the worker task's
> future and setting this flag for the duration of the `poll()` method.
Ok, but the `Future` wrapper would only work for the task, right?
So we would need to keep the `span` version (or come up with
something different) for the thread use-case and then again have a
lot of `if (thread) { do this } else if (task) { do this }` stuff, which 
I don't
really like.
What we could do is have another `tokio::task_local!()` thingy, which 
contains a bool 'log_to_tasklog'. Then have another `scope` and 
`sync_scope` around the worker logic (So we would substitute the span 
stuff with another TLS).
> This just seems like a whole lot of overhead we don't need for such
> simple functionality. Also, the `on_enter` and `on_exit` methods make it
> look like you could easily enter and exit this type of span, but that's
> not the case. `on_exit` always stores `false`, so nested spans
> temporarily disabling and enabling the worker task log would just end up
> with a messed up state (this would need to be a counter...).
Right, I would need a counter here..
>> [..]
>> +
>> +impl Visit for EventVisitor<'_> {
>> +    fn record_debug(&mut self, field: &Field, value: &dyn fmt::Debug) {
>> +        if field.name() == "message" {
>> +            self.buf.push_str(&format!("{:?}", value));
> String implements fmt::Write. You can
>
>      use std::fmt::Write as _;
>      let _ = write!(state.buf, "{value:?}");
>
> it's possible for this to be more efficient since it does not enforce
> the creation of a separate allocated string.

Thanks!

> [..]




  reply	other threads:[~2023-11-02 14:58 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-25 13:53 [pbs-devel] [RFC proxmox-backup v2 0/2] Tasklog rewrite with tracing Gabriel Goller
2023-10-25 13:53 ` [pbs-devel] [RFC proxmox-backup v2 1/2] log: removed task_log! macro and moved to tracing Gabriel Goller
2023-10-27  8:31   ` Gabriel Goller
2023-10-25 13:53 ` [pbs-devel] [RFC proxmox v2 2/2] proxmox-log: added tracing infra Gabriel Goller
2023-11-02 13:43   ` Wolfgang Bumiller
2023-11-02 14:58     ` Gabriel Goller [this message]
2023-11-03  8:56       ` Wolfgang Bumiller
2023-11-03  9:24         ` Gabriel Goller
2023-11-03  9:52           ` Wolfgang Bumiller
2023-11-03 10:27             ` Gabriel Goller
2023-11-03 10:39               ` Wolfgang Bumiller
2023-11-03 10:49                 ` 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=f5833873-687a-4480-b8ad-fb9c151a301f@proxmox.com \
    --to=g.goller@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=w.bumiller@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 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