Implement subscribe::Filter for Option<Filter> #2407

jsgf · 2022-12-08T00:11:44Z

Motivation

It's currently awkward to have an optional per-subscriber filter.

Solution

Implement Filter<S> for Option<F> where F: Filter<S>, following the example of Subscribe. A None filter passes everything through.

Also, it looks like Filter for Arc/Box doesn't pass through all the methods, so extend the filter_impl_body macro to include them.

This also adds a couple of tests and updates the docs.

Pass all the Filter methods through to the underlying Box/Arc Filter rather than relying on default implementations.

Where None means "no filtering".

Make sure None lets everything through.

davidbarsky

three tiny nits that can you can apply, but this change looks good to me overall.

davidbarsky · 2022-12-13T16:42:26Z

tracing-subscriber/src/subscribe/mod.rs

+//! trait may also be implemented for user-defined types. [`Option<Filter>`]
+//! also implements [`Filter`], to allow for a (surprise!) optional filter -
+//! [`None`](Option::None) filters nothing (that is, allows everything through).


Suggested change

//! trait may also be implemented for user-defined types. [`Option<Filter>`]

//! also implements [`Filter`], to allow for a (surprise!) optional filter -

//! [`None`](Option::None) filters nothing (that is, allows everything through).

//! trait may also be implemented for user-defined types. [`Option<Filter>`]

//! also implements [`Filter`], which allows for an optional filter where

//! [`None`](Option::None) filters out _nothing_ (that is, allows everything through).

aww, i like the (surprise!) :(

davidbarsky · 2022-12-13T16:47:36Z

tracing-subscriber/tests/subscriber_filters/option.rs

+#[test]
+fn option_some() {
+    let (subscribe, handle) = subscriber::mock().only().run_with_handle();
+    let subscribe = Box::new(subscribe.with_filter(Some(filter())));


nit: i'm not sure if Box is necessary, but if it is, then that's fine.

I copied boxed.rs as a starting point. I can unbox.

davidbarsky · 2022-12-13T16:48:21Z

tracing-subscriber/tests/subscriber_filters/option.rs

+use super::*;
+use tracing_subscriber::{filter, prelude::*, Subscribe};
+
+fn filter<S>() -> filter::DynFilterFn<S> {


small nit, since it took me a second to understand what was happening here:

Suggested change

fn filter<S>() -> filter::DynFilterFn<S> {

fn filter_out_everything<S>() -> filter::DynFilterFn<S> {

Ditto copied from boxed.rs.

jsgf · 2022-12-13T17:31:05Z

Updated with fixes for review comments.

hawkw

Overall, this looks good and it definitely makes sense to add, thanks!

Because the interactions between per-subscriber filters can be somewhat complex, especially when the max-level hints are involved, it would be nice to see tests that test Option filters in combination with other subscribers with their own per-layer filters. The current tests are kind of only testing the happy path for the per-layer filtering system, where there aren't multiple sets of filters for separate subscribers, and it would be nice to test the more difficult cases, as well.

Thanks!

hawkw · 2022-12-13T21:04:11Z

tracing-subscriber/src/subscribe/mod.rs

+//! trait may also be implemented for user-defined types. [`Option<Filter>`]
+//! also implements [`Filter`], which allows for an optional filter where
+//! [`None`](Option::None) filters out _nothing_ (that is, allows everything
+//! through).


it could maybe be nice to have an example somewhere showing how and why we might want to use the Filter impl for Option?

hawkw · 2022-12-13T21:05:56Z

tracing-subscriber/tests/subscriber_filters/option.rs

+#[test]
+fn option_some() {
+    let (subscribe, handle) = subscriber::mock().only().run_with_handle();
+    let subscribe = subscribe.with_filter(Some(filter_out_everything()));
+
+    let _guard = tracing_subscriber::registry().with(subscribe).set_default();
+
+    for i in 0..2 {
+        tracing::info!(i);
+    }
+
+    handle.assert_finished();
+}
+
+#[test]
+fn option_none() {
+    let (subscribe, handle) = subscriber::mock()
+        .event(expect::event())
+        .event(expect::event())
+        .only()
+        .run_with_handle();
+    let subscribe = Box::new(subscribe.with_filter(None::<filter::DynFilterFn<_>>));
+
+    let _guard = tracing_subscriber::registry().with(subscribe).set_default();
+
+    for i in 0..2 {
+        tracing::info!(i);
+    }
+
+    handle.assert_finished();


let's also add some tests where a registry includes a subscriber that has an Option filter, as well as a subscriber with a non-optional filter, to ensure that the Option impl plays nicely with other per-layer filters?

jsgf · 2022-12-14T17:18:20Z

Updated with @hawkw's suggestions.

hawkw · 2022-12-14T22:43:31Z

tracing-subscriber/tests/subscriber_filters/option.rs

+    let (subscribe, handle) = subscriber::mock()
+        .event(expect::event())
+        .only()
+        .run_with_handle();
+    let subscribe = subscribe
+        .with_filter(filter::dynamic_filter_fn(|meta, _ctx| {
+            meta.target() == "interesting"
+        }))
+        .with_filter(None::<filter::DynFilterFn<_>>);


this is worth having, but the scenario i really wanted to test was not a single subscriber with two different filters, but two different subscribers with separate filters, where one subscriber's filter was a None. in particular, we want to assert that the None filter does not inadvertantly enable anything that the other subscriber's filter would enable.

we also really want to test that the correct max level hint is used, since that's something we've had bugs with in the past.

* Added tests for max level filtering and interest caching with multiple subscribers. * Changed the interest level for a `None` filter from `sometimes` to `always` so other filters can be cached more effectively.

Add tests and tweak `None` filter interest level

jsgf · 2023-05-01T06:47:37Z

@hawkw @davidbarsky OK, I think this now, thanks to @ryanthomasjohnson, has the additional tests you wanted. I've merged in current master and all the tests pass locally.

hawkw · 2023-05-01T19:47:19Z

@hawkw @davidbarsky OK, I think this now, thanks to @ryanthomasjohnson, has the additional tests you wanted. I've merged in current master and all the tests pass locally.

Cool, taking a look!

jsgf · 2023-05-31T16:55:42Z

Ping? Is there anything outstanding on my side for this?

davidbarsky · 2023-05-31T19:57:00Z

Ping? Is there anything outstanding on my side for this?

No, don't think so. the test that you wrote seems to exercise what Eliza wanted it to exercise. I'll rebase, but I'm perfectly fine with the changes.

zertosh · 2023-07-26T15:46:23Z

Any chance this can merge?

hawkw · 2023-05-01T19:58:48Z

tracing-subscriber/src/subscribe/mod.rs

 //! function pointer. In addition, when more control is required, the [`Filter`]
 //! trait may also be implemented for user-defined types.
 //!
+//! [`Option<Filter>`] also implements [`Filter`], which allows for an optional


nit, not a blocker: it could be nice to link to the implementation of Filter for Option<Filter> in the docs here?

What's the rustdoc syntax to link to a trait implementation?

It's currently awkward to have an optional per-layer filter. Implement `Filter<L>` for `Option<F> where F: Filter<L>`, following the example of `Layer`. A `None` filter passes everything through. Also, it looks like Filter for Arc/Box doesn't pass through all the methods, so extend the `filter_impl_body` macro to include them. This also adds a couple of tests and updates the docs. --------- Co-authored-by: David Barsky <me@davidbarsky.com> Co-authored-by: Ryan Johnson <ryantj@fb.com> Co-authored-by: Eliza Weisman <eliza@buoyant.io>

@shayne-fletcher

# 0.3.18 (November 13, 2023) This release of `tracing-subscriber` adds support for the [`NO_COLOR`] environment variable (an informal standard to disable emitting ANSI color escape codes) in `fmt::Layer`, reintroduces support for the [`chrono`] crate, and increases the minimum supported Rust version (MSRV) to Rust 1.63.0. It also introduces several minor API improvements. ### Added - **chrono**: Add [`chrono`] implementations of `FormatTime` ([#2690]) - **subscriber**: Add support for the [`NO_COLOR`] environment variable in `fmt::Layer` ([#2647]) - **fmt**: make `format::Writer::new()` public ([#2680]) - **filter**: Implement `layer::Filter` for `Option<Filter>` ([#2407]) ### Changed - **log**: bump version of `tracing-log` to 0.2 ([#2772]) - Increased minimum supported Rust version (MSRV) to 1.63.0+. [`chrono`]: https://github.com/chronotope/chrono [`NO_COLOR`]: https://no-color.org/ [#2690]: #2690 [#2647]: #2647 [#2680]: #2680 [#2407]: #2407 [#2772]: #2772 Thanks to @shayne-fletcher, @dmlary, @kaifastromai, and @jsgf for contributing!

Summary: Replaces the various `tracing` forks for [tokio-rs/tracing#2407] with crates-io releases containing those changes. [tokio-rs/tracing#2407]: tokio-rs/tracing#2407 Reviewed By: capickett, davidbarsky Differential Revision: D51266382 fbshipit-source-id: 5ad0b7b9477e5fa450e52c639357fe485e0e89ff

@shayne-fletcher

# 0.3.18 (November 13, 2023) This release of `tracing-subscriber` adds support for the [`NO_COLOR`] environment variable (an informal standard to disable emitting ANSI color escape codes) in `fmt::Layer`, reintroduces support for the [`chrono`] crate, and increases the minimum supported Rust version (MSRV) to Rust 1.63.0. It also introduces several minor API improvements. ### Added - **chrono**: Add [`chrono`] implementations of `FormatTime` ([tokio-rs#2690]) - **subscriber**: Add support for the [`NO_COLOR`] environment variable in `fmt::Layer` ([tokio-rs#2647]) - **fmt**: make `format::Writer::new()` public ([tokio-rs#2680]) - **filter**: Implement `layer::Filter` for `Option<Filter>` ([tokio-rs#2407]) ### Changed - **log**: bump version of `tracing-log` to 0.2 ([tokio-rs#2772]) - Increased minimum supported Rust version (MSRV) to 1.63.0+. [`chrono`]: https://github.com/chronotope/chrono [`NO_COLOR`]: https://no-color.org/ [tokio-rs#2690]: tokio-rs#2690 [tokio-rs#2647]: tokio-rs#2647 [tokio-rs#2680]: tokio-rs#2680 [tokio-rs#2407]: tokio-rs#2407 [tokio-rs#2772]: tokio-rs#2772 Thanks to @shayne-fletcher, @dmlary, @kaifastromai, and @jsgf for contributing!

jsgf added 4 commits December 7, 2022 14:23

Fully implement Filter for Box<Filter>

8919fa0

Pass all the Filter methods through to the underlying Box/Arc Filter rather than relying on default implementations.

Implement Filter for Option<F: Filter>

00e304b

Where None means "no filtering".

Add test for Option<Filter> on subscribers

5540321

Make sure None lets everything through.

Add documentation for Option<Filter>

38f5416

jsgf requested review from hawkw, davidbarsky and a team as code owners December 8, 2022 00:11

jsgf mentioned this pull request Dec 8, 2022

Implement layer::Filter for Option<Filter> #2402

Closed

davidbarsky requested changes Dec 13, 2022

View reviewed changes

review comment fixes

74f94ee

davidbarsky approved these changes Dec 13, 2022

View reviewed changes

Merge branch 'master' into option-layer-filter

bca61af

hawkw requested changes Dec 13, 2022

View reviewed changes

address more review comments

b447e17

hawkw reviewed Dec 14, 2022

View reviewed changes

ryanthomasjohnson and others added 3 commits March 27, 2023 10:12

Add tests and tweak None filter interest level

7a6bb31

* Added tests for max level filtering and interest caching with multiple subscribers. * Changed the interest level for a `None` filter from `sometimes` to `always` so other filters can be cached more effectively.

Merge pull request #1 from ryanthomasjohnson/ryantj/option-layer-filter

8568d96

Add tests and tweak `None` filter interest level

Merge branch 'tokio-rs:master' into option-layer-filter

1b7b671

Run cargo fmt

59cba8c

jsgf requested a review from hawkw May 1, 2023 06:49

Merge branch 'master' into option-layer-filter

dfd857a

hawkw approved these changes Jul 26, 2023

View reviewed changes

Merge branch 'master' into option-layer-filter

29d15d4

Fix clippy warning

883c594

davidbarsky mentioned this pull request Aug 3, 2023

tracing-journald: remove unnecessary mut #2673

Merged

davidbarsky added 2 commits August 4, 2023 10:47

Merge branch 'master' into option-layer-filter

01be443

Merge branch 'master' into option-layer-filter

6507da9

davidbarsky enabled auto-merge (squash) September 28, 2023 23:47

davidbarsky merged commit 62d57a6 into tokio-rs:master Sep 28, 2023
55 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement subscribe::Filter for Option<Filter> #2407

Implement subscribe::Filter for Option<Filter> #2407

jsgf commented Dec 8, 2022

davidbarsky left a comment

davidbarsky Dec 13, 2022

hawkw May 1, 2023

davidbarsky Dec 13, 2022

jsgf Dec 13, 2022

davidbarsky Dec 13, 2022

jsgf Dec 13, 2022

jsgf commented Dec 13, 2022

hawkw left a comment

hawkw Dec 13, 2022

hawkw Dec 13, 2022

jsgf commented Dec 14, 2022

hawkw Dec 14, 2022

jsgf commented May 1, 2023 •

edited

hawkw commented May 1, 2023

jsgf commented May 31, 2023

davidbarsky commented May 31, 2023

zertosh commented Jul 26, 2023

hawkw May 1, 2023

jsgf Jul 27, 2023

	fn filter<S>() -> filter::DynFilterFn<S> {
	fn filter_out_everything<S>() -> filter::DynFilterFn<S> {

Implement subscribe::Filter for Option<Filter> #2407

Implement subscribe::Filter for Option<Filter> #2407

Conversation

jsgf commented Dec 8, 2022

Motivation

Solution

davidbarsky 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsgf commented Dec 13, 2022

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

jsgf commented Dec 14, 2022

Choose a reason for hiding this comment

jsgf commented May 1, 2023 • edited

hawkw commented May 1, 2023

jsgf commented May 31, 2023

davidbarsky commented May 31, 2023

zertosh commented Jul 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsgf commented May 1, 2023 •

edited