-
Notifications
You must be signed in to change notification settings - Fork 716
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: adds From<Level> for Directive #918
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks!
If you have a minute, it might be nice to add an example in the add_directive
docs (here) showing usage with a Level
rather than a LevelFilter
? That should make this more obvious for other users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for adding the docs! i had a couple little nits to pick, but if we get those taken care of, i think this will be good to merge!
/// enable all traces at or below a certain verbosity level, or | ||
/// parsed from a string specifying a directive. | ||
/// | ||
/// If a filter directive is inserted that matches exactly the same spans | ||
/// and events as a previous filter, but sets a different level for those | ||
/// spans and events, the previous directive is overwritten. | ||
/// | ||
/// [`LevelFilter`]: struct.LevelFilter.html | ||
/// [`LevelFilter`]: ../filter/struct.LevelFilter.html | ||
/// [`Level`]: ../../tracing_core/struct.Level.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this link work on docs.rs? i think relative URLs like this will work locally & on netlify, since we are building all the other docs in the same target dir, but i don't think it will work on docs.rs. typically, i use the full docs.rs URL when linking across crates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I built with cargo doc --open
it did. I actually copied it from the top of the same file, there's a bunch that use that path. If it doesn't work on docsrs we may want to change the links at the top too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah so I just tried https://docs.rs/tracing-subscriber/0.2.11/tracing_subscriber/filter/struct.EnvFilter.html and none of the links at the top work, but they do work locally.
So, what should I put here? literally https://docs.rs/tracing-core/0.1.14/tracing_core/metadata/struct.Level.html
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what should I put here? literally
https://docs.rs/tracing-core/0.1.14/tracing_core/metadata/struct.Level.html
?
Yup, that's right.
/// From [`LevelFilter`] | ||
/// ```rust | ||
/// use tracing_subscriber::filter::{EnvFilter, LevelFilter}; | ||
/// let mut filter = EnvFilter::from_default_env() | ||
/// .add_directive(LevelFilter::INFO.into()); | ||
/// ``` | ||
/// Or from [`Level`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a little formatting nit:
/// From [`LevelFilter`] | |
/// ```rust | |
/// use tracing_subscriber::filter::{EnvFilter, LevelFilter}; | |
/// let mut filter = EnvFilter::from_default_env() | |
/// .add_directive(LevelFilter::INFO.into()); | |
/// ``` | |
/// Or from [`Level`] | |
/// | |
/// From [`LevelFilter`]: | |
/// | |
/// ```rust | |
/// use tracing_subscriber::filter::{EnvFilter, LevelFilter}; | |
/// let mut filter = EnvFilter::from_default_env() | |
/// .add_directive(LevelFilter::INFO.into()); | |
/// ``` | |
/// | |
/// Or from [`Level`]: | |
/// |
/// ``` | ||
/// Parsed from a string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
/// ``` | |
/// Parsed from a string | |
/// ``` | |
/// | |
/// Parsed from a string: | |
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once the CI build passes, i'll be happy to merge this!
/// | ||
/// ```rust | ||
/// # use tracing_subscriber::filter::{EnvFilter, LevelFilter}; | ||
/// # use tracing::Level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doctest doesn't compile, because it's missing a semicolon. i think adding that
/// # use tracing::Level | |
/// # use tracing::Level; |
should fix CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woops, fixed that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, this looks great! thanks again!
Fixed - **env-filter**: Fixed a regression where `Option<Level>` lost its `Into<LevelFilter>` impl ([#966]) - **env-filter**: Fixed `EnvFilter` enabling spans that should not be enabled when multiple subscribers are in use ([#927]) Changed - **json**: `format::Json` now outputs fields in a more readable order ([#892]) - Updated `tracing-core` dependency to 0.1.16 Added - **fmt**: Add `BoxMakeWriter` for erasing the type of a `MakeWriter` implementation ([#958]) - **fmt**: Add `TestWriter` `MakeWriter` implementation to support libtest output capturing ([#938]) - **layer**: Add `Layer` impl for `Option<T> where T: Layer` ([#910]) - **env-filter**: Add `From<Level>` impl for `Directive` ([#918]) - Multiple documentation fixes and improvements Thanks to @Pothulapati, @samrg472, @bryanburgers, @keetonian, and @SriRamanujam for contributing to this release! [#927]: #927 [#966]: #966 [#958]: #958 [#892]: #892 [#938]: #938 [#910]: #910 [#918]: #918
# 0.2.12 (September 8, 2020) ### Fixed - **env-filter**: Fixed a regression where `Option<Level>` lost its `Into<LevelFilter>` impl ([#966]) - **env-filter**: Fixed `EnvFilter` enabling spans that should not be enabled when multiple subscribers are in use ([#927]) ### Changed - **json**: `format::Json` now outputs fields in a more readable order ([#892]) - Updated `tracing-core` dependency to 0.1.16 ### Added - **fmt**: Add `BoxMakeWriter` for erasing the type of a `MakeWriter` implementation ([#958]) - **fmt**: Add `TestWriter` `MakeWriter` implementation to support libtest output capturing ([#938]) - **layer**: Add `Layer` impl for `Option<T> where T: Layer` ([#910]) - **env-filter**: Add `From<Level>` impl for `Directive` ([#918]) - Multiple documentation fixes and improvements Thanks to @Pothulapati, @samrg472, @bryanburgers, @keetonian, and @SriRamanujam for contributing to this release! [#927]: #927 [#966]: #966 [#958]: #958 [#892]: #892 [#938]: #938 [#910]: #910 [#918]: #918
Motivation
If you have a
Level
and want to set a directive forEnvFilter
you need to do the mechanical transformation of:.add_directive(LevelFilter::from_level(log_lvl).into())
. To me, it wasn't obvious initially that this was even possible, I had to look through the docs for two different tracing repos to find that the pathway would work.Solution
If this implementation is added, the definition will be shown on the
Directive
docs page, and you can write.add_directive(Level::INFO.into())
and skip the intermediate transformationThere was some discussion on discord about this, if it's not desirable because of other reasons feel free to close