subscriber: clarify filter.not() docs w.r.t. event_enabled (tokio-r…

…s#2251) * Explain filter.not() w.r.t. event_enabled Co-authored-by: David Barsky <[email protected]> Co-authored-by: Eliza Weisman <[email protected]>
kaffarell · May 22, 2024 · 64f31fa · 64f31fa
1 parent 89179c5
commit 64f31fa
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 6 deletions.
diff --git a/tracing-subscriber/src/filter/layer_filters/combinator.rs b/tracing-subscriber/src/filter/layer_filters/combinator.rs
@@ -405,9 +405,55 @@ where
     ///
     /// This inverts the values returned by the [`enabled`] and [`callsite_enabled`]
     /// methods on the wrapped filter; it does *not* invert [`event_enabled`], as
-    /// implementing that method is optional, and filters which do not implement
-    /// filtering on event field values will return `true` even for events that their
-    /// [`enabled`] method would disable.
+    /// filters which do not implement filtering on event field values will return
+    /// the default `true` even for events that their [`enabled`] method disables.
+    ///
+    /// Consider a normal filter defined as:
+    ///
+    /// ```ignore (pseudo-code)
+    /// // for spans
+    /// match callsite_enabled() {
+    ///     ALWAYS => on_span(),
+    ///     SOMETIMES => if enabled() { on_span() },
+    ///     NEVER => (),
+    /// }
+    /// // for events
+    /// match callsite_enabled() {
+    ///    ALWAYS => on_event(),
+    ///    SOMETIMES => if enabled() && event_enabled() { on_event() },
+    ///    NEVER => (),
+    /// }
+    /// ```
+    ///
+    /// and an inverted filter defined as:
+    ///
+    /// ```ignore (pseudo-code)
+    /// // for spans
+    /// match callsite_enabled() {
+    ///     ALWAYS => (),
+    ///     SOMETIMES => if !enabled() { on_span() },
+    ///     NEVER => on_span(),
+    /// }
+    /// // for events
+    /// match callsite_enabled() {
+    ///     ALWAYS => (),
+    ///     SOMETIMES => if !enabled() { on_event() },
+    ///     NEVER => on_event(),
+    /// }
+    /// ```
+    ///
+    /// A proper inversion would do `!(enabled() && event_enabled())` (or
+    /// `!enabled() || !event_enabled()`), but because of the implicit `&&`
+    /// relation between `enabled` and `event_enabled`, it is difficult to
+    /// short circuit and not call the wrapped `event_enabled`.
+    ///
+    /// A combinator which remembers the result of `enabled` in order to call
+    /// `event_enabled` only when `enabled() == true` is possible, but requires
+    /// additional thread-local mutable state to support a very niche use case.
+    //
+    //  Also, it'd mean the wrapped layer's `enabled()` always gets called and
+    //  globally applied to events where it doesn't today, since we can't know
+    //  what `event_enabled` will say until we have the event to call it with.
     ///
     /// [`Filter`]: crate::layer::Filter
     /// [`enabled`]: crate::layer::Filter::enabled

diff --git a/tracing-subscriber/src/filter/layer_filters/mod.rs b/tracing-subscriber/src/filter/layer_filters/mod.rs
@@ -301,9 +301,55 @@ pub trait FilterExt<S>: layer::Filter<S> {
     ///
     /// This inverts the values returned by the [`enabled`] and [`callsite_enabled`]
     /// methods on the wrapped filter; it does *not* invert [`event_enabled`], as
-    /// implementing that method is optional, and filters which do not implement
-    /// filtering on event field values will return `true` even for events that their
-    /// [`enabled`] method would disable.
+    /// filters which do not implement filtering on event field values will return
+    /// the default `true` even for events that their [`enabled`] method disables.
+    ///
+    /// Consider a normal filter defined as:
+    ///
+    /// ```ignore (pseudo-code)
+    /// // for spans
+    /// match callsite_enabled() {
+    ///     ALWAYS => on_span(),
+    ///     SOMETIMES => if enabled() { on_span() },
+    ///     NEVER => (),
+    /// }
+    /// // for events
+    /// match callsite_enabled() {
+    ///    ALWAYS => on_event(),
+    ///    SOMETIMES => if enabled() && event_enabled() { on_event() },
+    ///    NEVER => (),
+    /// }
+    /// ```
+    ///
+    /// and an inverted filter defined as:
+    ///
+    /// ```ignore (pseudo-code)
+    /// // for spans
+    /// match callsite_enabled() {
+    ///     ALWAYS => (),
+    ///     SOMETIMES => if !enabled() { on_span() },
+    ///     NEVER => on_span(),
+    /// }
+    /// // for events
+    /// match callsite_enabled() {
+    ///     ALWAYS => (),
+    ///     SOMETIMES => if !enabled() { on_event() },
+    ///     NEVER => on_event(),
+    /// }
+    /// ```
+    ///
+    /// A proper inversion would do `!(enabled() && event_enabled())` (or
+    /// `!enabled() || !event_enabled()`), but because of the implicit `&&`
+    /// relation between `enabled` and `event_enabled`, it is difficult to
+    /// short circuit and not call the wrapped `event_enabled`.
+    ///
+    /// A combinator which remembers the result of `enabled` in order to call
+    /// `event_enabled` only when `enabled() == true` is possible, but requires
+    /// additional thread-local mutable state to support a very niche use case.
+    //
+    //  Also, it'd mean the wrapped layer's `enabled()` always gets called and
+    //  globally applied to events where it doesn't today, since we can't know
+    //  what `event_enabled` will say until we have the event to call it with.
     ///
     /// [`Filter`]: crate::subscribe::Filter
     /// [`enabled`]: crate::subscribe::Filter::enabled