-
Notifications
You must be signed in to change notification settings - Fork 687
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 Filtered::filter_mut #1959
subscriber: add Filtered::filter_mut #1959
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.
overall, this looks good to me --- thanks!
i had a couple suggestions for the documentation, and i thought we might as well also add an immutable accessor while we're here.
I would probably recommend adding examples to the |
283e773
to
7573d2f
Compare
tracing-subscriber/src/reload.rs
Outdated
/// Allows reloading the state of an associated `Layer`. | ||
/// Allows reloading the state of an associated [`Layer`](crate::layer::Layer). | ||
/// | ||
/// # Examples |
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.
I used the same example, I assume that's fine?
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, that's fine!
i might put the examples in the top-level documentation, though --- it looks like most of the existing general usage docs are there, rather than on the individual types in this module. though, honestly, all the documentation here is rather sparse...
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.
do you mean the documentation of the reload
module or of the crate? (I moved it to the reload
module docs for now)
tracing-subscriber/src/reload.rs
Outdated
/// **Warning:** The [`Filtered`](crate::filter::Filtered) type currently can't be changed | ||
/// at runtime via the [`Handle::reload`] method. | ||
/// Use the [`Handle::modify`] method to change the filter instead. | ||
/// (see <https://github.com/tokio-rs/tracing/issues/1629>) |
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.
Not sure if you want a link to the issue in there or not, I can remove it
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.
some more docs suggestions --- sorry for being picky!
tracing-subscriber/src/reload.rs
Outdated
/// Allows reloading the state of an associated `Layer`. | ||
/// Allows reloading the state of an associated [`Layer`](crate::layer::Layer). | ||
/// | ||
/// # Examples |
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, that's fine!
i might put the examples in the top-level documentation, though --- it looks like most of the existing general usage docs are there, rather than on the individual types in this module. though, honestly, all the documentation here is rather sparse...
tracing-subscriber/src/reload.rs
Outdated
/// Reloading a [`Filtered`](crate::filter::Filtered) layer to change the filter at runtime. | ||
/// | ||
/// ``` | ||
/// # use tracing::info; | ||
/// # use tracing_subscriber::{filter,fmt,reload,Registry,prelude::*}; | ||
/// # fn main() { | ||
/// let (filtered_layer, reload_handle) = | ||
/// reload::Layer::new(fmt::Layer::default().with_filter(filter::LevelFilter::WARN)); | ||
/// # | ||
/// # // specifying the Registry type is required | ||
/// # let _: &reload::Handle<filter::Filtered<fmt::Layer<Registry>, | ||
/// # filter::LevelFilter, Registry>,Registry> | ||
/// # = &reload_handle; | ||
/// # | ||
/// info!("This will be ignored"); | ||
/// reload_handle.modify(|layer| *layer.filter_mut() = filter::LevelFilter::INFO); | ||
/// info!("This will be logged"); | ||
/// # } | ||
/// ``` |
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.
it would be nice to also have an example of reloading a global filtering Layer
...i'm surprised we don't have that already. but we should probably add that in a follow-up PR, rather than here.
## Motivation Changing the filter of a Filtered at runtime is currently only possible by replacing it with a new Filtered via the reload::Handle's reload method. This currently doesn't work (see tokio-rs#1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza). Making it possible to change the filter via the handle's modify method and mutating the inner filter is an easy workaround.
7573d2f
to
ddae0e9
Compare
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 good to me! Thanks for taking the time to address all of my docs nitpicks! :)
thanks for the review and the suggestions! |
Missing example was noticed in #1959 (comment) Should the descriptions of the examples distinguish global filtering and per-layer filtering a bit more explicitly? Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Partially fixes #1629 (I think making `reload::Handle::reload` work with `Filtered` would be cleaner, but this approach seemed easier to me) I assumed opening the PR against v0.1.x is correct as I couldn't find the `Filtered` type in master. I think it'd be sensible to note the fact that `reload::Handle::reload` doesn't work with `Filtered` in the docs somewhere, should I add that? ## Motivation Changing the filter of a `Filtered` at runtime is currently only possible by replacing it with a new `Filtered` via the `reload::Handle::reload` method. This currently doesn't work as the new `Filtered` won't receive a `FilterId` (see #1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza) so this seems like a reasonable (and easy) workaround for now. (I can't judge whether this method is only useful as a workaround for the bug or if it suits the public API independently) ## Solution Offer mutable access to the `Filtered::filter` field in the public API. This can be used via the `reload::Handle::modify` method to change the filter inside the existing `Filtered`. Fixes #1629
Partially fixes #1629 (I think making `reload::Handle::reload` work with `Filtered` would be cleaner, but this approach seemed easier to me) I assumed opening the PR against v0.1.x is correct as I couldn't find the `Filtered` type in master. I think it'd be sensible to note the fact that `reload::Handle::reload` doesn't work with `Filtered` in the docs somewhere, should I add that? ## Motivation Changing the filter of a `Filtered` at runtime is currently only possible by replacing it with a new `Filtered` via the `reload::Handle::reload` method. This currently doesn't work as the new `Filtered` won't receive a `FilterId` (see #1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza) so this seems like a reasonable (and easy) workaround for now. (I can't judge whether this method is only useful as a workaround for the bug or if it suits the public API independently) ## Solution Offer mutable access to the `Filtered::filter` field in the public API. This can be used via the `reload::Handle::modify` method to change the filter inside the existing `Filtered`. Fixes #1629
# 0.3.10 (Apr 1, 2022) This release adds several new features, including a `Filter` implementation and new builder API for `EnvFilter`, support for using a `Vec<L> where L: Layer` as a `Layer`, and a number of smaller API improvements to make working with dynamic and reloadable layers easier. ### Added - **registry**: Implement `Filter` for `EnvFilter`, allowing it to be used with per-layer filtering ([#1983]) - **registry**: `Filter::on_new_span`, `Filter::on_enter`, `Filter::on_exit`, `Filter::on_close` and `Filter::on_record` callbacks to allow `Filter`s to track span states internally ([#1973], [#2017], [#2031]) - **registry**: `Filtered::filter` and `Filtered::filter_mut` accessors ([#1959]) - **registry**: `Filtered::inner` and `Filtered::inner_mut` accessors to borrow the wrapped `Layer` ([#2034]) - **layer**: Implement `Layer` for `Vec<L: Layer>`, to allow composing together a dynamically sized list of `Layer`s ([#2027]) - **layer**: `Layer::boxed` method to make type-erasing `Layer`s easier ([#2026]) - **fmt**: `fmt::Layer::writer` and `fmt::Layer::writer_mut` accessors ([#2034]) - **fmt**: `fmt::Layer::set_ansi` method to allow changing the ANSI formatting configuration at runtime ([#2034]) - **env-filter**: `EnvFilter::builder` to configure a new `EnvFilter` prior to parsing it ([#2035]) - Several documentation fixes and improvements ([#1972], [#1971], [#2023], [#2023]) ### Fixed - **fmt**: `fmt::Layer`'s auto traits no longer depend on the `Subscriber` type parameter's auto traits ([2025]) - **env-filter**: Fixed missing help text when the `ansi` feature is disabled ([#2029]) Thanks to new contributors @TimoFreiberg and @wagenet, as well as @CAD97 for contributing to this release! [#1983]: #1983 [#1973]: #1973 [#2017]: #2017 [#2031]: #2031 [#1959]: #1959 [#2034]: #2034 [#2027]: #2027 [#2026]: #2026 [#2035]: #2035 [#1972]: #1972 [#1971]: #1971 [#2023]: #2023
# 0.3.10 (Apr 1, 2022) This release adds several new features, including a `Filter` implementation and new builder API for `EnvFilter`, support for using a `Vec<L> where L: Layer` as a `Layer`, and a number of smaller API improvements to make working with dynamic and reloadable layers easier. ### Added - **registry**: Implement `Filter` for `EnvFilter`, allowing it to be used with per-layer filtering ([#1983]) - **registry**: `Filter::on_new_span`, `Filter::on_enter`, `Filter::on_exit`, `Filter::on_close` and `Filter::on_record` callbacks to allow `Filter`s to track span states internally ([#1973], [#2017], [#2031]) - **registry**: `Filtered::filter` and `Filtered::filter_mut` accessors ([#1959]) - **registry**: `Filtered::inner` and `Filtered::inner_mut` accessors to borrow the wrapped `Layer` ([#2034]) - **layer**: Implement `Layer` for `Vec<L: Layer>`, to allow composing together a dynamically sized list of `Layer`s ([#2027]) - **layer**: `Layer::boxed` method to make type-erasing `Layer`s easier ([#2026]) - **fmt**: `fmt::Layer::writer` and `fmt::Layer::writer_mut` accessors ([#2034]) - **fmt**: `fmt::Layer::set_ansi` method to allow changing the ANSI formatting configuration at runtime ([#2034]) - **env-filter**: `EnvFilter::builder` to configure a new `EnvFilter` prior to parsing it ([#2035]) - Several documentation fixes and improvements ([#1972], [#1971], [#2023], [#2023]) ### Fixed - **fmt**: `fmt::Layer`'s auto traits no longer depend on the `Subscriber` type parameter's auto traits ([2025]) - **env-filter**: Fixed missing help text when the `ansi` feature is disabled ([#2029]) Thanks to new contributors @TimoFreiberg and @wagenet, as well as @CAD97 for contributing to this release! [#1983]: #1983 [#1973]: #1973 [#2017]: #2017 [#2031]: #2031 [#1959]: #1959 [#2034]: #2034 [#2027]: #2027 [#2026]: #2026 [#2035]: #2035 [#1972]: #1972 [#1971]: #1971 [#2023]: #2023
# 0.3.10 (Apr 1, 2022) This release adds several new features, including a `Filter` implementation and new builder API for `EnvFilter`, support for using a `Vec<L> where L: Layer` as a `Layer`, and a number of smaller API improvements to make working with dynamic and reloadable layers easier. ### Added - **registry**: Implement `Filter` for `EnvFilter`, allowing it to be used with per-layer filtering ([#1983]) - **registry**: `Filter::on_new_span`, `Filter::on_enter`, `Filter::on_exit`, `Filter::on_close` and `Filter::on_record` callbacks to allow `Filter`s to track span states internally ([#1973], [#2017], [#2031]) - **registry**: `Filtered::filter` and `Filtered::filter_mut` accessors ([#1959]) - **registry**: `Filtered::inner` and `Filtered::inner_mut` accessors to borrow the wrapped `Layer` ([#2034]) - **layer**: Implement `Layer` for `Vec<L: Layer>`, to allow composing together a dynamically sized list of `Layer`s ([#2027]) - **layer**: `Layer::boxed` method to make type-erasing `Layer`s easier ([#2026]) - **fmt**: `fmt::Layer::writer` and `fmt::Layer::writer_mut` accessors ([#2034]) - **fmt**: `fmt::Layer::set_ansi` method to allow changing the ANSI formatting configuration at runtime ([#2034]) - **env-filter**: `EnvFilter::builder` to configure a new `EnvFilter` prior to parsing it ([#2035]) - Several documentation fixes and improvements ([#1972], [#1971], [#2023], [#2023]) ### Fixed - **fmt**: `fmt::Layer`'s auto traits no longer depend on the `Subscriber` type parameter's auto traits ([2025]) - **env-filter**: Fixed missing help text when the `ansi` feature is disabled ([#2029]) Thanks to new contributors @TimoFreiberg and @wagenet, as well as @CAD97 for contributing to this release! [#1983]: #1983 [#1973]: #1973 [#2017]: #2017 [#2031]: #2031 [#1959]: #1959 [#2034]: #2034 [#2027]: #2027 [#2026]: #2026 [#2035]: #2035 [#1972]: #1972 [#1971]: #1971 [#2023]: #2023
# 0.3.10 (Apr 1, 2022) This release adds several new features, including a `Filter` implementation and new builder API for `EnvFilter`, support for using a `Vec<L> where L: Layer` as a `Layer`, and a number of smaller API improvements to make working with dynamic and reloadable layers easier. ### Added - **registry**: Implement `Filter` for `EnvFilter`, allowing it to be used with per-layer filtering ([#1983]) - **registry**: `Filter::on_new_span`, `Filter::on_enter`, `Filter::on_exit`, `Filter::on_close` and `Filter::on_record` callbacks to allow `Filter`s to track span states internally ([#1973], [#2017], [#2031]) - **registry**: `Filtered::filter` and `Filtered::filter_mut` accessors ([#1959]) - **registry**: `Filtered::inner` and `Filtered::inner_mut` accessors to borrow the wrapped `Layer` ([#2034]) - **layer**: Implement `Layer` for `Vec<L: Layer>`, to allow composing together a dynamically sized list of `Layer`s ([#2027]) - **layer**: `Layer::boxed` method to make type-erasing `Layer`s easier ([#2026]) - **fmt**: `fmt::Layer::writer` and `fmt::Layer::writer_mut` accessors ([#2034]) - **fmt**: `fmt::Layer::set_ansi` method to allow changing the ANSI formatting configuration at runtime ([#2034]) - **env-filter**: `EnvFilter::builder` to configure a new `EnvFilter` prior to parsing it ([#2035]) - Several documentation fixes and improvements ([#1972], [#1971], [#2023], [#2023]) ### Fixed - **fmt**: `fmt::Layer`'s auto traits no longer depend on the `Subscriber` type parameter's auto traits ([2025]) - **env-filter**: Fixed missing help text when the `ansi` feature is disabled ([#2029]) Thanks to new contributors @TimoFreiberg and @wagenet, as well as @CAD97 for contributing to this release! [#1983]: #1983 [#1973]: #1973 [#2017]: #2017 [#2031]: #2031 [#1959]: #1959 [#2034]: #2034 [#2027]: #2027 [#2026]: #2026 [#2035]: #2035 [#1972]: #1972 [#1971]: #1971 [#2023]: #2023
# 0.3.10 (Apr 1, 2022) This release adds several new features, including a `Filter` implementation and new builder API for `EnvFilter`, support for using a `Vec<L> where L: Layer` as a `Layer`, and a number of smaller API improvements to make working with dynamic and reloadable layers easier. ### Added - **registry**: Implement `Filter` for `EnvFilter`, allowing it to be used with per-layer filtering ([#1983]) - **registry**: `Filter::on_new_span`, `Filter::on_enter`, `Filter::on_exit`, `Filter::on_close` and `Filter::on_record` callbacks to allow `Filter`s to track span states internally ([#1973], [#2017], [#2031]) - **registry**: `Filtered::filter` and `Filtered::filter_mut` accessors ([#1959]) - **registry**: `Filtered::inner` and `Filtered::inner_mut` accessors to borrow the wrapped `Layer` ([#2034]) - **layer**: Implement `Layer` for `Vec<L: Layer>`, to allow composing together a dynamically sized list of `Layer`s ([#2027]) - **layer**: `Layer::boxed` method to make type-erasing `Layer`s easier ([#2026]) - **fmt**: `fmt::Layer::writer` and `fmt::Layer::writer_mut` accessors ([#2034]) - **fmt**: `fmt::Layer::set_ansi` method to allow changing the ANSI formatting configuration at runtime ([#2034]) - **env-filter**: `EnvFilter::builder` to configure a new `EnvFilter` prior to parsing it ([#2035]) - Several documentation fixes and improvements ([#1972], [#1971], [#2023], [#2023]) ### Fixed - **fmt**: `fmt::Layer`'s auto traits no longer depend on the `Subscriber` type parameter's auto traits ([#2025]) - **env-filter**: Fixed missing help text when the `ansi` feature is disabled ([#2029]) Thanks to new contributors @TimoFreiberg and @wagenet, as well as @CAD97 for contributing to this release! [#1983]: #1983 [#1973]: #1973 [#2017]: #2017 [#2031]: #2031 [#1959]: #1959 [#2034]: #2034 [#2027]: #2027 [#2026]: #2026 [#2035]: #2035 [#1972]: #1972 [#1971]: #1971 [#2023]: #2023 [#2025]: #2025 [#2029]: #2029
Partially fixes tokio-rs#1629 (I think making `reload::Handle::reload` work with `Filtered` would be cleaner, but this approach seemed easier to me) I assumed opening the PR against v0.1.x is correct as I couldn't find the `Filtered` type in master. I think it'd be sensible to note the fact that `reload::Handle::reload` doesn't work with `Filtered` in the docs somewhere, should I add that? ## Motivation Changing the filter of a `Filtered` at runtime is currently only possible by replacing it with a new `Filtered` via the `reload::Handle::reload` method. This currently doesn't work as the new `Filtered` won't receive a `FilterId` (see tokio-rs#1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza) so this seems like a reasonable (and easy) workaround for now. (I can't judge whether this method is only useful as a workaround for the bug or if it suits the public API independently) ## Solution Offer mutable access to the `Filtered::filter` field in the public API. This can be used via the `reload::Handle::modify` method to change the filter inside the existing `Filtered`. Fixes tokio-rs#1629
# 0.3.10 (Apr 1, 2022) This release adds several new features, including a `Filter` implementation and new builder API for `EnvFilter`, support for using a `Vec<L> where L: Layer` as a `Layer`, and a number of smaller API improvements to make working with dynamic and reloadable layers easier. ### Added - **registry**: Implement `Filter` for `EnvFilter`, allowing it to be used with per-layer filtering ([tokio-rs#1983]) - **registry**: `Filter::on_new_span`, `Filter::on_enter`, `Filter::on_exit`, `Filter::on_close` and `Filter::on_record` callbacks to allow `Filter`s to track span states internally ([tokio-rs#1973], [tokio-rs#2017], [tokio-rs#2031]) - **registry**: `Filtered::filter` and `Filtered::filter_mut` accessors ([tokio-rs#1959]) - **registry**: `Filtered::inner` and `Filtered::inner_mut` accessors to borrow the wrapped `Layer` ([tokio-rs#2034]) - **layer**: Implement `Layer` for `Vec<L: Layer>`, to allow composing together a dynamically sized list of `Layer`s ([tokio-rs#2027]) - **layer**: `Layer::boxed` method to make type-erasing `Layer`s easier ([tokio-rs#2026]) - **fmt**: `fmt::Layer::writer` and `fmt::Layer::writer_mut` accessors ([tokio-rs#2034]) - **fmt**: `fmt::Layer::set_ansi` method to allow changing the ANSI formatting configuration at runtime ([tokio-rs#2034]) - **env-filter**: `EnvFilter::builder` to configure a new `EnvFilter` prior to parsing it ([tokio-rs#2035]) - Several documentation fixes and improvements ([tokio-rs#1972], [tokio-rs#1971], [tokio-rs#2023], [tokio-rs#2023]) ### Fixed - **fmt**: `fmt::Layer`'s auto traits no longer depend on the `Subscriber` type parameter's auto traits ([tokio-rs#2025]) - **env-filter**: Fixed missing help text when the `ansi` feature is disabled ([tokio-rs#2029]) Thanks to new contributors @TimoFreiberg and @wagenet, as well as @CAD97 for contributing to this release! [tokio-rs#1983]: tokio-rs#1983 [tokio-rs#1973]: tokio-rs#1973 [tokio-rs#2017]: tokio-rs#2017 [tokio-rs#2031]: tokio-rs#2031 [tokio-rs#1959]: tokio-rs#1959 [tokio-rs#2034]: tokio-rs#2034 [tokio-rs#2027]: tokio-rs#2027 [tokio-rs#2026]: tokio-rs#2026 [tokio-rs#2035]: tokio-rs#2035 [tokio-rs#1972]: tokio-rs#1972 [tokio-rs#1971]: tokio-rs#1971 [tokio-rs#2023]: tokio-rs#2023 [tokio-rs#2025]: tokio-rs#2025 [tokio-rs#2029]: tokio-rs#2029
Partially fixes #1629 (I think making
reload::Handle::reload
work withFiltered
would be cleaner, but this approach seemed easier to me)I assumed opening the PR against v0.1.x is correct as I couldn't find the
Filtered
type in master.I think it'd be sensible to note the fact that
reload::Handle::reload
doesn't work withFiltered
in the docs somewhere, should I add that?Motivation
Changing the filter of a
Filtered
at runtime is currently only possible by replacing it with a newFiltered
via thereload::Handle::reload
method.This currently doesn't work as the new
Filtered
won't receive aFilterId
(see #1629).While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza) so this seems like a reasonable (and easy) workaround for now.
(I can't judge whether this method is only useful as a workaround for the bug or if it suits the public API independently)
Solution
Offer mutable access to the
Filtered::filter
field in the public API. This can be used via thereload::Handle::modify
method to change the filter inside the existingFiltered
.