Re: [pbs-devel] [RFC proxmox v2 2/2] proxmox-log: added tracing infra

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Fri, Nov 03, 2023 at 10:24:22AM +0100, Gabriel Goller wrote:
> On 11/3/23 09:56, Wolfgang Bumiller wrote:
> > > > > +}
> > > > > +
> > > > > +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?
> > I don't see the difference. In the threaded version you'd just
> > initialize in_worker_task to `true` - I mean, it does not matter where
> > you access that `Arc<AtomicBool>` from?
> > 
> > I don't know. I find this whole thing a bit backwards? Maybe it's just
> > me, but I'm not convinced that's how this part of `tracing` is meant to
> > be used.
> > 
> > Consider this:
> > If you already have an Arc<Bool>, all you need is a thread-local
> > copy of it:
> > - A Future based worker would have a wrapping Future containing that
> >    Arc, which, for the duration of `poll()`, sets the thread-local to
> >    point to its Arc.
> > - A thread based worker would just set it at the very beginning.
> > - A guard to enable/disable it (if we even need it, like on_enter/on_exit)
> >    would contain the old value and restore it on Drop (giving us a
> >    push/pop mechanics for cheap).
> > 
> > Also note that AFAICT neither the current tracing variant nor what I
> > wrote above deal with `tokio::spawn()`ed tasks, but that's fine.
> > If we want the ability to inherit the logging facility, this would need
> > to be independent from the boolean anyway since tasks can run on
> > separate threads - unless we restrict it to `LocalSet`s.
> > So I think we might need to first decide how that bit *should* work
> > before adding the ability to enter/leave the scope at will.
> > Unless I'm missing something.
> To be honest, I don't really get the advantage of a Future-Wrapper?
> Why not just have a:
> ```rust
> tokio::spawn(async move {

^ this async{} block up here should not be necessary, `.scope()` returns
a future.

>     LOGGER.scope(logger, async move {
>         // worker logic
>     })
> })
> ```
> and
> ```rust
> let _child = std::thread::Builder::new()
>     .name(upid.clone())
>     .spawn(move || {
>         LOGGER.sync_scope(logger, || {
>             // worker logic
>         })
>     });

It's essentially the same, so that's fine too.
I'm guessing you intend to skip the boolean part then and only go with
whether a logger is set?