tracing icon indicating copy to clipboard operation
tracing copied to clipboard

set_global needs guard alternative

Open HK416-is-all-you-need opened this issue 2 years ago • 7 comments

Feature Request

As of now only set_global_default is only reliable way to set logger available to all threads within application. While set_default/with_default actually sets logger onto thread local hence making it unsuitable for needs of multi-threaded application. Documentation on these functions fails to mention that.

Ideally we should have be able to created scoped and global logger. The reason being is that sometimes you want to be able to gracefully drop your logger, especially if you have some caching mechanism to prevent network IO from being used too often. In my case I have fluentd logger which only sends log records after reaching certain limit, hence I need logger to be scoped so that it would flush it on application termination.

Obviously I can do it with hacks myself, but I believe proper logger would benefit from having scoped guard for global one. Considering complexity of tracing over log I do not see a reason to copy this flaw

Alternatives

Install default logger with each new thread

HK416-is-all-you-need avatar Aug 19 '21 05:08 HK416-is-all-you-need

In both log and tracing, global defaults are only set once for performance reasons. Because the global default is only set a single time, synchronizing access to the global default is much simpler. If the global default was able to be set multiple times, more complex synchronization (e.g. a read-write lock) would probably be required, introducing additional overhead on every access to the global default. It would be preferable to avoid this since a majority of use cases don't need to set the global default multiple times. We could certainly consider adding a way to set the global subscriber in a scope, but we'd need to do so in a way that doesn't introduce additional overhead when a scoped global default is not in use.

In your particular use case, is it possible to use an approach where every thread calls dispatcher::set_default with clones of the same Dispatch? Since Dispatch is reference counted, the subscriber will be dropped when all threads have terminated.

An alternative solution is to construct the subscriber alongside a separate drop guard that triggers a flush (perhaps using a channel or by setting a flag and unparking a spawned thread?). This way, the subscriber can be set as the global default, and the flush guard held in the program's main function. This is the approach used by tracing-appender and tracing-flame.

Documentation on these functions fails to mention that.

You're right that the docs for set_default and with_default don't actually state that the default is set only for the current thread. It's implied here but the function documentation should really mention that explicitly as well.

hawkw avatar Aug 19 '21 15:08 hawkw

I believe it should be possible to achieve without a need for locks. Consider this current logic to set global dispatcher uses atomic to track initialziation state and has following states: UNINIT, INITIALITING, INITIALIZED. We can add UNINITIALIZING and rely on it to make sure to safely drop global dispatcher.

In your particular use case, is it possible to use an approach where every thread calls dispatcher::set_default with clones of the same Dispatch?

I could actually make it just reference and impl trait for it, then drop it once program exits (a bit unsafe but it is fine) or just store guard for each into thread local

HK416-is-all-you-need avatar Aug 20 '21 01:08 HK416-is-all-you-need

On side note better API to global logger would be following:

fn set_global_logger<C: Collect>(logger: &Collect) -> Guard {
    let ptr = logger as *const C;
    //panic if guard is acquired?
    GLOBAL_COLLECTOR.swap(ptr, Ordering::SeqCst);

   Guard {
   }
}

fn get_global() -> &dyn Collect {
    //need to check if drop is ongoing so still nee state?
    let ptr = GLOBAL_COLLECTOR.get(Ordering::SeqCst);
    unsafe {
        match ptr.is_null() {
            true => &NONE_COLLECTOR,
            false => &*ptr,
        }
    }
}

impl Drop for Guard {
    fn drop(&mut self) {
        //need to wait until all references to Collect are released though
        GLOBAL_COLLECTOR.store(core::ptr::null(), Ordering::SeqCst);
    }
}

There are two main benefits:

  1. User retains full control over lifetime of logger;
  2. tracing-core doesn't need alloc cfg for global logger

HK416-is-all-you-need avatar Aug 20 '21 04:08 HK416-is-all-you-need

@HK416-is-all-you-need -- I don't quite understand the use case you mentioned, could you explain it a bit more?

If I am understanding correctly, when you hit some threshold, you want to drop the global logger so that no more logging happens, and then at some point re-add a global logger again?

bryangarza avatar May 07 '22 03:05 bryangarza

My use case is quite simple. I need graceful shutdown for logger in order to clean logger cache (I have fluentd server where we store logs, but I do not send each log entry alone, I accumulate particular number and then send once it reaches threshold, hence graceful shutdown is needed to ensure that yet to be sent log entries are being sent)

P.s. I achieved with work around by creating guard for my tracing-fluentd layer which on drop would terminate thread worker and flush all log entries. Since I doubt anyone will make proper API to provide unset for global logger

HK416-is-all-you-need avatar May 07 '22 06:05 HK416-is-all-you-need

Thanks for explaining

Just brainstorming -- if there was a flush() API defined on the tracing-fluentd Layer impl directly, would that work better for you versus having to drop the layer?

For example, if the Layer impl has flush() defined on it, then you could call it from the on_close or on_event, assuming that you are maintaining some sort of state to track how close you are to reaching the threshold.

(Though yes, not having a way to set a scoped global logger might be a gap regardless)

bryangarza avatar May 07 '22 07:05 bryangarza

For example, if the Layer impl has flush() defined on it, then you could call it from the on_close or on_event, assuming that you are maintaining some sort of state to track how close you are to reaching the threshold.

I'm not sure what exacly you're thinking here: on_event is where you make log hence I perform fluent logging (or cache it until it reaches threshold). At this point you can already check whether you need to flush or not

on_close can happen numerous times depending on your span structure, it makes a little sense to check it every time span closes (not to mention you're going to do it in parallel in case of multi-threaded runtime)

Drop and flush() is no different. Both are just function calls, I use Drop only because it is easier to reason about it. But it can be made into free function call quite easily, the point is there is no point in doing when you have Drop friendly guard for that purpose.

In any case as long as global logger doesn't provide scoping, you have to work it around somehow

HK416-is-all-you-need avatar May 07 '22 08:05 HK416-is-all-you-need

Hi @bryangarza,

I ran into a similar use case here.

Currently, is there any recommended way to drop the global subscriber, and then tracing::subscriber::stracing::subscriber::set_global_default again?

If there is some feature need to implement in tracing, I am happy to contribute. Please guide me here. Thanks in advanced. :pray:

yanganto avatar Dec 07 '22 02:12 yanganto

Out of curiosity why is the global state a mutable static? Why not use an atomic pointer? Or better if you want to allow the ability to change the global state, why not use a dual atomic pointer?

Like the only reason i can think of is because no std wouldnt support this. However why not just add support for it since most users will be using std and then if they try mutate the global state after init we just return an error since that isnt supported without std.

TroyKomodo avatar Apr 15 '23 03:04 TroyKomodo