From 1b2a0546991521c35f23cf2bbaee824c65f1ffd7 Mon Sep 17 00:00:00 2001 From: Christopher Durham Date: Thu, 28 Jul 2022 17:50:41 -0400 Subject: [PATCH] subscriber: clarify `filter.not()` docs w.r.t. event_enabled (#2251) * Explain filter.not() w.r.t. event_enabled Co-authored-by: David Barsky Co-authored-by: Eliza Weisman --- .../filter/subscriber_filters/combinator.rs | 52 +++++++++++++++++-- .../src/filter/subscriber_filters/mod.rs | 52 +++++++++++++++++-- 2 files changed, 98 insertions(+), 6 deletions(-) diff --git a/tracing-subscriber/src/filter/subscriber_filters/combinator.rs b/tracing-subscriber/src/filter/subscriber_filters/combinator.rs index 067b974fd0..7ccf011cb7 100644 --- a/tracing-subscriber/src/filter/subscriber_filters/combinator.rs +++ b/tracing-subscriber/src/filter/subscriber_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::subscribe::Filter /// [`enabled`]: crate::subscribe::Filter::enabled diff --git a/tracing-subscriber/src/filter/subscriber_filters/mod.rs b/tracing-subscriber/src/filter/subscriber_filters/mod.rs index ade200dc13..9b84477953 100644 --- a/tracing-subscriber/src/filter/subscriber_filters/mod.rs +++ b/tracing-subscriber/src/filter/subscriber_filters/mod.rs @@ -301,9 +301,55 @@ pub trait FilterExt: subscribe::Filter { /// /// 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