subscriber: Fix `EnvFilter` to match targets #368

LucioFranco · 2019-09-27T18:57:27Z

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 [email protected]

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 <[email protected]>

hawkw

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.

hawkw · 2019-10-01T21:04:38Z

tracing-subscriber/src/filter/env/mod.rs

@@ -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) {


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?

…-target-matching

hawkw

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

tracing-subscriber/src/filter/env/directive.rs

hawkw · 2019-10-03T17:27:44Z

tracing-subscriber/src/filter/env/directive.rs

@@ -692,6 +710,13 @@ impl SpanMatcher {
            .unwrap_or_else(|| self.base_level.clone())
    }

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


Why not just put the target field on SpanMatcher?

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

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:

tracing/tracing-subscriber/src/filter/env/directive.rs

Lines 673 to 687 in 019d61e

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.

hawkw

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

hawkw · 2019-10-03T19:54:59Z

tracing-subscriber/src/filter/env/directive.rs

@@ -692,6 +710,13 @@ impl SpanMatcher {
            .unwrap_or_else(|| self.base_level.clone())
    }

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


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:

tracing/tracing-subscriber/src/filter/env/directive.rs

Lines 673 to 687 in 019d61e

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

tracing-subscriber/src/filter/env/mod.rs

hawkw · 2019-10-03T20:03:44Z

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...

hawkw · 2019-10-03T21:16:10Z

tracing-subscriber/src/filter/env/mod.rs

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


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.

Let me do this.

hawkw

This looks good to me now, thanks!

hawkw · 2019-10-03T21:31:55Z

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

Signed-off-by: Lucio Franco <[email protected]>

hawkw

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

Co-Authored-By: Eliza Weisman <[email protected]>

Signed-off-by: Lucio Franco <[email protected]>

…nvfilter-target-matching Signed-off-by: Lucio Franco <[email protected]>

LucioFranco · 2019-10-31T16:53:44Z

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

hawkw

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

hawkw · 2019-10-31T20:29:16Z

tracing-subscriber/src/filter/env/mod.rs

+/// 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.


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

tracing-subscriber/src/filter/env/mod.rs

hawkw · 2019-10-31T20:32:36Z

tracing-subscriber/src/filter/env/mod.rs

+/// 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.


Suggested change

/// from the value. Examples, `1`, `\"some_string\"`, etc.

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

can we also add an example with a regex here?

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

tracing-subscriber/src/filter/env/mod.rs

Co-Authored-By: Eliza Weisman <[email protected]>

…-target-matching

LucioFranco · 2020-01-30T15:47:57Z

Docs look great thanks @davidbarsky <3

hawkw

@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 · 2020-01-30T22:17:43Z

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.

This PR is extracted from #368 and introduces additional documentation for the filter directives in tracing-subscriber. Signed-off-by: David Barsky <[email protected]> Co-authored-by: Lucio Franco <[email protected]> Co-authored-by: Eliza Weisman <[email protected]>

LucioFranco requested a review from hawkw September 27, 2019 18:57

hawkw reviewed Oct 1, 2019

View reviewed changes

LucioFranco added 2 commits October 3, 2019 13:07

Pass target within scope

18a47d2

Merge remote-tracking branch 'origin/master' into lucio/fix-envfilter…

3fa215f

…-target-matching

hawkw requested changes Oct 3, 2019

View reviewed changes

Add newtypes and use Arc<str>

019d61e

LucioFranco requested a review from hawkw October 3, 2019 19:43

hawkw requested changes Oct 3, 2019

View reviewed changes

LucioFranco added 3 commits October 3, 2019 16:53

Clean up usage of TargetFilter

e7359ce

More targetfilter usage and fix scope loop

92aa7cc

Remove commented code

055c4c2

hawkw reviewed Oct 3, 2019

View reviewed changes

hawkw approved these changes Oct 3, 2019

View reviewed changes

Clean up if statement

4e46aac

Fix span filtering to pick first target

29fedee

LucioFranco mentioned this pull request Oct 4, 2019

feat(observability): Add rate limited debug messages vectordotdev/vector#971

Merged

Add docs and a test

ef98f4b

Signed-off-by: Lucio Franco <[email protected]>

hawkw requested changes Oct 30, 2019

View reviewed changes

LucioFranco and others added 3 commits October 31, 2019 11:44

Apply suggestions from code review

9166bfe

Co-Authored-By: Eliza Weisman <[email protected]>

Improve doc comments

0783a26

Signed-off-by: Lucio Franco <[email protected]>

Merge branch 'master' of github.com:tokio-rs/tracing into lucio/fix-e…

4841632

…nvfilter-target-matching Signed-off-by: Lucio Franco <[email protected]>

hawkw requested changes Oct 31, 2019

View reviewed changes

davidbarsky and others added 4 commits January 29, 2020 09:15

Update tracing-subscriber/src/filter/env/mod.rs

1d3f086

Co-Authored-By: Eliza Weisman <[email protected]>

Update tracing-subscriber/src/filter/env/mod.rs

ca309d8

Co-Authored-By: Eliza Weisman <[email protected]>

Update tracing-subscriber/src/filter/env/mod.rs

cee8294

Co-Authored-By: Eliza Weisman <[email protected]>

Update tracing-subscriber/src/filter/env/mod.rs

ee84a04

Co-Authored-By: Eliza Weisman <[email protected]>

davidbarsky and others added 9 commits January 29, 2020 09:15

Update tracing-subscriber/src/filter/env/mod.rs

5088573

Co-Authored-By: Eliza Weisman <[email protected]>

Update tracing-subscriber/src/filter/env/mod.rs

c8d57de

Co-Authored-By: Eliza Weisman <[email protected]>

Update tracing-subscriber/src/filter/env/mod.rs

bc0fec8

Co-Authored-By: Eliza Weisman <[email protected]>

Update tracing-subscriber/src/filter/env/mod.rs

a851b83

Co-Authored-By: Eliza Weisman <[email protected]>

Merge remote-tracking branch 'origin/master' into lucio/fix-envfilter…

235d7f8

…-target-matching

merge from master

b8a47a9

re-introduce TargetFilter

4fb9187

add link to Layer.

0303251

Rewrite docs

0bd3504

davidbarsky requested a review from hawkw January 29, 2020 18:39

fix docs tests (maybe)

6dea29e

hawkw requested changes Jan 30, 2020

View reviewed changes

davidbarsky mentioned this pull request Jan 30, 2020

docs: document filter directives #554

Merged

davidbarsky closed this Jan 30, 2020

LucioFranco deleted the lucio/fix-envfilter-target-matching branch February 24, 2020 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subscriber: Fix `EnvFilter` to match targets #368

subscriber: Fix `EnvFilter` to match targets #368

LucioFranco commented Sep 27, 2019

hawkw left a comment

hawkw Oct 1, 2019

hawkw left a comment

hawkw Oct 3, 2019

LucioFranco Oct 3, 2019

hawkw Oct 3, 2019

hawkw left a comment

hawkw Oct 3, 2019

hawkw commented Oct 3, 2019

hawkw Oct 3, 2019

LucioFranco Oct 3, 2019

hawkw left a comment

hawkw commented Oct 3, 2019

hawkw left a comment

LucioFranco commented Oct 31, 2019

hawkw left a comment

hawkw Oct 31, 2019

hawkw Oct 31, 2019

hawkw Oct 31, 2019

davidbarsky Jan 30, 2020

LucioFranco commented Jan 30, 2020

hawkw left a comment

davidbarsky commented Jan 30, 2020

	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(),
	}
	}

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

subscriber: Fix EnvFilter to match targets #368

subscriber: Fix EnvFilter to match targets #368

Conversation

LucioFranco commented Sep 27, 2019

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw commented Oct 3, 2019

hawkw left a comment

Choose a reason for hiding this comment

LucioFranco commented Oct 31, 2019

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucioFranco commented Jan 30, 2020

hawkw left a comment

Choose a reason for hiding this comment

davidbarsky commented Jan 30, 2020

subscriber: Fix `EnvFilter` to match targets #368

subscriber: Fix `EnvFilter` to match targets #368