subscriber: add `Targets` filter, a lighter-weight `EnvFilter` #1550

hawkw · 2021-09-11T00:51:32Z

This branch adds a new Targets filter to tracing_subscriber. The
Targets filter is very similar to EnvFilter, but it only consists
of filtering directives consisting of a target and level. Because it
doesn't support filtering on field names, span contexts, or field
values, the implementation is much simpler, and it doesn't require the
env_filter feature flag. Also, Targets can easily implement the
Filter trait for per-layer filtering, while adding a Filter
implementation for EnvFilter will require additional effort.

Because the Targets filter doesn't allow specifiyng span or
field-value filters, the syntax for parsing one from a string is
significantly simpler than EnvFilter's. Therefore, it can have a very
simple handwritten parser implementation that doesn't require the
regex crate. This should be useful for users who are concerned about
the number of dependencies required by EnvFilter.

The new implementation is quite small, as it mostly uses the same code
as the static filter subset of EnvFilter. This code was factored out
into a shared module for use in both EnvFilter and Targets. The code
required for dynamic filtering with EnvFilter (i.e. on fields and
spans) is still in the filter::env module and is only enabled by the
env-filter feature flag.

I'm open to renaming the new type; I thought filter::Targets seemed
good, but would also be willing to go with TargetFilter or something.

Signed-off-by: Eliza Weisman eliza@buoyant.io

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

This branch adds a new `Targets` filter to `tracing_subscriber`. The `Targets` filter is very similar to `EnvFilter`, but it _only_ consists of filtering directives consisting of a target and level. Because it doesn't support filtering on field names, span contexts, or field values, the implementation is *much* simpler, and it doesn't require the `env_filter` feature flag. Also, `Targets` can easily implement the `Filter` trait for per-layer filtering, while adding a `Filter` implementation for `EnvFilter` will require additional effort. Because the `Targets` filter doesn't allow specifiyng span or field-value filters, the syntax for parsing one from a string is significantly simpler than `EnvFilter`'s. Therefore, it can have a very simple handwritten parser implementation that doesn't require the `regex` crate. This should be useful for users who are concerned about the number of dependencies required by `EnvFilter`. The new implementation is quite small, as it mostly uses the same code as the static filter subset of `EnvFilter`. This code was factored out into a shared module for use in both `EnvFilter` and `Targets`. The code required for _dynamic_ filtering with `EnvFilter` (i.e. on fields and spans) is still in the `filter::env` module and is only enabled by the `env-filter` feature flag. I'm open to renaming the new type; I thought `filter::Targets` seemed good, but would also be willing to go with `TargetFilter` or something.

hawkw · 2021-09-11T01:44:31Z

bleh, looks like i screwed up the doctests

matklad

I love this, thanks!

matklad · 2021-09-11T06:23:57Z

tracing-subscriber/src/filter/directive.rs

+/// Unlike a dynamic directive, this can be cached by the callsite.
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub(crate) struct StaticDirective {
+    pub(in crate::filter) target: Option<String>,


Curious, why do you prefer pub(in crate::filter) to `pub(super)?

In general, I try to avoid using super except in modules that are nested in the same file (like tests modules), because it's nice to be able to move the file around in the module tree and not have to fix imports and visibility modifiers.

matklad · 2021-09-11T06:33:45Z

tracing-subscriber/src/filter/directive.rs

+            })
+            .reverse();
+
+        #[cfg(debug_assertions)]


matklad · 2021-09-11T06:37:44Z

tracing-subscriber/src/filter/directive.rs

+
+        if meta.is_event() && !self.field_names.is_empty() {
+            let fields = meta.fields();
+            for name in &self.field_names {


Just a note: if you write for x in &xs here, then you can if let Some(x) = &ox above instead of ref target. To me, using & for iteration and match ergonomics feel very close semantically, and it is a tiny bit odd to see one, but not the other.

yeah, I agree that this would be nicer. however, this code was already there; it was just moved around. I didn't modify the existing code that I moved in this PR except where it was strictly necessary for the change, for the sake of keeping the git history neater. i might come back and fix that up in a follow-up branch.

matklad · 2021-09-11T06:47:19Z

tracing-subscriber/src/filter/directive.rs

+
+impl FromStr for StaticDirective {
+    type Err = ParseError;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {


For from_str and other parsing routines, I find it very useful to add comments with examples of what is parsed:

// this parses // 1 + 2

That’s a very efficient way to give a lot of context to the (usually somewhat tricky) code bellow. Examples usually work better than grammar, as you don’t need to decipher them,

tracing-subscriber/src/filter/directive.rs

matklad · 2021-09-11T07:05:56Z

tracing-subscriber/src/filter/directive.rs

+impl From<Box<dyn Error + Send + Sync>> for ParseError {
+    fn from(e: Box<dyn Error + Send + Sync>) -> Self {
+        Self {
+            kind: ParseErrorKind::Field(e),


This impl feels odd to me: it seems like it is only applicable to one, specific call-site, but it is very very general, as return type is a poor man’s anyhow basically.

Ohhhhh, I now see that I can formulate a code-smell here:

having From for BarError, which is used only once.

the single call-site means that we can convert from Foo to Bar in one specific context. But the impl means that we can do that in any context. I think, in such situations it’s better to do the conversion manually.

This is philosophical, but what is somewhat more well-grounded is that ParseError is a public type, so this would be a public impl which the user could use. And such an impl is a very odd one for a public API.

again, this is code that previously existed, but was moved and renamed. I believe that this impl actually exists because a user requested the ability to create custom ParseErrors for some kind of testing purpose.

matklad · 2021-09-11T07:08:09Z

tracing-subscriber/src/filter/mod.rs


+pub use self::directive::ParseError as DirectiveParseError;


If you need to do this rename even internally, maybe might as well just call the thing DirectiveParseError?

tracing-subscriber/src/filter/targets.rs

matklad · 2021-09-11T15:37:07Z

tracing-subscriber/src/filter/directive.rs

+                let fields = maybe_fields
+                    .strip_suffix("}]")
+                    .ok_or_else(|| ParseError::msg("expected fields list to end with '}]'"))?;
+                field_names.extend(fields.split(',').filter_map(|s| {


Hm, is this .split(',') correct? There's a .split(',') which the Target does:

https://github.com/matklad/tracing/blob/33f030f3f6a12ff6543e5969eea3ca549ab4dc7c/tracing-subscriber/src/filter/targets.rs#L257-L259

So, the two would conflict I believe?

With my "stuck writing parsers" hat on, I sort-of feel that neither regexes, nor .split are the most appropriate parsing tools in this case. I personally would reach for "let's write recursive descent parser by hand" here, sort-of how sevmer does this: https://github.com/dtolnay/semver/blob/master/src/parse.rs.

You're right that as this is currently used, there will never be commas inside the directive, because this FromStr impl is only called in Targets::from_str, which already splits on commas. However, I wanted to handle directives with field lists in this FromStr impl, because I thought it would eventually be nice to allow the user to manually parse individual StaticDirectives. Also, if people want to use field name filtering with Targets, we could eventually rewrite the Targets parser to not just split on commas and use recursive descent or something, so that commas can be nested within filter directives.

I wanted to get something simple working now, though, and go back and add support for field name filtering later.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

…o eliza/target-filter

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2021-09-11T16:34:13Z

thanks @matklad for the review!

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

davidbarsky

I think @matklad got most of the comments I wanted to leave :)

@jsgf

# 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. ([#1523]) ### Fixed - **layer** `Context` methods that are provided when the `Subscriber` implements `LookupSpan` no longer require the "registry" feature flag ([#1525]) - **layer** `fmt::Debug` implementation for `Layered` no longer requires the `S` type parameter to implement `Debug` ([#1528]) ### Added - **registry**: `Filter` trait, `Filtered` type, `Layer::with_filter` method, and other APIs for per-layer filtering ([#1523]) - **filter**: `FilterFn` and `DynFilterFn` types that implement global (`Layer`) and per-layer (`Filter`) filtering for closures and function pointers ([#1523]) - **filter**: `Targets` filter, which implements a lighter-weight form of `EnvFilter`-like filtering ([#1550]) - **env-filter**: Added support for filtering on floating-point values ([#1507]) - **layer**: `Layer::on_layer` callback, called when layering the `Layer` onto a `Subscriber` ([#1523]) - **layer**: `Layer` implementations for `Box<L>` and `Arc<L>` where `L: Layer` ([#1536]) - **layer**: `Layer` implementations for `Box<dyn Layer<S> + Send + Sync + 'static>` and `Arc<dyn Layer<S> + Send + Sync + 'static>` ([#1536]) - A number of small documentation fixes and improvements ([#1553], [#1544], [#1539], [#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 [#1507]: #1507 [#1523]: #1523 [#1524]: #1524 [#1525]: #1525 [#1528]: #1528 [#1539]: #1539 [#1544]: #1544 [#1550]: #1550 [#1553]: #1553

@jsgf

# 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. ([#1523]) ### Fixed - **layer** `Context` methods that are provided when the `Subscriber` implements `LookupSpan` no longer require the "registry" feature flag ([#1525]) - **layer** `fmt::Debug` implementation for `Layered` no longer requires the `S` type parameter to implement `Debug` ([#1528]) ### Added - **registry**: `Filter` trait, `Filtered` type, `Layer::with_filter` method, and other APIs for per-layer filtering ([#1523]) - **filter**: `FilterFn` and `DynFilterFn` types that implement global (`Layer`) and per-layer (`Filter`) filtering for closures and function pointers ([#1523]) - **filter**: `Targets` filter, which implements a lighter-weight form of `EnvFilter`-like filtering ([#1550]) - **env-filter**: Added support for filtering on floating-point values ([#1507]) - **layer**: `Layer::on_layer` callback, called when layering the `Layer` onto a `Subscriber` ([#1523]) - **layer**: `Layer` implementations for `Box<L>` and `Arc<L>` where `L: Layer` ([#1536]) - **layer**: `Layer` implementations for `Box<dyn Layer<S> + Send + Sync + 'static>` and `Arc<dyn Layer<S> + Send + Sync + 'static>` ([#1536]) - A number of small documentation fixes and improvements ([#1553], [#1544], [#1539], [#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 [#1507]: #1507 [#1523]: #1523 [#1524]: #1524 [#1525]: #1525 [#1528]: #1528 [#1539]: #1539 [#1544]: #1544 [#1550]: #1550 [#1553]: #1553

@jsgf

# 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. ([#1523]) ### Fixed - **layer** `Context` methods that are provided when the `Subscriber` implements `LookupSpan` no longer require the "registry" feature flag ([#1525]) - **layer** `fmt::Debug` implementation for `Layered` no longer requires the `S` type parameter to implement `Debug` ([#1528]) ### Added - **registry**: `Filter` trait, `Filtered` type, `Layer::with_filter` method, and other APIs for per-layer filtering ([#1523]) - **filter**: `FilterFn` and `DynFilterFn` types that implement global (`Layer`) and per-layer (`Filter`) filtering for closures and function pointers ([#1523]) - **filter**: `Targets` filter, which implements a lighter-weight form of `EnvFilter`-like filtering ([#1550]) - **env-filter**: Added support for filtering on floating-point values ([#1507]) - **layer**: `Layer::on_layer` callback, called when layering the `Layer` onto a `Subscriber` ([#1523]) - **layer**: `Layer` implementations for `Box<L>` and `Arc<L>` where `L: Layer` ([#1536]) - **layer**: `Layer` implementations for `Box<dyn Layer>` and `Arc<dyn Layer>` ([#1536]) - A number of small documentation fixes and improvements ([#1553], [#1544], [#1539], [#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 [#1507]: #1507 [#1523]: #1523 [#1524]: #1524 [#1525]: #1525 [#1528]: #1528 [#1539]: #1539 [#1544]: #1544 [#1550]: #1550 [#1553]: #1553

@jsgf

# 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. ([#1523]) ### Fixed - **layer** `Context` methods that are provided when the `Subscriber` implements `LookupSpan` no longer require the "registry" feature flag ([#1525]) - **layer** `fmt::Debug` implementation for `Layered` no longer requires the `S` type parameter to implement `Debug` ([#1528]) ### Added - **registry**: `Filter` trait, `Filtered` type, `Layer::with_filter` method, and other APIs for per-layer filtering ([#1523]) - **filter**: `FilterFn` and `DynFilterFn` types that implement global (`Layer`) and per-layer (`Filter`) filtering for closures and function pointers ([#1523]) - **filter**: `Targets` filter, which implements a lighter-weight form of `EnvFilter`-like filtering ([#1550]) - **env-filter**: Added support for filtering on floating-point values ([#1507]) - **layer**: `Layer::on_layer` callback, called when layering the `Layer` onto a `Subscriber` ([#1523]) - **layer**: `Layer` implementations for `Box<L>` and `Arc<L>` where `L: Layer` ([#1536]) - **layer**: `Layer` implementations for `Box<dyn Layer>` and `Arc<dyn Layer>` ([#1536]) - A number of small documentation fixes and improvements ([#1553], [#1544], [#1539], [#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 [#1507]: #1507 [#1523]: #1523 [#1524]: #1524 [#1525]: #1525 [#1528]: #1528 [#1539]: #1539 [#1544]: #1544 [#1550]: #1550 [#1553]: #1553

I accidentally renamed this type in PR #1550 after getting confused by a renaming import that I thought was publicly re-exported, but it wasn't actually..sorry about that! This commit renames (or...un-renames?) the type. Whoopsie!

I accidentally renamed this type in PR #1550 after getting confused by a renaming import that I thought was publicly re-exported, but it wasn't actually..sorry about that! This commit renames (or...un-renames?) the type. Whoopsie! Fixes #1557

## Motivation The `DirectiveSet` type used in `EnvFilter` and `Targets` uses `SmallVec` to store the filtering directives when the `SmallVec` feature is enabled. This is intended to improve the performance of iterating over small sets of directives, by avoiding a heap pointer dereference. PR #1550 changed the directives themselves to also use `SmallVec` for storing _field_ filters. This was intended to make the same optimization for field filters. However, it had unintended consequences: an empty `SmallVec` is an array of `T` items (plus metadata), while an empty `Vec` is just a couple of words. Since _most_ filters don't have field filters, this meant that we were suddenly using a lot more space to store...nothing. This made `EnvFilter`s _much_ larger, causing problems for some users (see #1567). ## Solution This branch undoes the change to `SmallVec` for field name/value filters. This takes the size of an `EnvFilter` from 5420 bytes back down to 1272 bytes. I also added some tests that just print the size of various `EnvFilter` and `Targets` values. These don't make any assertions, but can be run for development purposes when making changes to these types. Fixes #1567 Signed-off-by: Eliza Weisman <eliza@buoyant.io>

This branch adds a new `Targets` filter to `tracing_subscriber`. The `Targets` filter is very similar to `EnvFilter`, but it _only_ consists of filtering directives consisting of a target and level. Because it doesn't support filtering on field names, span contexts, or field values, the implementation is *much* simpler, and it doesn't require the `env_filter` feature flag. Also, `Targets` can easily implement the `Filter` trait for per-layer filtering, while adding a `Filter` implementation for `EnvFilter` will require additional effort. Because the `Targets` filter doesn't allow specifiyng span or field-value filters, the syntax for parsing one from a string is significantly simpler than `EnvFilter`'s. Therefore, it can have a very simple handwritten parser implementation that doesn't require the `regex` crate. This should be useful for users who are concerned about the number of dependencies required by `EnvFilter`. The new implementation is quite small, as it mostly uses the same code as the static filter subset of `EnvFilter`. This code was factored out into a shared module for use in both `EnvFilter` and `Targets`. The code required for _dynamic_ filtering with `EnvFilter` (i.e. on fields and spans) is still in the `filter::env` module and is only enabled by the `env-filter` feature flag. I'm open to renaming the new type; I thought `filter::Targets` seemed good, but would also be willing to go with `TargetFilter` or something. Signed-off-by: Eliza Weisman <eliza@buoyant.io>