subscriber: add combinators for combining filters #1578

hawkw · 2021-09-18T19:29:39Z

This isn't quite done yet, but I thought it was kinda cool.

hawkw · 2021-09-19T18:53:10Z

tracing-subscriber/src/filter/layer_filters/combinator.rs

+    }
+
+    fn max_level_hint(&self) -> Option<LevelFilter> {
+        // TODO(eliza): figure this out???


this is the main reason this PR is still a draft

jeez, yeah, that really fucks with my brain. i'd maybe return None for now

The reason this is confusing is that I don't think we can simply return the "opposite" of the inner filter's hint, since the filter might be enabling things based on a particular characteristic other than levels, and we might still enable things of the same level. I think maybe we should just return None and have this break caching...though honestly, I'm not totally sure how necessary the Not filter even is...

hawkw · 2021-09-19T19:29:33Z

tracing-subscriber/tests/layer_filters/combinators.rs

+#[test]
+fn and() {
+    let (layer, handle) = layer::mock()
+        .event(
+            event::msg("a very interesting event")
+                .at_level(tracing::Level::INFO)
+                .with_target("interesting_target"),
+        )
+        .done()
+        .run_with_handle();
+
+    // Enables spans and events with targets starting with `interesting_target`:
+    let target_filter = filter::filter_fn(|meta| meta.target().starts_with("interesting_target"));
+
+    // Enables spans and events with levels `INFO` and below:
+    let level_filter = LevelFilter::INFO;
+
+    // Combine the two filters together, returning a filter that only enables
+    // spans and events that *both* filters will enable:
+    let filter = target_filter.and(level_filter);
+
+    let _subscriber = tracing_subscriber::registry()
+        .with(layer.with_filter(filter))
+        .set_default();
+
+    // This event will *not* be enabled:
+    tracing::info!("an event with an uninteresting target");
+
+    // This event *will* be enabled:
+    tracing::info!(target: "interesting_target", "a very interesting event");
+
+    // This event will *not* be enabled:
+    tracing::debug!(target: "interesting_target", "interesting debug event...");
+
+    handle.assert_finished();
+}


whoops this was supposed to be in a separate commit. lol whatever.

hawkw · 2021-09-19T19:30:24Z

@davidbarsky any thoughts on whether the approach in a131037 or in 14e3a9b is better?

hawkw · 2021-09-24T17:57:35Z

@davidbarsky ping

hawkw · 2021-12-01T21:32:40Z

@davidbarsky ping

davidbarsky · 2021-12-02T18:29:02Z

@hawkw notes before i review the rest: very naively: I think I like the approach in 14e3a9b better, despite the fact that it might result in slightly worse compile times, but i'd be shocked if its noticable. that being said, what is the actual impact of a131037 in terms of combinator use?

tracing-subscriber/src/filter/layer_filters/combinator.rs

davidbarsky · 2021-12-21T00:11:08Z

tracing-subscriber/src/filter/layer_filters/combinator.rs

+    }
+
+    fn max_level_hint(&self) -> Option<LevelFilter> {
+        // TODO(eliza): figure this out???


jeez, yeah, that really fucks with my brain. i'd maybe return None for now

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

apparently the `S` type param just can't ever be inferred when using the combinators. that kicks ass lmao Signed-off-by: Eliza Weisman <eliza@buoyant.io>

...but i'm not sure if that's better or worse. this fixes the type inference issues and allows us to check that the combined layers are both `Filter`s for the same subscriber. it also allows the `FilterExt` trait to only be implemented for `Filter`s. on the other hand, it means propagating a bunch of phantomdatas everywhere. this makes the code grosser and also takes longer for the compiler to typecheck (though i doubt people will be nesting these combinators deep enough to notice). it also means we can't derive traits like `Debug` and `Clone`, and need manual implementations. which is *fine*, it's just annoying... Signed-off-by: Eliza Weisman <eliza@buoyant.io>

@davidbarsky

thanks @davidbarsky Co-authored-by: David Barsky <me@davidbarsky.com>

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

@matze

# 0.3.7 (Jan 25, 2022) This release adds combinators for combining filters. Additionally, this release also updates the `thread-local` crate to v1.1.4, fixing warnings for the security advisory [RUSTSEC-2022-0006]. Note that previous versions of `tracing-subscriber` did not use any of the `thread-local` crate's APIs effected by the vulnerability. However, updating the version fixes warnings emitted by `cargo audit` and similar tools. ### Added - **filter**: Added combinators for combining filters ([#1578]) ### Fixed - **registry**: Updated `thread-local` to v1.1.4 ([#1858]) Thanks to new contributor @matze for contributing to this release! [RUSTSEC-2022-0006]: https://rustsec.org/advisories/RUSTSEC-2022-0006 [#1578]: #1578 [#1858]: #1858

@matze

# 0.3.7 (Jan 25, 2022) This release adds combinators for combining filters. Additionally, this release also updates the `thread-local` crate to v1.1.4, fixing warnings for the security advisory [RUSTSEC-2022-0006]. Note that previous versions of `tracing-subscriber` did not use any of the `thread-local` crate's APIs effected by the vulnerability. However, updating the version fixes warnings emitted by `cargo audit` and similar tools. ### Added - **filter**: Added combinators for combining filters ([#1578]) ### Fixed - **registry**: Updated `thread-local` to v1.1.4 ([#1858]) Thanks to new contributor @matze for contributing to this release! [RUSTSEC-2022-0006]: https://rustsec.org/advisories/RUSTSEC-2022-0006 [#1578]: #1578 [#1858]: #1858

## Motivation In some cases, it can be useful to express a more complex filtering behavior by composing multiple filters. This is especially useful when re-using the same logic in multiple filters. ## Solution This branch adds a new `FilterExt` extension trait with combinators for composing filters. The current set of combinators are `And` (enables spans/events that are enabled by *both* filters), `Or` (enables any span or event enabled by *either* filter), and `Not` (negate the output of a filter). Signed-off-by: Eliza Weisman <eliza@buoyant.io>

@matze

# 0.3.7 (Jan 25, 2022) This release adds combinators for combining filters. Additionally, this release also updates the `thread-local` crate to v1.1.4, fixing warnings for the security advisory [RUSTSEC-2022-0006]. Note that previous versions of `tracing-subscriber` did not use any of the `thread-local` crate's APIs effected by the vulnerability. However, updating the version fixes warnings emitted by `cargo audit` and similar tools. ### Added - **filter**: Added combinators for combining filters ([tokio-rs#1578]) ### Fixed - **registry**: Updated `thread-local` to v1.1.4 ([tokio-rs#1858]) Thanks to new contributor @matze for contributing to this release! [RUSTSEC-2022-0006]: https://rustsec.org/advisories/RUSTSEC-2022-0006 [tokio-rs#1578]: tokio-rs#1578 [tokio-rs#1858]: tokio-rs#1858

hawkw requested a review from davidbarsky September 18, 2021 19:29

hawkw self-assigned this Sep 18, 2021

hawkw commented Sep 19, 2021

View reviewed changes

davidbarsky reviewed Dec 21, 2021

View reviewed changes

hawkw and others added 9 commits December 22, 2021 09:27

subscriber: use a macro for filter deref impls

ee747e7

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

subscriber: add layer filter combinators

4dde711

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

wip

883e142

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

more docs

c0185b2

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

doctest fixy

8538951

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

no "type constraints", WE DIE LIKE MEN

f99cb77

apparently the `S` type param just can't ever be inferred when using the combinators. that kicks ass lmao Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Apply docs suggestions from code review

029bb5f

thanks @davidbarsky Co-authored-by: David Barsky <me@davidbarsky.com>

make new constructorspub(crate)

17b5680

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

hawkw force-pushed the eliza/filter-combinators branch from 647ff18 to 17b5680 Compare December 22, 2021 17:32

hawkw marked this pull request as ready for review December 22, 2021 17:33

hawkw requested a review from a team as a code owner December 22, 2021 17:33

davidbarsky approved these changes Dec 22, 2021

View reviewed changes

Merge branch 'v0.1.x' into eliza/filter-combinators

88b0de6

hawkw changed the title ~~[WIP] subscriber: add combinators for combining filters~~ subscriber: add combinators for combining filters Jan 14, 2022

davidbarsky and others added 2 commits January 21, 2022 13:46

Merge branch 'v0.1.x' into eliza/filter-combinators

8e3a922

Merge branch 'v0.1.x' into eliza/filter-combinators

347ca4e

hawkw merged commit 5ddbd4e into v0.1.x Jan 24, 2022

hawkw deleted the eliza/filter-combinators branch January 24, 2022 22:05

hawkw mentioned this pull request Jan 25, 2022

subscriber: prepare to release v0.3.7 #1860

Merged

davidbarsky mentioned this pull request Sep 26, 2023

chore: backport roughly a year's worth of changes #2728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subscriber: add combinators for combining filters #1578

subscriber: add combinators for combining filters #1578

hawkw commented Sep 18, 2021

hawkw Sep 19, 2021

davidbarsky Dec 21, 2021

hawkw Jan 13, 2022

hawkw Sep 19, 2021

hawkw commented Sep 19, 2021

hawkw commented Sep 24, 2021

hawkw commented Dec 1, 2021

davidbarsky commented Dec 2, 2021

davidbarsky Dec 21, 2021

subscriber: add combinators for combining filters #1578

subscriber: add combinators for combining filters #1578

Conversation

hawkw commented Sep 18, 2021

hawkw Sep 19, 2021

Choose a reason for hiding this comment

davidbarsky Dec 21, 2021

Choose a reason for hiding this comment

hawkw Jan 13, 2022

Choose a reason for hiding this comment

hawkw Sep 19, 2021

Choose a reason for hiding this comment

hawkw commented Sep 19, 2021

hawkw commented Sep 24, 2021

hawkw commented Dec 1, 2021

davidbarsky commented Dec 2, 2021

davidbarsky Dec 21, 2021

Choose a reason for hiding this comment