-
Notifications
You must be signed in to change notification settings - Fork 726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce Layer Filtering #508
Conversation
S: Subscriber + 'static, | ||
{ | ||
/// Filter on a specific span or event's metadata. | ||
fn filter(&self, metadata: &Metadata<'_>, ctx: &Context<'_, S>) -> bool; |
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 this also needs a register_callsite
-esque hook (maybe call it filter_static
? or something) so that filters can indicate that they never want to see something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that the interest level for each layer can be cached? That makes sense. I think that will be the primary interface for Filter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that the interest level for each layer can be cached? That makes sense. I think that will be the primary interface for
Filter
.
so that the sum total interest can be cached
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.
if all filters will never be interested in a trace, it needs a cached never
interest.
if any are sometimes
interested, it probably should always get a cached sometimes
if all filters are always
interested, it needs a cached always
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.
there may be some optimizations to quick-disable something when we know a given filter is the only "sometimes", but that seems more complex. i'm mostly concerned about all always
and all never
getting cached always
and never
interests, respectively...
tracing-subscriber/src/layer.rs
Outdated
L: Layer<S>, | ||
S: Subscriber + 'static, | ||
F: Filter<L, S> + 'static, | ||
{ |
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 this needs to implement the enabled
and register_callsite
layer fns. I think we will also want to change the rules for how layers handle filters — I'd really like to get it so that if all the per-layer filters disable something, the call to enabled
will return false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this would require thinking a little about interest evaluation orders, which I have not done.
As for: if metadata.level().cmp(&self.level) == self.ordering {
true
} else {
false
} ...I've replaced that with a |
@hawkw and I chatted about how to handle a few bugs that cropped up. Here's the approach that seemed to be the most composable (to me):
Some of this work was on this branch, if I recall correctly: https://github.com/tokio-rs/tracing/commits/eliza/filter-layers-extension. Additionally, instead of a Hashset, we were considering using a bitset to avoid hashes + enable really fast lookups. However, this does mean there is a maximum number of filters, but the numbers (512 or 1024) is so high, I don't expect any user of S: Subscriber + for<'span> LookupSpan<'span> + Filter We also discussed an approach where a filter would be passed into every layer, meaning that layers would need to opt-in to being filtered. I don't think we opted for this approach. |
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>
5521955
to
0e2aef8
Compare
There are several noteable changes in this commit that mean that logging will behave differently to how it did previously. - There are now no separate filter levels for terminal and file output. This is a feature that is lacking in `tracing` but is set to be resolved by tokio-rs/tracing#508. At which point we MAY include it. - The default log level was previously `WARN` - it is now `INFO`.
Is there any chance that this is going to be merged soon? |
Obsoleted by #1523. |
Resolves #302.
Currently, spans and events can only be enabled/disable on a global subscriber basis. This PR adds per-layer filtering in
tracing-subscriber
, which means that layers can conditionally filter spans and events without impacting others layers. In practice, this enables layers to conditionally filter on metadata like source and levels and forward those spans and events to different outputs sinks. This also allows for the creation of sampling or rate-limiting filters.With this PR, customers will be able to write something like:
Still to do:
Filtered
.