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

subscriber: add more ergonomic subscriber configuration APIs #660

Merged
merged 14 commits into from
Apr 4, 2020

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Apr 3, 2020

Motivation

Users have said fairly frequently that configuring new subscribers in an
application is unnecessarily verbose and confusing. We should try to
make this nicer, especially as it's a common "on ramp" for new tracing
users.

Solution

This branch adds new APIs, inspired by tonic and warp, that should
make setting up a subscriber a little less verbose. This includes:

  • Many modules in tracing-subscriber now expose free functions
    that construct default instances of various types. This makes
    configuring subscribers using these types more concise, a la warp.
  • An extension trait for adding .set_default, .init, and .try_init
    methods to subscribers. This generalizes the similar functions on
    fmt's SubscriberBuilder to work with other subscribers.

All the old APIs are still left as they were previously. The new APIs
just provide shorthand for them.

@hawkw hawkw added kind/feature New feature or request crate/subscriber Related to the `tracing-subscriber` crate labels Apr 3, 2020
@hawkw hawkw requested review from yaahc, davidbarsky, LucioFranco and a team April 3, 2020 00:11
@hawkw hawkw self-assigned this Apr 3, 2020
@yaahc
Copy link
Collaborator

yaahc commented Apr 3, 2020

We could have set_global_default and init?

color_backtrace uses install() which I quite like. As for the try_init vs init. I'm kinda of the opinion that the panicking version might just be the better version to push people towards, because how often are you going to want to recover from errors configuring the global logger?

I think it might be okay to only have init and direct people to use the less ergonomic but existing tracing::subscriber::set_global_default if they want to recover from from the failure. My guess is that most people will be fine with using init, and if people are really annoyed by the inconsistent ergonomics between panicking and non-panicking global setup we can always add try_init or set_global_default to the trait later.

@@ -50,11 +50,11 @@ fn do_another_thing(

#[tracing::instrument]
fn main() {
let subscriber = Registry::default()
.with(FmtLayer::default())
tracing_subscriber::registry()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish there was a way to replace this expr with something like tracing_subscriber::fmt(), aka some sort of conversion fn for a fmt builder to a registry + layer

hawkw added 10 commits April 3, 2020 12:07
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
examples/examples/fmt-custom-event.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/fmt/mod.rs Show resolved Hide resolved
Copy link
Member

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a pretty good improvement and I'm not seeing any issues. I'd like to try this out and report back if I run into any issues.

examples/examples/fmt-custom-field.rs Show resolved Hide resolved
hawkw added 3 commits April 3, 2020 14:42
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Member Author

hawkw commented Apr 3, 2020

@yaahc are you good with this or do we want to continue bikeshedding naming? I'm open to further discussion if you don't like the current scheme.

Re:

I think it might be okay to only have init and direct people to use the less ergonomic but existing tracing::subscriber::set_global_default if they want to recover from from the failure. My guess is that most people will be fine with using init, and if people are really annoyed by the inconsistent ergonomics between panicking and non-panicking global setup we can always add try_init or set_global_default to the trait later.

Hmm, maybe...I think the main use-case for the non-panicking version of the API, besides cases where people want to return as many errors from main as possible, rather than panicking, is one where we don't know if a global subscriber has been set or not. For example, this is pretty common in tests, where people want to say things like

#[test]
fn my_great_test() {
    // ignore errors if another test has already set the global subscriber
    let _ = tracing_subscriber::fmt()
        .with_whatever(...)
        .try_init();

    // ...
}

which is also a pretty common pattern in the env_logger world.

And, since trying to set a global subscriber in tests is one of the few cases where people will actually initialize a subscriber more than once per program, I think it may actually be one of the cases where providing an ergonomic API has the most impact...

What do you all think?

@yaahc
Copy link
Collaborator

yaahc commented Apr 3, 2020

This makes sense and i agree with it. +1

@hawkw hawkw merged commit 62e0cc0 into master Apr 4, 2020
hawkw added a commit that referenced this pull request Apr 6, 2020
# 0.2.4 (April 6, 2020)

This release includes several API ergonomics improvements, including
shorthand constructors for many types, and an extension trait for
initializing subscribers using method-chaining style. Additionally,
several bugs in less commonly used `fmt` APIs were fixed.

### Added

- **fmt**: Shorthand free functions for constructing most types in `fmt`
  (including `tracing_subscriber::fmt()` to return a
  `SubscriberBuilder`, `tracing_subscriber::fmt::layer()` to return a
  format `Layer`, etc) (#660)
- **registry**: Shorthand free function `tracing_subscriber::registry()`
  to construct a new registry (#660)
- Added `SubscriberInitExt` extension trait for more ergonomic
  subscriber initialization (#660)
  
### Changed

- **fmt**: Moved `LayerBuilder` methods to `Layer` (#655)

### Deprecated

- **fmt**: `LayerBuilder`, as `Layer` now implements all builder methods
  (#655)
  
### Fixed

- **fmt**: Fixed `Compact` formatter not omitting levels with
  `with_level(false)` (#657)
- **fmt**: Fixed `fmt::Layer` duplicating the fields for a new span if
  another layer has already formatted its fields (#634)
- **fmt**: Added missing space when using `record` to add new fields to
  a span that already has fields (#659)
- Updated outdated documentation (#647)


Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/subscriber Related to the `tracing-subscriber` crate kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants