-
Notifications
You must be signed in to change notification settings - Fork 726
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
Add Subscribe::event_enabled to conditionally dis/enable events based on fields #2008
Conversation
I'm on board with adding this. I think it makes sense to add the ability to filter out an event after its fields are recorded, people have definitely asked for this in the past. In addition to the use-case of filtering based on fields, it would also help with something else I've heard some demand for --- rate limiting event instances based on their fields, so that an event is rate-limited only if it's recorded multiple times with the same field values. The one thing I would note that's important to document is that this method is going to behave differently from
I think this makes sense, as it would also allow us to add this API without a breaking change. Since we don't really expect user code to call |
22dc571
to
ad609d5
Compare
This now uses a design where events are separately checked for The one downside I see to this design is that there might be a layer which could fuse some work between This PR is now ready for review to merge. |
Interesting alternative solution: just add EDIT: wait, no, |
ad609d5
to
fde00c9
Compare
Rebased. I'd appreciate if this could move forward, as it has implications for my tracing-filter work. |
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'd definitely like to move forward with this change, as well!
The current API proposal seems good to me --- @davidbarsky, want to weigh in?
I think the next step will be actually implementing the intended behavior (i.e. actually calling event_enabled
and skipping events based on it). IMO, the current API design seems good and we can go ahead and start on actually implementing it.
We may also want to consider adding similar functionality to Filter
, to allow a per-subscriber filter to disable recording an event for its attached subscriber based on field values, but that can be done in a follow-up change.
Whoops, that was there in ad609d5 but got lost in the rebase 😅 It should be back now! I also added some trait-level docs that should help clarify the relationship between the three filtering methods. |
6968e3c
to
40db607
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.
One thing that I think we need to figure out is that, unlike enabled
, the event_enabled
method is order-dependent. If a subscriber returns false
in event_enabled
, any subscribers above it in the stack will still have recorded the event.
Ideally, I think we would want to change this to behave like enabled
does, which it looks like we've made a partial attempt to do: the implementation of event_enabled
for Layered
is non-order-dependent...when a Layered
consists of two Subscribe
rs. However, in most cases, a Layered
consists of a Subscribe
r and a Collect
or, which may itself be a Layered
.
Since we don't have an event_enabled
method for Collect
, Layered
's Collect
implementation doesn't continue asking the rest of the stack before returning true
or false
, so the non-order-dependent behavior only exists when subscribers are composed individually before being layered onto a Collect
or.
This seems unfortunate. We should try to have consistent behavior here.
I think that if we want to make event_enabled
non-order-dependent, we would need to also add an event_enabled
method to the Collect
trait. This could have a default impl that returns true
, so it wouldn't be a breaking change to add this in v0.1.x. This would allow the Collect
impl for Layered
to behave the same as the Subscribe
impl for Layered
.
Alternatively, we could just decide that we are okay with order-dependent behavior for event_enabled
, and change the Subscribe
impl for Layered
to match. However, this seems not great, because it's inconsistent with all the rest of the filtering methods...
Honestly, I just hadn't realized that it was still order-dependent; the point of I'm not super happy with the docs -- it's kind of weird to talk about The other thing that should be added is a test that this commit fixes, but I'm completely unsure how to go about structuring such a test. |
d9591bd
to
53ed1dc
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.
overall, the code change change looks good to me, thanks for working on this! i had some minor suggestions for the documentation, but once those are addressed, i'll be very happy to merge this!
@@ -686,7 +686,31 @@ pub(crate) mod tests; | |||
/// be composed together with other subscribers to build a [collector]. See the | |||
/// [module-level documentation](crate::subscribe) for details. | |||
/// | |||
/// # Enabling interest | |||
/// | |||
/// Whenever an tracing event (or span) is emitted, it goes through a number of |
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.
might be nice if "event" and "span" linked to the tracing
docs, but it's not a big deal --- people reading this documentation probably know what those are by now.
also, @davidbarsky, do you also want to take a look at the docs change in this PR? no pressure either way! |
i think you got most of concerns :) |
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
e065edb
to
f5f1758
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.
this looks good to me overall. i left a couple last suggestions, but i'm happy to merge the PR regardless.
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
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 the additional tests, this looks good to me!
Ugh, the CI failure is unrelated and is due to another crate's dependency changing its MSRV: https://github.com/tokio-rs/tracing/runs/6990243761?check_suite_focus=true I'll fix that separately. |
## Motivation Allow filter layers to filter on the contents of events (see #2007). ## Solution This branch adds a new `Subscriber::event_enabled` method, taking an `Event` and returning `bool`. This is called before the `Subscriber::event` method, and if it returns `false`, `Subscriber::event` is not called. For simple subscriber (e.g. not using `Layer`s), the `event_enabled` method is not particulary necessary, as the subscriber can just skip the `event` call. However, this branch also adds a new `Layer::event_enabled` method, with the signature: ```rust fn event_enabled(&self, event: &Event<'_>, ctx: Context<'_, S>) -> bool; ``` This is called before `Layer::on_event`, and if `event_enabled` returns `false`, `on_event` is not called (nor is `Subscriber::event`). This allows filter `Layer`s to filter out an `Event` based on its fields. Closes #2007
## Motivation Allow filter layers to filter on the contents of events (see #2007). ## Solution This branch adds a new `Subscriber::event_enabled` method, taking an `Event` and returning `bool`. This is called before the `Subscriber::event` method, and if it returns `false`, `Subscriber::event` is not called. For simple subscriber (e.g. not using `Layer`s), the `event_enabled` method is not particulary necessary, as the subscriber can just skip the `event` call. However, this branch also adds a new `Layer::event_enabled` method, with the signature: ```rust fn event_enabled(&self, event: &Event<'_>, ctx: Context<'_, S>) -> bool; ``` This is called before `Layer::on_event`, and if `event_enabled` returns `false`, `on_event` is not called (nor is `Subscriber::event`). This allows filter `Layer`s to filter out an `Event` based on its fields. Closes #2007
## Motivation Allow filter layers to filter on the contents of events (see #2007). ## Solution This branch adds a new `Subscriber::event_enabled` method, taking an `Event` and returning `bool`. This is called before the `Subscriber::event` method, and if it returns `false`, `Subscriber::event` is not called. For simple subscriber (e.g. not using `Layer`s), the `event_enabled` method is not particulary necessary, as the subscriber can just skip the `event` call. However, this branch also adds a new `Layer::event_enabled` method, with the signature: ```rust fn event_enabled(&self, event: &Event<'_>, ctx: Context<'_, S>) -> bool; ``` This is called before `Layer::on_event`, and if `event_enabled` returns `false`, `on_event` is not called (nor is `Subscriber::event`). This allows filter `Layer`s to filter out an `Event` based on its fields. Closes #2007
## Motivation Allow filter layers to filter on the contents of events (see #2007). ## Solution This branch adds a new `Subscriber::event_enabled` method, taking an `Event` and returning `bool`. This is called before the `Subscriber::event` method, and if it returns `false`, `Subscriber::event` is not called. For simple subscriber (e.g. not using `Layer`s), the `event_enabled` method is not particulary necessary, as the subscriber can just skip the `event` call. However, this branch also adds a new `Layer::event_enabled` method, with the signature: ```rust fn event_enabled(&self, event: &Event<'_>, ctx: Context<'_, S>) -> bool; ``` This is called before `Layer::on_event`, and if `event_enabled` returns `false`, `on_event` is not called (nor is `Subscriber::event`). This allows filter `Layer`s to filter out an `Event` based on its fields. Closes #2007
# 0.1.28 (June 23, 2022) This release of `tracing-core` adds new `Value` implementations, including one for `String`, to allow recording `&String` as a value without having to call `as_str()` or similar, and for 128-bit integers (`i128` and `u128`). In addition, it adds new methods and trait implementations for `Subscriber`s. ### Added - `Value` implementation for `String` ([#2164]) - `Value` implementation for `u128` and `i28` ([#2166]) - `downcast_ref` and `is` methods for `dyn Subscriber + Sync`, `dyn Subscriber + Send`, and `dyn Subscriber + Send + Sync` ([#2160]) - `Subscriber::event_enabled` method to enable filtering based on `Event` field values ([#2008]) - `Subscriber` implementation for `Box<S: Subscriber + ?Sized>` and `Arc<S: Subscriber + ?Sized>` ([#2161]) Thanks to @jswrenn and @CAD97 for contributing to this release! [#2164]: #2164 [#2166]: #2166 [#2160]: #2160 [#2008]: #2008 [#2161]: #2161
# 0.1.28 (June 23, 2022) This release of `tracing-core` adds new `Value` implementations, including one for `String`, to allow recording `&String` as a value without having to call `as_str()` or similar, and for 128-bit integers (`i128` and `u128`). In addition, it adds new methods and trait implementations for `Subscriber`s. ### Added - `Value` implementation for `String` ([#2164]) - `Value` implementation for `u128` and `i28` ([#2166]) - `downcast_ref` and `is` methods for `dyn Subscriber + Sync`, `dyn Subscriber + Send`, and `dyn Subscriber + Send + Sync` ([#2160]) - `Subscriber::event_enabled` method to enable filtering based on `Event` field values ([#2008]) - `Subscriber` implementation for `Box<S: Subscriber + ?Sized>` and `Arc<S: Subscriber + ?Sized>` ([#2161]) Thanks to @jswrenn and @CAD97 for contributing to this release! [#2164]: #2164 [#2166]: #2166 [#2160]: #2160 [#2008]: #2008 [#2161]: #2161
# 0.3.12 (Jun 29, 2022) This release of `tracing-subscriber` adds a new `Layer::event_enabled` method, which allows `Layer`s to filter events *after* their field values are recorded; a `Filter` implementation for `reload::Layer`, to make using `reload` with per-layer filtering more ergonomic, and additional inherent method downcasting APIs for the `Layered` type. In addition, it includes dependency updates, and minor fixes for documentation and feature flagging. ### Added - **layer**: `Layer::event_enabled` method, which can be implemented to filter events based on their field values (#2008) - **reload**: `Filter` implementation for `reload::Layer` (#2159) - **layer**: `Layered::downcast_ref` and `Layered::is` inherent methods (#2160) ### Changed - **parking_lot**: Updated dependency on `parking_lot` to 0.13.0 (#2143) - Replaced `lazy_static` dependency with `once_cell` (#2147) ### Fixed - Don't enable `tracing-core` features by default (#2107) - Several documentation link and typo fixes (#2064, #2068, #2077, #2161, #1088) Thanks to @ben0x539, @jamesmunns, @georgemp, @james7132, @jswrenn, @CAD97, and @guswynn for contributing to this release!
# 0.3.12 (Jun 29, 2022) This release of `tracing-subscriber` adds a new `Layer::event_enabled` method, which allows `Layer`s to filter events *after* their field values are recorded; a `Filter` implementation for `reload::Layer`, to make using `reload` with per-layer filtering more ergonomic, and additional inherent method downcasting APIs for the `Layered` type. In addition, it includes dependency updates, and minor fixes for documentation and feature flagging. ### Added - **layer**: `Layer::event_enabled` method, which can be implemented to filter events based on their field values (#2008) - **reload**: `Filter` implementation for `reload::Layer` (#2159) - **layer**: `Layered::downcast_ref` and `Layered::is` inherent methods (#2160) ### Changed - **parking_lot**: Updated dependency on `parking_lot` to 0.13.0 (#2143) - Replaced `lazy_static` dependency with `once_cell` (#2147) ### Fixed - Don't enable `tracing-core` features by default (#2107) - Several documentation link and typo fixes (#2064, #2068, #2077, #2161, #1088) Thanks to @ben0x539, @jamesmunns, @georgemp, @james7132, @jswrenn, @CAD97, and @guswynn for contributing to this release!
PR #2008 added a new method to the `tracing_core::Subscriber` trait, and modified `tracing-subscriber` to and implement that method. However, that PR did *not* increase the minimum `tracing-core` dependency for the `tracing-subscriber` crate. This means that if a dependent of `tracing-subscriber` updates *only* `tracing-subscriber` dependency version (e.g. by running `cargo update -p tracing-subscriber`), it will not also update its `tracing-core` version to one that contains the new method, and `tracing-subscriber` will fail to compile. This is a common occurence with projects using Dependabot, for example. This commit updates `tracing-subscriber`'s minimum `tracing-core` dep to 0.1.28. Once this merges, I'll release 0.3.13 of `tracing-subscriber` and yank 0.3.12.
## Motivation PR #2008 added a new method to the `tracing_core::Subscriber` trait, and modified `tracing-subscriber` to and implement that method. However, that PR did *not* increase the minimum `tracing-core` dependency for the `tracing-subscriber` crate. This means that if a dependent of `tracing-subscriber` updates *only* `tracing-subscriber` dependency version (e.g. by running `cargo update -p tracing-subscriber`), it will not also update its `tracing-core` version to one that contains the new method, and `tracing-subscriber` will fail to compile. This is a common occurrence with projects using Dependabot, for example. ## Solution This commit updates `tracing-subscriber`'s minimum `tracing-core` dep to 0.1.28. Once this merges, I'll release 0.3.13 of `tracing-subscriber` and yank 0.3.12.
…io-rs#2008) ## Motivation Allow filter layers to filter on the contents of events (see tokio-rs#2007). ## Solution This branch adds a new `Subscriber::event_enabled` method, taking an `Event` and returning `bool`. This is called before the `Subscriber::event` method, and if it returns `false`, `Subscriber::event` is not called. For simple subscriber (e.g. not using `Layer`s), the `event_enabled` method is not particulary necessary, as the subscriber can just skip the `event` call. However, this branch also adds a new `Layer::event_enabled` method, with the signature: ```rust fn event_enabled(&self, event: &Event<'_>, ctx: Context<'_, S>) -> bool; ``` This is called before `Layer::on_event`, and if `event_enabled` returns `false`, `on_event` is not called (nor is `Subscriber::event`). This allows filter `Layer`s to filter out an `Event` based on its fields. Closes tokio-rs#2007
# 0.1.28 (June 23, 2022) This release of `tracing-core` adds new `Value` implementations, including one for `String`, to allow recording `&String` as a value without having to call `as_str()` or similar, and for 128-bit integers (`i128` and `u128`). In addition, it adds new methods and trait implementations for `Subscriber`s. ### Added - `Value` implementation for `String` ([tokio-rs#2164]) - `Value` implementation for `u128` and `i28` ([tokio-rs#2166]) - `downcast_ref` and `is` methods for `dyn Subscriber + Sync`, `dyn Subscriber + Send`, and `dyn Subscriber + Send + Sync` ([tokio-rs#2160]) - `Subscriber::event_enabled` method to enable filtering based on `Event` field values ([tokio-rs#2008]) - `Subscriber` implementation for `Box<S: Subscriber + ?Sized>` and `Arc<S: Subscriber + ?Sized>` ([tokio-rs#2161]) Thanks to @jswrenn and @CAD97 for contributing to this release! [tokio-rs#2164]: tokio-rs#2164 [tokio-rs#2166]: tokio-rs#2166 [tokio-rs#2160]: tokio-rs#2160 [tokio-rs#2008]: tokio-rs#2008 [tokio-rs#2161]: tokio-rs#2161
# 0.3.12 (Jun 29, 2022) This release of `tracing-subscriber` adds a new `Layer::event_enabled` method, which allows `Layer`s to filter events *after* their field values are recorded; a `Filter` implementation for `reload::Layer`, to make using `reload` with per-layer filtering more ergonomic, and additional inherent method downcasting APIs for the `Layered` type. In addition, it includes dependency updates, and minor fixes for documentation and feature flagging. ### Added - **layer**: `Layer::event_enabled` method, which can be implemented to filter events based on their field values (tokio-rs#2008) - **reload**: `Filter` implementation for `reload::Layer` (tokio-rs#2159) - **layer**: `Layered::downcast_ref` and `Layered::is` inherent methods (tokio-rs#2160) ### Changed - **parking_lot**: Updated dependency on `parking_lot` to 0.13.0 (tokio-rs#2143) - Replaced `lazy_static` dependency with `once_cell` (tokio-rs#2147) ### Fixed - Don't enable `tracing-core` features by default (tokio-rs#2107) - Several documentation link and typo fixes (tokio-rs#2064, tokio-rs#2068, tokio-rs#2077, tokio-rs#2161, tokio-rs#1088) Thanks to @ben0x539, @jamesmunns, @georgemp, @james7132, @jswrenn, @CAD97, and @guswynn for contributing to this release!
Motivation
Allow filter layers to filter on the contents of events (closes #2007).
Solution
Add
Subscribe::event_enabled(&self, event: &Event<'_>, ctx: Context<'_, C>) -> bool;
. This is called beforeSubscribe::on_event
, and ifevent_enabled
returnsfalse
,on_event
is not called (nor isCollect::event
).Previously, this PR did
Subscribe::on_event
returnsControlFlow
, and theLayered
subscriber only continues to callon_event
onControlFlow::Continue
.This PR exists mainly as a place for discussion of this potential solution. Looking at this diff, and given that most subscriber layers will want to continue and not break the layering, it might make sense to make this a new method which delegates to on_event by default. Since Layered is the primary way of layering two subscribers together, perhaps the footgun of calling
on_event
rather than "filter_on_event
" would not be a big deal in practice?