-
Notifications
You must be signed in to change notification settings - Fork 704
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
implement Targets::enabled_for
#1903
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.
looks good to me for the most part! i have some minor style suggestions, and i'm unsure if the TargetMatch
trait is really needed...it seems like we could be able to avoid adding that. let me know what you think about my suggestions!
/// ``` | ||
/// | ||
/// [target]: tracing_core::Metadata::target | ||
/// [`tracing::event!`]: tracing::event! |
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'm surprised this link needs a reference?
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 am not great at rustdoc links so ill check if its needed
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 took forever to figure out, its not required, but rustdoc doesnt work for this unless --all-features
is on (which is on for tracing-subscribers's docs.rs, which is good), as tracing
is an optional dep!
also, i'm not super fond of the name |
@hawkw I like I think I covered all your comments! |
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.
looks good to me, thank you!
one last thought is that it might be nice to add to the unit tests for Targets
to assert that would_enable
always agrees with the output of the actual Filter::enabled
impl, but since the implementation is pretty straightforward, it's probably not a big deal.
# 0.3.8 (Feb 4, 2022) This release adds *experimental* support for recording structured field values using the [`valuable`] crate to the `format::Json` formatter. In particular, user-defined types which are recorded using their [`valuable::Valuable`] implementations will be serialized as JSON objects, rather than using their `fmt::Debug` representation. See [this blog post][post] for details on `valuable`. Note that `valuable` support currently requires `--cfg tracing_unstable`. See the documentation for details. Additionally, this release includes a number of other smaller API improvements. ### Added - **json**: Experimental support for recording [`valuable`] values as structured JSON ([#1862], [#1901]) - **filter**: `Targets::would_enable` method for testing if a `Targets` filter would enable a given target ([#1903]) - **fmt**: `map_event_format`, `map_fmt_fields`, and `map_writer` methods to `fmt::Layer` and `fmt::SubscriberBuilder` ([#1871]) ### Changed - `tracing-core`: updated to [0.1.22][core-0.1.22] ### Fixed - Set `smallvec` minimal version to 1.2.0, to fix compilation errors with `-Z minimal-versions` ([#1890]) - Minor documentation fixes ([#1902], [#1893]) Thanks to @guswynn, @glts, and @lilyball for contributing to this release! [`valuable`]: https://crates.io/crates/valuable [`valuable::Valuable`]: https://docs.rs/valuable/latest/valuable/trait.Valuable.html [post]: https://tokio.rs/blog/2021-05-valuable [core-0.1.22]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.22 [#1862]: #1862 [#1901]: #1901 [#1903]: #1903 [#1871]: #1871 [#1890]: #1890 [#1902]: #1902 [#1893]: #1893
# 0.3.8 (Feb 4, 2022) This release adds *experimental* support for recording structured field values using the [`valuable`] crate to the `format::Json` formatter. In particular, user-defined types which are recorded using their [`valuable::Valuable`] implementations will be serialized as JSON objects, rather than using their `fmt::Debug` representation. See [this blog post][post] for details on `valuable`. Note that `valuable` support currently requires `--cfg tracing_unstable`. See the documentation for details. Additionally, this release includes a number of other smaller API improvements. ### Added - **json**: Experimental support for recording [`valuable`] values as structured JSON ([#1862], [#1901]) - **filter**: `Targets::would_enable` method for testing if a `Targets` filter would enable a given target ([#1903]) - **fmt**: `map_event_format`, `map_fmt_fields`, and `map_writer` methods to `fmt::Layer` and `fmt::SubscriberBuilder` ([#1871]) ### Changed - `tracing-core`: updated to [0.1.22][core-0.1.22] ### Fixed - Set `smallvec` minimal version to 1.2.0, to fix compilation errors with `-Z minimal-versions` ([#1890]) - Minor documentation fixes ([#1902], [#1893]) Thanks to @guswynn, @glts, and @lilyball for contributing to this release! [`valuable`]: https://crates.io/crates/valuable [`valuable::Valuable`]: https://docs.rs/valuable/latest/valuable/trait.Valuable.html [post]: https://tokio.rs/blog/2021-05-valuable [core-0.1.22]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.22 [#1862]: #1862 [#1901]: #1901 [#1903]: #1903 [#1871]: #1871 [#1890]: #1890 [#1902]: #1902 [#1893]: #1893
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
## Motivation As discussed on discord, this API + `Targets` being `: Clone` makes it easier to solve the original problem I had tried to solve in #1889. My plan on how to use this is in https://github.com/MaterializeInc/materialize/issues/10441 if you are interested! ## Solution I considered doing some macro magic to create a `Metadata` with a callsite and empty fields and everything, to be able to called `DirectiveSet::enabled`, but it felt cleaner and easier to reason about the special-case-ness (`Targets` never having field filters) using a new set of methods that do a similar thing. For testing I opted for just a doc-test, let me know if thats fine!
# 0.3.8 (Feb 4, 2022) This release adds *experimental* support for recording structured field values using the [`valuable`] crate to the `format::Json` formatter. In particular, user-defined types which are recorded using their [`valuable::Valuable`] implementations will be serialized as JSON objects, rather than using their `fmt::Debug` representation. See [this blog post][post] for details on `valuable`. Note that `valuable` support currently requires `--cfg tracing_unstable`. See the documentation for details. Additionally, this release includes a number of other smaller API improvements. ### Added - **json**: Experimental support for recording [`valuable`] values as structured JSON ([tokio-rs#1862], [tokio-rs#1901]) - **filter**: `Targets::would_enable` method for testing if a `Targets` filter would enable a given target ([tokio-rs#1903]) - **fmt**: `map_event_format`, `map_fmt_fields`, and `map_writer` methods to `fmt::Layer` and `fmt::SubscriberBuilder` ([tokio-rs#1871]) ### Changed - `tracing-core`: updated to [0.1.22][core-0.1.22] ### Fixed - Set `smallvec` minimal version to 1.2.0, to fix compilation errors with `-Z minimal-versions` ([tokio-rs#1890]) - Minor documentation fixes ([tokio-rs#1902], [tokio-rs#1893]) Thanks to @guswynn, @glts, and @lilyball for contributing to this release! [`valuable`]: https://crates.io/crates/valuable [`valuable::Valuable`]: https://docs.rs/valuable/latest/valuable/trait.Valuable.html [post]: https://tokio.rs/blog/2021-05-valuable [core-0.1.22]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.22 [tokio-rs#1862]: tokio-rs#1862 [tokio-rs#1901]: tokio-rs#1901 [tokio-rs#1903]: tokio-rs#1903 [tokio-rs#1871]: tokio-rs#1871 [tokio-rs#1890]: tokio-rs#1890 [tokio-rs#1902]: tokio-rs#1902 [tokio-rs#1893]: tokio-rs#1893
Motivation
As discussed on discord, this API +
Targets
being: Clone
makes it easier to solve the original problem I had tried to solve in #1889.My plan on how to use this is in https://github.com/MaterializeInc/database-issues/issues/3102 if you are interested!
Solution
I considered doing some macro magic to create a
Metadata
with a callsite and empty fields and everything, to be able to calledDirectiveSet::enabled
, but it felt cleaner and easier to reason about the special-case-ness (Targets
never having field filters) using a new set of methods that do a similar thing.For testing I opted for just a doc-test, let me know if thats fine!