Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: add `dispatcher::set_default` #383

Closed
hawkw opened this issue Oct 15, 2019 · 5 comments

Comments

@hawkw
Copy link
Member

@hawkw hawkw commented Oct 15, 2019

Feature Request

Crates

  • tracing-core

Motivation

Currently, the only ways to set the default subscriber are set_global_default and with_default. The with_default function takes a closure, and sets the default subscriber for the duration of that closure, while set_global_default sets a default for the entire lifetime of the program, and fails if called more than once.

The current API surface makes some use-cases difficult. For example, if I wanted to use a fresh subscriber in each of my tests, I would need to use with_default, requiring indenting the entire test into a closure. When adding tracing to an existing codebase, this can result in unnecessarily large diffs.

Proposal

@Ralith suggested that we add a method for setting the default subscriber, returning an RAII drop guard that unsets the subscriber when dropped. This could be used to set the subscriber only for the duration of a scope, without having to indent the entire scope as with with_default.

I'll add that the current implementation of with_default uses a drop guard anyway:

/// A guard that resets the current default dispatcher to the prior
/// default dispatcher when dropped.
#[cfg(feature = "std")]
struct ResetGuard(Option<Dispatch>);
/// Sets this dispatch as the default for the duration of a closure.
///
/// The default dispatcher is used when creating a new [span] or
/// [`Event`].
///
/// **Note**: This function requires the Rust standard library. `no_std` users
/// should use [`set_global_default`] instead.
///
/// [span]: ../span/index.html
/// [`Subscriber`]: ../subscriber/trait.Subscriber.html
/// [`Event`]: ../event/struct.Event.html
/// [`set_global_default`]: ../fn.set_global_default.html
#[cfg(feature = "std")]
pub fn with_default<T>(dispatcher: &Dispatch, f: impl FnOnce() -> T) -> T {
// When this guard is dropped, the default dispatcher will be reset to the
// prior default. Using this (rather than simply resetting after calling
// `f`) ensures that we always reset to the prior dispatcher even if `f`
// panics.
let _guard = State::set_default(dispatcher.clone());
f()
}

This is in order to unset the default subscriber on panics. We could simply make this existing code public API.

Alternatives

Alternatively, we could not add a drop guard for setting the default subscriber. The existing API was chosen for consistency with existing tokio APIs, and adding a new way to set the default subscriber could introduce confusion.

@hawkw

This comment has been minimized.

Copy link
Member Author

@hawkw hawkw commented Oct 15, 2019

@carllerche any thoughts on whether we should do this?

@Ralith

This comment has been minimized.

Copy link
Collaborator

@Ralith Ralith commented Oct 15, 2019

For example, if I wanted to use a fresh subscriber in each of my tests

To be clear, there doesn't seem to be a simple way to set a single global subscriber for tests, so this use case isn't niche. You could start every test with a call that locks a global mutex and installs a global subscriber if one does not yet exist, but that's both uglier and requires more code than the proposed solution.

@hawkw

This comment has been minimized.

Copy link
Member Author

@hawkw hawkw commented Oct 15, 2019

You could start every test with a call that locks a global mutex and installs a global subscriber if one does not yet exist, but that's both uglier and requires more code than the proposed solution.

@Ralith AFAICT this is the state of the art for logging in tests in the log world, so I hadn't really considered it that bad — I've seen a lot of code where every test begins with a call to my_custom_init_env_logger() or whatever. But, I agree that this could be more ergonomic!

And, since tracing (unlike log) has a notion of scoped subscribers, we can do really nice stuff like separating the output from each test, as well.

@Ralith

This comment has been minimized.

Copy link
Collaborator

@Ralith Ralith commented Oct 15, 2019

this is the state of the art for logging in tests in the log world

Ah, I'm coming from slog with explicit Logger passing where there's no special effort needed.

@hawkw

This comment has been minimized.

Copy link
Member Author

@hawkw hawkw commented Oct 15, 2019

FWIW, the tokio_reactor::with_default and tokio_timer::with_default functions have been replaced with set_default functions that return a drop guard in the 0.2 alpha branch of tokio. Therefore, the "consistency with tokio" argument against adding a drop-guard-based API is pretty much moot, and I think we should probably go ahead and add set_default to tracing as well.

Here's what I think we should do:

  1. Rename this type
    /// A guard that resets the current default dispatcher to the prior
    /// default dispatcher when dropped.
    #[cfg(feature = "std")]
    struct ResetGuard(Option<Dispatch>);

    to DefaultGuard (for consistency with tokio's APIs), and make it pub.
  2. Add a dispatcher::set_default function that uses the existing code to set the default dispatcher, returning a guard. This is basically already implemented here:
    fn set_default(new_dispatch: Dispatch) -> ResetGuard {
    let prior = CURRENT_STATE
    .try_with(|state| {
    state.can_enter.set(true);
    state.default.replace(new_dispatch)
    })
    .ok();
    EXISTS.store(true, Ordering::Release);
    ResetGuard(prior)
    }
  3. Change dispatcher::with_default to use the new set_default. Don't remove it, since that would be a breaking change.
  4. Update the tracing-core version.
  5. Update tracing to use the new tracing-core version (as a path dep, for now) and add a new tracing::subscriber::set_default that works identically to tracing_core::dispatcher::set_default, but taking a Subscriber instead of a Dispatch.

This should be fairly straightforward, so I'm marking this as a "good first issue" — if anyone wants to work on this, please feel free to ask me any additional questions that come up.

@hawkw hawkw added good first issue and removed kind/rfc labels Oct 15, 2019
@hawkw hawkw changed the title consider a way to set the default subscriber with a guard core: add `dispatcher::set_default` Oct 15, 2019
bIgBV added a commit to bIgBV/tracing that referenced this issue Oct 17, 2019
* Add `tracing::subscriber::set_default` which sets the default
subscriber and returns a drop guard. This drop guard will reset the
dispatch on drop.
* Add `tracing_core::dispatcher::set_default` method which sets the
default dispatch and returns a drop guard.
* Update `tracing_core::dispatcher::with_default` method to use the new
`tracing_core::dispatcher::set_default` method.
* Add test to confirm expected behavior
* Fixes: tokio-rs#383
@hawkw hawkw closed this in 5fced33 Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.