Skip to content
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: Fix EnvFilter to match targets #368

Closed
wants to merge 27 commits into from

Conversation

LucioFranco
Copy link
Member

When a target and span are provided to filter upon, EnvFilter would
let events that existed outside the target to be not filtered. This
change makes it so that when you provide a target and span, the only
events that are not filtered out are those that exist within both
the target and the span.

Signed-off-by: Lucio Franco luciofranco14@gmail.com

When a target and span are provided to filter upon, `EnvFilter` would
let events that existed outside the target to be not filtered. This
change makes it so that when you provide a target and span, the only
events that are not filtered out are those that exist within both
the target and the span.

Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to propose an alternative implementation that might be more efficient. Rather than checking targets in enabled, what do you think of changing the scope to be a Vec<(Option<String>, LevelFilter)>, and for span directives with a target, pushing both the level and the target to scope, rather than just the level. Then, when we iterate over scope, only test against each level in scope if the target for that level matches the filtered metadata's target, or if the target is None.

If we're going to do this, we would probably want to start representing targets as Arc<String> rather than as Strings, so we don't have to copy the whole string when pushing it to the stack.

@@ -227,7 +227,9 @@ impl<S: Subscriber> Layer<S> for EnvFilter {
.with(|scope| {
for filter in scope.iter() {
if filter >= level {
return true;
if self.dynamics.match_target(metadata) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is implementing the correct behavior. It looks like this will return true only if all the dynamic directives in the filter match the metadata's target?

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right approach! I noticed some places where the behaviour is inconsistent with the rest of the filter code, these should probably be changed.

I also commented on some potential style and refactoring changes, but these are not blockers.

tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/directive.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/directive.rs Outdated Show resolved Hide resolved
@@ -692,6 +710,13 @@ impl SpanMatcher {
.unwrap_or_else(|| self.base_level.clone())
}

pub(crate) fn target(&self) -> Option<Arc<str>> {
self.field_matches
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just put the target field on SpanMatcher?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So not sure what is right here. I went with the first target, which for me will work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if that was unclear — what I was saying is, it doesn't really seem necessary for every SpanMatch to have a target field, when we can just clone the CallsiteMatch's target when constructing a new SpanMatcher, here:

pub(crate) fn to_span_match(&self, attrs: &span::Attributes<'_>) -> SpanMatcher {
let field_matches = self
.field_matches
.iter()
.map(|m| {
let m = m.to_span_match();
attrs.record(&mut m.visitor());
m
})
.collect();
SpanMatcher {
field_matches,
base_level: self.base_level.clone(),
}
}

We could change that function to:

 pub(crate) fn to_span_match(&self, attrs: &span::Attributes<'_>) -> SpanMatcher { 
     let field_matches = ...;
     SpanMatcher { 
         field_matches, 
         base_level: self.base_level.clone(), 
         target: self.target.clone();
     } 
 } 

and remove the target field from SpanMatch.

@LucioFranco LucioFranco requested a review from hawkw October 3, 2019 19:43
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the loop over scope in enabled will still return early if a filter doesn't match, which is incorrect. This needs to be changed.

Beyond that, my other comments are style nits that are not blockers.

Also, I think you need to run rustfmt to get CI to pass?

tracing-subscriber/src/filter/env/directive.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/directive.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/directive.rs Outdated Show resolved Hide resolved
@@ -692,6 +710,13 @@ impl SpanMatcher {
.unwrap_or_else(|| self.base_level.clone())
}

pub(crate) fn target(&self) -> Option<Arc<str>> {
self.field_matches
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if that was unclear — what I was saying is, it doesn't really seem necessary for every SpanMatch to have a target field, when we can just clone the CallsiteMatch's target when constructing a new SpanMatcher, here:

pub(crate) fn to_span_match(&self, attrs: &span::Attributes<'_>) -> SpanMatcher {
let field_matches = self
.field_matches
.iter()
.map(|m| {
let m = m.to_span_match();
attrs.record(&mut m.visitor());
m
})
.collect();
SpanMatcher {
field_matches,
base_level: self.base_level.clone(),
}
}

We could change that function to:

 pub(crate) fn to_span_match(&self, attrs: &span::Attributes<'_>) -> SpanMatcher { 
     let field_matches = ...;
     SpanMatcher { 
         field_matches, 
         base_level: self.base_level.clone(), 
         target: self.target.clone();
     } 
 } 

and remove the target field from SpanMatch.

tracing-subscriber/src/filter/mod.rs Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Oct 3, 2019

Also, it would be great to have documentation for the new behavior. But, since the old behavior was not really well-documented, I'm fine with merging this and adding docs in a follow-up...

if filter >= level {
return true;
if &filter.level >= level {
if filter.target.matches(&target) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurs to me that if we think the nested if is ugly or unclear, this could be simplified to

if &filter.level >= level && filter.target.matches(&target) {
    return true;
}

Happy to merge it regardless, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me do this.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me now, thanks!

@hawkw
Copy link
Member

hawkw commented Oct 3, 2019

Don't forget to add that this will close #367!

Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if we're moving forwards with this behavior (29fedee) we really need to document that, because it's different from the rules described in these docs...

tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
LucioFranco and others added 3 commits October 31, 2019 11:44
Co-Authored-By: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
…nvfilter-target-matching

Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
@LucioFranco
Copy link
Member Author

@hawkw I think I've fixed up the docs per your comments.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think the rules around how targets are matched when multiple span directives match (choosing the first target, rather than selecting the target of the specific directive that was matched) needs to be documented, as I mentioned in #368 (review). That behaviour has the potential to be very surprising, and the documentation doesn't mention it at all.

If we're not able to document this behavior, I think we need to spend some more time implementing the correct behavior instead (probably requiring some more internal refactoring).

tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
/// from the value. Examples, `1`, `\"some_string\"`, etc.
/// - `level` sets a maximum verbosity level accepted by this directive
///
/// The portion of the synatx that is included within the square brackets is `tracing` specific.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put this at the top where we discuss the relationship with env_logger?

tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
/// Example, `[span{field=\"value\"}]=debug`, `[{field}]=trace`, etc.
/// - `value` matches the _output_ of the span's value. If a value is a numeric literal or a bool,
// it will match that value only. Otherwise, it's a regex that matches the `std::fmt::Debug` output
/// from the value. Examples, `1`, `\"some_string\"`, etc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// from the value. Examples, `1`, `\"some_string\"`, etc.
/// from the value. For example: `1`, `\"some_string\"`, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also add an example with a regex here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you provide an example of a good regex for this?

tracing-subscriber/src/filter/env/mod.rs Outdated Show resolved Hide resolved
davidbarsky and others added 4 commits January 29, 2020 09:15
Co-Authored-By: Eliza Weisman <eliza@buoyant.io>
Co-Authored-By: Eliza Weisman <eliza@buoyant.io>
Co-Authored-By: Eliza Weisman <eliza@buoyant.io>
Co-Authored-By: Eliza Weisman <eliza@buoyant.io>
@LucioFranco
Copy link
Member Author

Docs look great thanks @davidbarsky <3

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidbarsky I think the docs changes you've made look great! However, I don't think the comments I made in #368 (review) have been addressed:

I still think the rules around how targets are matched when multiple span directives match (choosing the first target, rather than selecting the target of the specific directive that was matched) needs to be documented, as I mentioned in #368 (review). That behaviour has the potential to be very surprising, and the documentation doesn't mention it at all.

If we're not able to document this behavior, I think we need to spend some more time implementing the correct behavior instead (probably requiring some more internal refactoring).

Again, it's specifically the new behavior introduced in this PR that I think needs to be documented before we merge this change.

Thanks for working on this!

@davidbarsky
Copy link
Member

As discussed offline, I'll close this PR and consider tackling more generalized filtering in #508. The documentation written by Lucio and edited by me is being reviewed as a part of #554.

davidbarsky added a commit that referenced this pull request Jan 31, 2020
This PR is extracted from #368 and introduces additional documentation for
the filter directives in tracing-subscriber.

Signed-off-by: David Barsky <me@davidbarsky.com>
Co-authored-by: Lucio Franco <luciofranco14@gmail.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@LucioFranco LucioFranco deleted the lucio/fix-envfilter-target-matching branch February 24, 2020 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants