-
Notifications
You must be signed in to change notification settings - Fork 691
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: implement per-layer filtering #1523
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.
(initial review, sorry. will add more comments later.)
098fab3
to
ff20065
Compare
whew, okay, this finally has enough docs that i'm marking it as ready...there's room for improvement and expanding on some docs stuff, but i'd like to start the review process now that i'm not failing the "missing rustdoc" lint anymore. |
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 maybe a fourth or fifth of the way through my review, but I'll leave some comments in the meantime.
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.
additional examples changes a la david's suggestions
4c5249b
to
ccdc354
Compare
// If you don't understand this...that's fine, just don't mess with | ||
// 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.
i love this passive-aggressive smiley that says "stay out of my territory"
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 wasn't intended to be passive aggressive, it was supposed to be reassuring! the intended message was "it's okay if this stuff is complicated, it won't be on the test"... ._.
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 think it's hilarious!
…1537) ## Motivation When reviewing #1523, I suggested that the examples use `fmt::Layer` directly to allow readers to copy-and-paste examples into their own code. However, @hawkw pointed out that rustdoc doesn't receive feature flags from Cargo, which means that running tracing-subscriber's tests under `--no-default-features` would fail. ## Solution It turned out that that we're only running `cargo check --no-default-features` for tracing-subscriber. Running `cargo test --no-default-features` resulted in compilation failures, but it seems like nobody noticed. Building on #1532, this branch argues that running tracing-subscriber's tests under `cargo test --lib --tests --no-default-features` is _probably_ fine if we're able to skip the doctests (which are _not_ aware of Cargo's feature flags).
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>
Currently, the `console_subscriber::build()` and `console_subscriber::init()` functions configure an `EnvFilter` that enables the `tokio` and `runtime` targets, plus any targets configured by `RUST_LOG`. This means that the `tokio` spans used by the console will always be enabled at the `TRACE` level for all layers. This includes the `fmt` layer we previously added in `init`. Since this resulted in the `fmt` layer logs being cluttered with a bunch of `tokio=trace` spans and events even when only high-level application logs are enabled by `RUST_LOG`, we removed the `fmt` layer in PR #64. However, tokio-rs/tracing#1523 was recently merged and [released][1], adding support for [per-layer filtering][2] to `tracing-subscriber`. We can now use the per-layer filtering API to selectively enable the `tokio`/`runtime` spans and events at the `TRACE` level _only_ for the `TasksLayer`, and add a filter to the `fmt` layer based on `RUST_LOG`. This allows us to put back the `fmt` layer in `console_subscriber::init`. Closes #76
Currently, the `console_subscriber::build()` and `console_subscriber::init()` functions configure an `EnvFilter` that enables the `tokio` and `runtime` targets, plus any targets configured by `RUST_LOG`. This means that the `tokio` spans used by the console will always be enabled at the `TRACE` level for all layers. This includes the `fmt` layer we previously added in `init`. Since this resulted in the `fmt` layer logs being cluttered with a bunch of `tokio=trace` spans and events even when only high-level application logs are enabled by `RUST_LOG`, we removed the `fmt` layer in PR #64. However, tokio-rs/tracing#1523 was recently merged and [released][1], adding support for [per-layer filtering][2] to `tracing-subscriber`. We can now use the per-layer filtering API to selectively enable the `tokio`/`runtime` spans and events at the `TRACE` level _only_ for the `TasksLayer`, and add a filter to the `fmt` layer based on `RUST_LOG`. This allows us to put back the `fmt` layer in `console_subscriber::init`. Now, if we run the example app (which uses `init`) with a `RUST_LOG` value that enables only the app's logs, we get nice output: ![image](https://user-images.githubusercontent.com/2796466/133124582-608c2b72-db2f-4588-bb75-585312ac8d66.png) However, we can also enable the `console-subscriber` crate's internal logging: ![image](https://user-images.githubusercontent.com/2796466/133124746-7df10f14-cf8c-40ef-96f3-5046039a8577.png) And, we can still enable `tokio=trace` ourselves, if we actually _want_ to see all the task spans and waker events: ![image](https://user-images.githubusercontent.com/2796466/133124851-3e4af25a-06a4-4912-9950-bfbab6dba4c7.png) I also added some `trace!` and `debug!` macros in `examples/app.rs` to demo that `console_subscriber::init()` also enables logging. Closes #76
Thanks so much for landing this! I would vote a huge +1 to making it work with |
The `Layer::on_layer` method on `Layer` was added in PR #1523. PR #1536, which added `Layer` implementations to `Box<dyn Layer<...> + ...>` and `Arc<dyn Layer<...> + ...>`, merged prior to #1523. However, when merging #1523, I didn't think to update the `Layer` impl for `Box`ed and `Arc`ed layers to forward `on_layer` to the inner `Layer`. This means that when a `Layer` is wrapped in an `Arc` or a `Box`, the `on_layer` method never gets called. When per-layer filters are in use, the `on_layer` method is necessary to ensure the filter is registered with the inner subscriber and has a valid ID. This bug means that when per-layer filters are wrapped in a `Box` or `Arc`, they won't be registered, and per-layer filtering breaks. This PR fixes the bug by adding `on_layer` implementations to the `Layer` impls for `Arc`ed and `Box`ed layers. I also added some tests --- thanks to @Noah-Kennedy for the original repro that these were based on (#1563 (comment)). Signed-off-by: Eliza Weisman <eliza@buoyant.io>
The `Layer::on_layer` method on `Layer` was added in PR #1523. PR #1536, which added `Layer` implementations to `Box<dyn Layer<...> + ...>` and `Arc<dyn Layer<...> + ...>`, merged prior to #1523. However, when merging #1523, I didn't think to update the `Layer` impl for `Box`ed and `Arc`ed layers to forward `on_layer` to the inner `Layer`. This means that when a `Layer` is wrapped in an `Arc` or a `Box`, the `on_layer` method never gets called. When per-layer filters are in use, the `on_layer` method is necessary to ensure the filter is registered with the inner subscriber and has a valid ID. This bug means that when per-layer filters are wrapped in a `Box` or `Arc`, they won't be registered, and per-layer filtering breaks. This PR fixes the bug by adding `on_layer` implementations to the `Layer` impls for `Arc`ed and `Box`ed layers. I also added some tests --- thanks to @Noah-Kennedy for the original repro that these were based on (#1563 (comment)). I also added a nicer debug assertion to `Filtered` for cases where `Layer` impls don't call `on_layer`, so that this fails less confusingly in the future. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation Currently, filtering with `Layer`s is always _global_. If a `Layer` performs filtering, it will disable a span or event for _all_ layers that compose the current subscriber. In some cases, however, it is often desirable for individual layers to see different spans or events than the rest of the `Layer` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Layer`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Layer::on_event` and `Layer::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-layer filtering implementation would satisfy the following _desiderata_: * If a per-layer filter disables a span, it shouldn't be present _for the layer that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-layer filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular layers. This means that per-layer filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-layer filters shouldn't interfere with non-filtered `Layer`s. If a subscriber contains layers without any filters, as well as layers with per-layer filters, the non-filtered `Layer`s should behave exactly as they would without any per-layer filters present. * Similarly, per-layer filters shouldn't interfere with global filtering. If a `Layer` in a stack is performing global filtering (e.g. the current filtering behavior), per-layer filters should also be effected by the global filter. * Per-layer filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-layer-filtered layers_. * Per-layer filters should be _tree-shaped_. If a `Subscriber` consists of multiple layers that have been `Layered` together to form new `Layer`s, and some of the `Layered` layers have per-layer filters, those per-layer filters should effect all layers in that subtree. Similarly, if `Layer`s in a per-layer filtered subtree have their _own_ per-layer filters, those layers should be effected by the union of their own filters and any per-layer filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-layer filtering correctly is somewhat more complex than simply skipping events and spans in a `Layer`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-layer filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-layer filtering feature _doesn't_ interfere with the global filtering behavior that layers currently have when not using the new per-layer filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-layer filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Layer`. When `enabled` is called on a `Filtered` layer, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-layer filter disables a span/event, and there are no non-per-layer-filtered layers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` layer's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped layer if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_layer` method was added to the `Layer` trait, which allows running a callback when adding a `Layer` to a `Subscriber`. This allows mutating both the `Layer` and the `Subscriber`, since they are both passed by value when layering a `Layer` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` layer is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_layer` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` layer. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-layer filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` layer in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 layers, let alone 64 separate per-layer filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` layers. When `Filtered` layers are nested using the `Layered`, the `Context` _combines_ the filter ID of the inner and outer layers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `layer` module. Since we were adding more code to the `Layer` module (the `Filter` trait), the `layer.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Layered` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-layer filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-layer filtering, I added a new `Layer` implementation that does the same thing, but implements `Layer` rather than `Subscriber`. This allows testing per-layer filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-layer filters. This is in order to avoid stabilizing a lot of the per-layer filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host layers with per-layer filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-layer filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Layer` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-layer filter evaluations, so that we don't need to always use `enabled` when there are several per-layer filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
; This is the 1st commit message: subscriber: implement per-subscriber filtering (#1523) ## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io> ; The commit message #2 will be skipped: ; fixup: start making changes for filter ; The commit message #3 will be skipped: ; fixup: move stuff to `subscribe` folder from `layer ; The commit message #4 will be skipped: ; fixup: additional compilation errors
## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation Currently, filtering with `Subscribe`s is always _global_. If a `Subscribe` performs filtering, it will disable a span or event for _all_ subscribers that compose the current subscriber. In some cases, however, it is often desirable for individual subscribers to see different spans or events than the rest of the `Subscribe` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like #302, #597, and #1348, all indicate that there is significant demand for the ability to add filters to individual `Subscribe`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Subscribe::on_event` and `Subscribe::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-subscriber filtering implementation would satisfy the following _desiderata_: * If a per-subscriber filter disables a span, it shouldn't be present _for the subscriber that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-subscriber filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular subscribers. This means that per-subscriber filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-subscriber filters shouldn't interfere with non-filtered `Subscribe`s. If a subscriber contains subscribers without any filters, as well as subscribers with per-subscriber filters, the non-filtered `Subscribe`s should behave exactly as they would without any per-subscriber filters present. * Similarly, per-subscriber filters shouldn't interfere with global filtering. If a `Subscribe` in a stack is performing global filtering (e.g. the current filtering behavior), per-subscriber filters should also be effected by the global filter. * Per-subscriber filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-subscriber-filtered subscribers_. * Per-subscriber filters should be _tree-shaped_. If a `Subscriber` consists of multiple subscribers that have been `Subscribeed` together to form new `Subscribe`s, and some of the `Subscribeed` subscribers have per-subscriber filters, those per-subscriber filters should effect all subscribers in that subtree. Similarly, if `Subscribe`s in a per-subscriber filtered subtree have their _own_ per-subscriber filters, those subscribers should be effected by the union of their own filters and any per-subscriber filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-subscriber filtering correctly is somewhat more complex than simply skipping events and spans in a `Subscribe`'s `on_event` and `new_span` callbacks. ## Solution This branch has a basic working implementation of per-subscriber filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-subscriber filtering feature _doesn't_ interfere with the global filtering behavior that subscribers currently have when not using the new per-subscriber filtering APIs, so existing configurations should all behave exactly as they do now. ### Implementation The new per-subscriber filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Subscribe`. When `enabled` is called on a `Filtered` subscriber, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-subscriber filter disables a span/event, and there are no non-per-subscriber-filtered subscribers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` subscriber's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped subscriber if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_subscriber` method was added to the `Subscribe` trait, which allows running a callback when adding a `Subscribe` to a `Subscriber`. This allows mutating both the `Subscribe` and the `Subscriber`, since they are both passed by value when subscribering a `Subscribe` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` subscriber is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_subscriber` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` subscriber. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-subscriber filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` subscriber in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 subscribers, let alone 64 separate per-subscriber filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` subscribers. When `Filtered` subscribers are nested using the `Subscribeed`, the `Context` _combines_ the filter ID of the inner and outer subscribers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. ### Notes Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `subscriber` module. Since we were adding more code to the `Subscribe` module (the `Filter` trait), the `subscriber.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Subscribeed` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-subscriber filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-subscriber filtering, I added a new `Subscribe` implementation that does the same thing, but implements `Subscribe` rather than `Subscriber`. This allows testing per-subscriber filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. ## Future Work There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-subscriber filters. This is in order to avoid stabilizing a lot of the per-subscriber filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host subscribers with per-subscriber filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-subscriber filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Subscribe` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-subscriber filter evaluations, so that we don't need to always use `enabled` when there are several per-subscriber filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes #302 Closes #597 Closes #1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Currently, filtering with `Layer`s is always _global_. If a `Layer` performs filtering, it will disable a span or event for _all_ layers that compose the current subscriber. In some cases, however, it is often desirable for individual layers to see different spans or events than the rest of the `Layer` stack. Issues in other projects, such as tokio-rs/console#64 and tokio-rs/console#76, linkerd/linkerd2-proxy#601, influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681, jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like the ability to add filters to individual `Layer`s. Unfortunately, doing this nicely is somewhat complicated. Although a naive implementation that simply skips events/spans in `Layer::on_event` and `Layer::new_span` based on some filter is relatively simple, this wouldn't really be an ideal solution, for a number of reasons. A proper per-layer filtering implementation would satisfy the following _desiderata_: * If a per-layer filter disables a span, it shouldn't be present _for the layer that filter is attached to_ when iterating over span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc), or when looking up a span's parents. * When _all_ per-layer filters disable a span or event, it should be completely disabled, rather than simply skipped by those particular layers. This means that per-layer filters should participate in `enabled`, as well as being able to skip spans and events in `new_span` and `on_event`. * Per-layer filters shouldn't interfere with non-filtered `Layer`s. If a subscriber contains layers without any filters, as well as layers with per-layer filters, the non-filtered `Layer`s should behave exactly as they would without any per-layer filters present. * Similarly, per-layer filters shouldn't interfere with global filtering. If a `Layer` in a stack is performing global filtering (e.g. the current filtering behavior), per-layer filters should also be effected by the global filter. * Per-layer filters _should_ be able to participate in `Interest` caching, _but only when doing so doesn't interfere with non-per-layer-filtered layers_. * Per-layer filters should be _tree-shaped_. If a `Subscriber` consists of multiple layers that have been `Layered` together to form new `Layer`s, and some of the `Layered` layers have per-layer filters, those per-layer filters should effect all layers in that subtree. Similarly, if `Layer`s in a per-layer filtered subtree have their _own_ per-layer filters, those layers should be effected by the union of their own filters and any per-layer filters that wrap them at higher levels in the tree. Meeting all these requirements means that implementing per-layer filtering correctly is somewhat more complex than simply skipping events and spans in a `Layer`'s `on_event` and `new_span` callbacks. This branch has a basic working implementation of per-layer filtering for `tracing-subscriber` v0.2. It should be possible to add this in a point release without a breaking change --- in particular, the new per-layer filtering feature _doesn't_ interfere with the global filtering behavior that layers currently have when not using the new per-layer filtering APIs, so existing configurations should all behave exactly as they do now. The new per-layer filtering API consists of a `Filter` trait that defines a filtering strategy and a `Filtered` type that combines a `Filter` with a `Layer`. When `enabled` is called on a `Filtered` layer, it checks the metadata against the `Filter` and stores whether that filter enabled the span/event in a thread-local cell. If every per-layer filter disables a span/event, and there are no non-per-layer-filtered layers in use, the `Registry`'s `enabled` will return `false`. Otherwise, when the span/event is recorded, each `Filtered` layer's `on_event` and `new_span` method checks with the `Registry` to see whether _it_ enabled or disabled that span/event, and skips passing it to the wrapped layer if it was disabled. When a span is recorded, the `Registry` which filters disabled it, so that they can skip it when looking up spans in other callbacks. A new `on_layer` method was added to the `Layer` trait, which allows running a callback when adding a `Layer` to a `Subscriber`. This allows mutating both the `Layer` and the `Subscriber`, since they are both passed by value when layering a `Layer` onto a `Subscriber`, and a new `register_filter` method was added to `LookupSpan`, which returns a `FilterId` type. When a `Filtered` layer is added to a subscriber that implements `LookupSpan`, it calls `register_filter` in its `on_layer` hook to generate a unique ID for its filter. `FilterId` can be used to look up whether a span or event was enabled by the `Filtered` layer. Internally, the filter results are stored in a simple bitset to reduce the performance overhead of adding per-layer filtering as much as possible. The `FilterId` type is actually a bitmask that's used to set the bit corresponding to a `Filtered` layer in that bitmap when it disables a span or event, and check whether the bit is set when querying if a span or event was disabled. Setting a bit in the bitset and testing whether its set is extremely efficient --- it's just a couple of bitwise ops. Additionally, the "filter map" that tracks which filters disabled a span or event is just a single `u64` --- we don't need to allocate a costly `HashSet` or similar every time we want to filter an event. This *does* mean that we are limited to 64 unique filters per `Registry`...however, I would be _very_ surprised if anyone _ever_ builds a subscriber with 64 layers, let alone 64 separate per-layer filters. Additionally, the `Context` and `SpanRef` types now also potentially carry `FilterId`s, so that they can filter span lookups and span traversals for `Filtered` layers. When `Filtered` layers are nested using the `Layered`, the `Context` _combines_ the filter ID of the inner and outer layers so that spans can be skipped if they were disabled by either. Finally, an implementation of the `Filter` trait was added for `LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added to produce a `Filter` implementation from a function pointer or closure. Some other miscellaneous factors to consider when reviewing this change include: * I did a bit of unrelated refactoring in the `layer` module. Since we were adding more code to the `Layer` module (the `Filter` trait), the `layer.rs` file got significantly longer and was becoming somewhat hard to navigate. Therefore, I split out the `Context` and `Layered` types into their own files. Most of the code in those files is the same as it was before, although some of it was changed in order to support per-layer filtering. Apologies in advance to reviewers for this... * In order to test this change, I added some new test-support code in `tracing-subscriber`. The `tracing` test support code provides a `Subscriber` implementation that can be configured to expect a particular set of spans and events, and assert that they occurred as expected. In order to test per-layer filtering, I added a new `Layer` implementation that does the same thing, but implements `Layer` rather than `Subscriber`. This allows testing per-layer filter behavior in the same way we test global filter behavior. Reviewers should feel free to just skim the test the test support changes, or skip reviewing them entirely. There is a bunch of additional stuff we can do once this change lands. In order to limit the size of this PR, I didn't make these changes yet, but once this merges, we will want to consider some important follow up changes: * [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of serving as a root subscriber for per-layer filters. This is in order to avoid stabilizing a lot of the per-layer filtering design as public API without taking the time to ensure there are no serious issues. In the future, we will want to add new APIs to allow users to implement their own root `Subscriber`s that can host layers with per-layer filtering. * [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus cannot currently be used as a per-layer filter. We should add an implementation of `Filter` for `EnvFilter`. * [ ] The new `FilterFn` and `DynFilterFn` types could conceivably _also_ be used as global filters. We could consider adding `Layer` implementations to allow them to be used for global filtering. * [ ] We could consider adding new `Filter` implementations for performing common filtering predicates. For example, "only enable spans/events with a particular target or a set of targets", "only enable spans", "only enable events", etc. Then, we could add a combinator API to combine these predicates to build new filters. * [ ] It would be nice if the `Interest` system could be used to cache the result of multiple individual per-layer filter evaluations, so that we don't need to always use `enabled` when there are several per-layer filters that disagree on whether or not they want a given span/event. There are a lot of unused bits in `Interest` that could potentially be used for this purpose. However, this would require new API changes in `tracing-core`, and is therefore out of scope for this PR. Closes tokio-rs#302 Closes tokio-rs#597 Closes tokio-rs#1348 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
# 0.2.21 (September 12, 2021) This release introduces the [`Filter`] trait, a new API for [per-layer filtering][plf]. This allows controlling which spans and events are recorded by various layers individually, rather than globally. In addition, it adds a new [`Targets`] filter, which provides a lighter-weight version of the filtering provided by [`EnvFilter`], as well as other smaller API improvements and fixes. ### Deprecated - **registry**: `SpanRef::parent_id`, which cannot properly support per-layer filtering. Use `.parent().map(SpanRef::id)` instead. ([tokio-rs#1523]) ### Fixed - **layer** `Context` methods that are provided when the `Subscriber` implements `LookupSpan` no longer require the "registry" feature flag ([tokio-rs#1525]) - **layer** `fmt::Debug` implementation for `Layered` no longer requires the `S` type parameter to implement `Debug` ([tokio-rs#1528]) ### Added - **registry**: `Filter` trait, `Filtered` type, `Layer::with_filter` method, and other APIs for per-layer filtering ([tokio-rs#1523]) - **filter**: `FilterFn` and `DynFilterFn` types that implement global (`Layer`) and per-layer (`Filter`) filtering for closures and function pointers ([tokio-rs#1523]) - **filter**: `Targets` filter, which implements a lighter-weight form of `EnvFilter`-like filtering ([tokio-rs#1550]) - **env-filter**: Added support for filtering on floating-point values ([tokio-rs#1507]) - **layer**: `Layer::on_layer` callback, called when layering the `Layer` onto a `Subscriber` ([tokio-rs#1523]) - **layer**: `Layer` implementations for `Box<L>` and `Arc<L>` where `L: Layer` ([tokio-rs#1536]) - **layer**: `Layer` implementations for `Box<dyn Layer>` and `Arc<dyn Layer>` ([tokio-rs#1536]) - A number of small documentation fixes and improvements ([tokio-rs#1553], [tokio-rs#1544], [tokio-rs#1539], [tokio-rs#1524]) Special thanks to new contributors @jsgf and @maxburke for contributing to this release! [`Filter`]: https://docs.rs/tracing-subscriber/0.2.21/tracing_subscriber/layer/trait.Filter.html [plf]: https://docs.rs/tracing-subscriber/0.2.21/tracing_subscriber/layer/index.html#per-layer-filtering [`Targets`]: https://docs.rs/tracing-subscriber/0.2.21/tracing_subscriber/filter/struct.Targets.html [`EnvFilter`]: https://docs.rs/tracing-subscriber/0.2.21/tracing_subscriber/filter/struct.EnvFilter.html [tokio-rs#1507]: tokio-rs#1507 [tokio-rs#1523]: tokio-rs#1523 [tokio-rs#1524]: tokio-rs#1524 [tokio-rs#1525]: tokio-rs#1525 [tokio-rs#1528]: tokio-rs#1528 [tokio-rs#1539]: tokio-rs#1539 [tokio-rs#1544]: tokio-rs#1544 [tokio-rs#1550]: tokio-rs#1550 [tokio-rs#1553]: tokio-rs#1553
…-rs#1576) The `Layer::on_layer` method on `Layer` was added in PR tokio-rs#1523. PR tokio-rs#1536, which added `Layer` implementations to `Box<dyn Layer<...> + ...>` and `Arc<dyn Layer<...> + ...>`, merged prior to tokio-rs#1523. However, when merging tokio-rs#1523, I didn't think to update the `Layer` impl for `Box`ed and `Arc`ed layers to forward `on_layer` to the inner `Layer`. This means that when a `Layer` is wrapped in an `Arc` or a `Box`, the `on_layer` method never gets called. When per-layer filters are in use, the `on_layer` method is necessary to ensure the filter is registered with the inner subscriber and has a valid ID. This bug means that when per-layer filters are wrapped in a `Box` or `Arc`, they won't be registered, and per-layer filtering breaks. This PR fixes the bug by adding `on_layer` implementations to the `Layer` impls for `Arc`ed and `Box`ed layers. I also added some tests --- thanks to @Noah-Kennedy for the original repro that these were based on (tokio-rs#1563 (comment)). I also added a nicer debug assertion to `Filtered` for cases where `Layer` impls don't call `on_layer`, so that this fails less confusingly in the future. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Motivation
Currently, filtering with
Layer
s is always global. If aLayer
performs filtering, it will disable a span or event for all layers
that compose the current subscriber. In some cases, however, it is often
desirable for individual layers to see different spans or events than
the rest of the
Layer
stack.Issues in other projects, such as tokio-rs/console#64 and
tokio-rs/console#76, linkerd/linkerd2-proxy#601,
influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681,
jackwaudby/spaghetti#86, etc; as well as
tracing
feature requests like#302, #597, and #1348, all indicate that there is significant demand for
the ability to add filters to individual
Layer
s.Unfortunately, doing this nicely is somewhat complicated. Although a
naive implementation that simply skips events/spans in
Layer::on_event
and
Layer::new_span
based on some filter is relatively simple, thiswouldn't really be an ideal solution, for a number of reasons. A proper
per-layer filtering implementation would satisfy the following
desiderata:
the layer that filter is attached to when iterating over span
contexts (such as
Context::event_scope
,SpanRef::scope
, etc), orwhen looking up a span's parents.
completely disabled, rather than simply skipped by those particular
layers. This means that per-layer filters should participate in
enabled
, as well as being able to skip spans and events innew_span
andon_event
.Layer
s.If a subscriber contains layers without any filters, as well as
layers with per-layer filters, the non-filtered
Layer
s shouldbehave exactly as they would without any per-layer filters present.
filtering. If a
Layer
in a stack is performing global filtering(e.g. the current filtering behavior), per-layer filters should also
be effected by the global filter.
Interest
caching, but only when doing so doesn't interfere with
non-per-layer-filtered layers.
Subscriber
consistsof multiple layers that have been
Layered
together to form newLayer
s, and some of theLayered
layers have per-layer filters,those per-layer filters should effect all layers in that subtree.
Similarly, if
Layer
s in a per-layer filtered subtree have theirown per-layer filters, those layers should be effected by the union
of their own filters and any per-layer filters that wrap them at
higher levels in the tree.
Meeting all these requirements means that implementing per-layer
filtering correctly is somewhat more complex than simply skipping
events and spans in a
Layer
'son_event
andnew_span
callbacks.Solution
This branch has a basic working implementation of per-layer filtering
for
tracing-subscriber
v0.2. It should be possible to add this in apoint release without a breaking change --- in particular, the new
per-layer filtering feature doesn't interfere with the global
filtering behavior that layers currently have when not using the new
per-layer filtering APIs, so existing configurations should all behave
exactly as they do now.
Implementation
The new per-layer filtering API consists of a
Filter
trait thatdefines a filtering strategy and a
Filtered
type that combines aFilter
with aLayer
. Whenenabled
is called on aFiltered
layer,it checks the metadata against the
Filter
and stores whether thatfilter enabled the span/event in a thread-local cell. If every per-layer
filter disables a span/event, and there are no non-per-layer-filtered
layers in use, the
Registry
'senabled
will returnfalse
.Otherwise, when the span/event is recorded, each
Filtered
layer'son_event
andnew_span
method checks with theRegistry
to seewhether it enabled or disabled that span/event, and skips passing it
to the wrapped layer if it was disabled. When a span is recorded, the
Registry
which filters disabled it, so that they can skip it whenlooking up spans in other callbacks.
A new
on_layer
method was added to theLayer
trait, which allowsrunning a callback when adding a
Layer
to aSubscriber
. This allowsmutating both the
Layer
and theSubscriber
, since they are bothpassed by value when layering a
Layer
onto aSubscriber
, and a newregister_filter
method was added toLookupSpan
, which returns aFilterId
type. When aFiltered
layer is added to a subscriber thatimplements
LookupSpan
, it callsregister_filter
in itson_layer
hook to generate a unique ID for its filter.
FilterId
can be used tolook up whether a span or event was enabled by the
Filtered
layer.Internally, the filter results are stored in a simple bitset to reduce
the performance overhead of adding per-layer filtering as much as
possible. The
FilterId
type is actually a bitmask that's used to setthe bit corresponding to a
Filtered
layer in that bitmap when itdisables a span or event, and check whether the bit is set when querying
if a span or event was disabled. Setting a bit in the bitset and testing
whether its set is extremely efficient --- it's just a couple of bitwise
ops. Additionally, the "filter map" that tracks which filters disabled a
span or event is just a single
u64
--- we don't need to allocate acostly
HashSet
or similar every time we want to filter an event. Thisdoes mean that we are limited to 64 unique filters per
Registry
...however, I would be very surprised if anyone everbuilds a subscriber with 64 layers, let alone 64 separate per-layer
filters.
Additionally, the
Context
andSpanRef
types now also potentiallycarry
FilterId
s, so that they can filter span lookups and spantraversals for
Filtered
layers. WhenFiltered
layers are nestedusing the
Layered
, theContext
combines the filter ID of the innerand outer layers so that spans can be skipped if they were disabled by
either.
Finally, an implementation of the
Filter
trait was added forLevelFilter
, and newFilterFn
andDynFilterFn
structs were addedto produce a
Filter
implementation from a function pointer or closure.Notes
Some other miscellaneous factors to consider when reviewing this change
include:
layer
module. Since wewere adding more code to the
Layer
module (theFilter
trait), thelayer.rs
file got significantly longer and was becoming somewhathard to navigate. Therefore, I split out the
Context
andLayered
types into their own files. Most of the code in those files is the
same as it was before, although some of it was changed in order to
support per-layer filtering. Apologies in advance to reviewers for
this...
tracing-subscriber
. Thetracing
test support code provides aSubscriber
implementation that can be configured to expect aparticular set of spans and events, and assert that they occurred as
expected. In order to test per-layer filtering, I added a new
Layer
implementation that does the same thing, but implements
Layer
rather than
Subscriber
. This allows testing per-layer filterbehavior in the same way we test global filter behavior. Reviewers
should feel free to just skim the test the test support changes, or
skip reviewing them entirely.
Future Work
There is a bunch of additional stuff we can do once this change lands.
In order to limit the size of this PR, I didn't make these changes yet,
but once this merges, we will want to consider some important follow
up changes:
tracing-subscriber
'sRegistry
is capable ofserving as a root subscriber for per-layer filters. This is in
order to avoid stabilizing a lot of the per-layer filtering
design as public API without taking the time to ensure there are
no serious issues. In the future, we will want to add new APIs to
allow users to implement their own root
Subscriber
s that canhost layers with per-layer filtering.
EnvFilter
type doesn't implement theFilter
trait and thuscannot currently be used as a per-layer filter. We should add an
implementation of
Filter
forEnvFilter
.FilterFn
andDynFilterFn
types could conceivably alsobe used as global filters. We could consider adding
Layer
implementations to allow them to be used for global filtering.
Filter
implementations for performingcommon filtering predicates. For example, "only enable spans/events
with a particular target or a set of targets", "only enable spans",
"only enable events", etc. Then, we could add a combinator API to
combine these predicates to build new filters.
Interest
system could be used to cachethe result of multiple individual per-layer filter evaluations, so
that we don't need to always use
enabled
when there are severalper-layer filters that disagree on whether or not they want a
given span/event. There are a lot of unused bits in
Interest
that could potentially be used for this purpose. However, this
would require new API changes in
tracing-core
, and is thereforeout of scope for this PR.
Closes #302
Closes #597
Closes #1348