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

[Gabriel Goller <g.goller@proxmox.com>]

On 11/3/23 09:56, Wolfgang Bumiller wrote:

> [..]
> Well it's either eprinln!, or, well, do nothing and hope there's another
> layer dealing with it.
> But really, what *is* this error anyway? AFAICT it means the task-local
> is not set, so we should not even run into this, and panicking might
> almost be fine...
Yes, that's right, this should never happen. I'll insert a panic!
call here.
> Alternatively we could drop the separate layer idea and just have 1
> layer where the file logger not existing is the same as the task logger
> not being enabled, that is, we fall back to using the syslogger.
> I guess that depends on how much additional functionality we really need
> from all this tracing infrastructure?
Right, I thought about this as well. We could just keep it simple:
check if the FileLogger has been created in the TLS, if not, log to
syslog.
> [..]
>>>> +}
>>>> +
>>>> +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 {
     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
         })
     });
```This isn't exactly the same as setting the logger (or the bool) in 
the poll() function, but it shouldn't change a lot?
>> 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).
What do you think about something like this?
>> [..]