From cdae8742783cbea0683ae0d237ecfe6e3f4fc04a Mon Sep 17 00:00:00 2001 From: Christopher Durham Date: Wed, 27 Jul 2022 17:37:38 -0400 Subject: [PATCH 1/4] Explain filter.not() w.r.t. event_enabled --- .../filter/subscriber_filters/combinator.rs | 52 +++++++++++++++++-- .../src/filter/subscriber_filters/mod.rs | 52 +++++++++++++++++-- 2 files changed, 94 insertions(+), 10 deletions(-) diff --git a/tracing-subscriber/src/filter/subscriber_filters/combinator.rs b/tracing-subscriber/src/filter/subscriber_filters/combinator.rs index 067b974fd0..cb76cc40e9 100644 --- a/tracing-subscriber/src/filter/subscriber_filters/combinator.rs +++ b/tracing-subscriber/src/filter/subscriber_filters/combinator.rs @@ -403,11 +403,53 @@ where /// If the wrapped filter would enable a span or event, it will be disabled. If /// it would disable a span or event, that span or event will be enabled. /// - /// 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. + /// The wrapped filter's [`callsite_enabled`] and [`enabled`] are inverted, + /// but [`event_enabled`] is ignored, as most filters always return `true` + /// from `event_enabled` and rely on `[callsite_]enabled` for filtering. + /// + /// A proper inversion would do `!(enabled() && event_enabled())` (or + /// equivalently `!enabled() || !event_enabled()`), but because of the + /// implicit `&&` relation between `enabled` and `event_enabled`, it is + /// not possible to short circuit and not call `event_enabled` from the + /// combinator. + /// + /// Essentially, where a normal filter is roughly + /// + /// ```ignore (psuedo-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 => (), + /// } + /// ``` + /// + /// an inverted filter is roughly + /// + /// ```ignore (psuedo-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 combinator which remembers the result of `enabled` in order to call + /// `event_enabled` only when `enabled() == true` is possible, but requires + /// additional state and locking to support a very niche use case. /// /// [`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..212118702e 100644 --- a/tracing-subscriber/src/filter/subscriber_filters/mod.rs +++ b/tracing-subscriber/src/filter/subscriber_filters/mod.rs @@ -299,11 +299,53 @@ pub trait FilterExt: subscribe::Filter { /// Inverts `self`, returning a filter that enables spans and events only if /// `self` would *not* enable them. /// - /// 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. + /// The wrapped filter's [`callsite_enabled`] and [`enabled`] are inverted, + /// but [`event_enabled`] is ignored, as most filters always return `true` + /// from `event_enabled` and rely on `[callsite_]enabled` for filtering. + /// + /// A proper inversion would do `!(enabled() && event_enabled())` (or + /// equivalently `!enabled() || !event_enabled()`), but because of the + /// implicit `&&` relation between `enabled` and `event_enabled`, it is + /// not possible to short circuit and not call `event_enabled` from the + /// combinator. + /// + /// Essentially, where a normal filter is roughly + /// + /// ```ignore (psuedo-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 => (), + /// } + /// ``` + /// + /// an inverted filter is roughly + /// + /// ```ignore (psuedo-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 combinator which remembers the result of `enabled` in order to call + /// `event_enabled` only when `enabled() == true` is possible, but requires + /// additional state and locking to support a very niche use case. /// /// [`Filter`]: crate::subscribe::Filter /// [`enabled`]: crate::subscribe::Filter::enabled From d7b1d4f04df8da07b45593f8fb8cfde04545884e Mon Sep 17 00:00:00 2001 From: Christopher Durham Date: Thu, 28 Jul 2022 15:24:51 -0400 Subject: [PATCH 2/4] Tweak Filter::not wording further --- .../src/filter/subscriber_filters/combinator.rs | 17 ++++++++++------- .../src/filter/subscriber_filters/mod.rs | 17 ++++++++++------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/tracing-subscriber/src/filter/subscriber_filters/combinator.rs b/tracing-subscriber/src/filter/subscriber_filters/combinator.rs index cb76cc40e9..e2948854f4 100644 --- a/tracing-subscriber/src/filter/subscriber_filters/combinator.rs +++ b/tracing-subscriber/src/filter/subscriber_filters/combinator.rs @@ -407,12 +407,6 @@ where /// but [`event_enabled`] is ignored, as most filters always return `true` /// from `event_enabled` and rely on `[callsite_]enabled` for filtering. /// - /// A proper inversion would do `!(enabled() && event_enabled())` (or - /// equivalently `!enabled() || !event_enabled()`), but because of the - /// implicit `&&` relation between `enabled` and `event_enabled`, it is - /// not possible to short circuit and not call `event_enabled` from the - /// combinator. - /// /// Essentially, where a normal filter is roughly /// /// ```ignore (psuedo-code) @@ -447,9 +441,18 @@ where /// } /// ``` /// + /// A proper inversion would do `!(enabled() && event_enabled())` (or + /// equivalently `!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 state and locking to support a very niche use case. + /// 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 212118702e..21ec8a7bd7 100644 --- a/tracing-subscriber/src/filter/subscriber_filters/mod.rs +++ b/tracing-subscriber/src/filter/subscriber_filters/mod.rs @@ -303,12 +303,6 @@ pub trait FilterExt: subscribe::Filter { /// but [`event_enabled`] is ignored, as most filters always return `true` /// from `event_enabled` and rely on `[callsite_]enabled` for filtering. /// - /// A proper inversion would do `!(enabled() && event_enabled())` (or - /// equivalently `!enabled() || !event_enabled()`), but because of the - /// implicit `&&` relation between `enabled` and `event_enabled`, it is - /// not possible to short circuit and not call `event_enabled` from the - /// combinator. - /// /// Essentially, where a normal filter is roughly /// /// ```ignore (psuedo-code) @@ -343,9 +337,18 @@ pub trait FilterExt: subscribe::Filter { /// } /// ``` /// + /// A proper inversion would do `!(enabled() && event_enabled())` (or + /// equivalently `!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 state and locking to support a very niche use case. + /// 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 From 602d875f971704db31291ed928fa93161d27bb99 Mon Sep 17 00:00:00 2001 From: Christopher Durham Date: Thu, 28 Jul 2022 17:24:23 -0400 Subject: [PATCH 3/4] Apply nits Co-authored-by: David Barsky --- .../src/filter/subscriber_filters/combinator.rs | 17 +++++++++-------- .../src/filter/subscriber_filters/mod.rs | 17 +++++++++-------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/tracing-subscriber/src/filter/subscriber_filters/combinator.rs b/tracing-subscriber/src/filter/subscriber_filters/combinator.rs index e2948854f4..713ca41788 100644 --- a/tracing-subscriber/src/filter/subscriber_filters/combinator.rs +++ b/tracing-subscriber/src/filter/subscriber_filters/combinator.rs @@ -403,11 +403,12 @@ where /// If the wrapped filter would enable a span or event, it will be disabled. If /// it would disable a span or event, that span or event will be enabled. /// - /// The wrapped filter's [`callsite_enabled`] and [`enabled`] are inverted, - /// but [`event_enabled`] is ignored, as most filters always return `true` - /// from `event_enabled` and rely on `[callsite_]enabled` for filtering. + /// This inverts the values returned by the [`enabled`] and [`callsite_enabled`] + /// methods on the wrapped filter; it does *not* invert [`event_enabled`], as + /// filters which do not implement filtering on event field values will return + /// the default `true` even for events that their [`enabled`] method disables. /// - /// Essentially, where a normal filter is roughly + /// Consider a normal filter defined as: /// /// ```ignore (psuedo-code) /// // for spans @@ -424,7 +425,7 @@ where /// } /// ``` /// - /// an inverted filter is roughly + /// and an inverted filter defined as: /// /// ```ignore (psuedo-code) /// // for spans @@ -442,9 +443,9 @@ where /// ``` /// /// A proper inversion would do `!(enabled() && event_enabled())` (or - /// equivalently `!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`. + /// `!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 diff --git a/tracing-subscriber/src/filter/subscriber_filters/mod.rs b/tracing-subscriber/src/filter/subscriber_filters/mod.rs index 21ec8a7bd7..ff0fb49e57 100644 --- a/tracing-subscriber/src/filter/subscriber_filters/mod.rs +++ b/tracing-subscriber/src/filter/subscriber_filters/mod.rs @@ -299,11 +299,12 @@ pub trait FilterExt: subscribe::Filter { /// Inverts `self`, returning a filter that enables spans and events only if /// `self` would *not* enable them. /// - /// The wrapped filter's [`callsite_enabled`] and [`enabled`] are inverted, - /// but [`event_enabled`] is ignored, as most filters always return `true` - /// from `event_enabled` and rely on `[callsite_]enabled` for filtering. + /// This inverts the values returned by the [`enabled`] and [`callsite_enabled`] + /// methods on the wrapped filter; it does *not* invert [`event_enabled`], as + /// filters which do not implement filtering on event field values will return + /// the default `true` even for events that their [`enabled`] method disables. /// - /// Essentially, where a normal filter is roughly + /// Consider a normal filter defined as: /// /// ```ignore (psuedo-code) /// // for spans @@ -320,7 +321,7 @@ pub trait FilterExt: subscribe::Filter { /// } /// ``` /// - /// an inverted filter is roughly + /// and an inverted filter defined as: /// /// ```ignore (psuedo-code) /// // for spans @@ -338,9 +339,9 @@ pub trait FilterExt: subscribe::Filter { /// ``` /// /// A proper inversion would do `!(enabled() && event_enabled())` (or - /// equivalently `!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`. + /// `!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 From cf3cacd3708513ce6d1b49a0c7d7cda2b39ad64d Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Thu, 28 Jul 2022 14:32:41 -0700 Subject: [PATCH 4/4] fix spelling of "pseudo" --- .../src/filter/subscriber_filters/combinator.rs | 4 ++-- tracing-subscriber/src/filter/subscriber_filters/mod.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tracing-subscriber/src/filter/subscriber_filters/combinator.rs b/tracing-subscriber/src/filter/subscriber_filters/combinator.rs index 713ca41788..7ccf011cb7 100644 --- a/tracing-subscriber/src/filter/subscriber_filters/combinator.rs +++ b/tracing-subscriber/src/filter/subscriber_filters/combinator.rs @@ -410,7 +410,7 @@ where /// /// Consider a normal filter defined as: /// - /// ```ignore (psuedo-code) + /// ```ignore (pseudo-code) /// // for spans /// match callsite_enabled() { /// ALWAYS => on_span(), @@ -427,7 +427,7 @@ where /// /// and an inverted filter defined as: /// - /// ```ignore (psuedo-code) + /// ```ignore (pseudo-code) /// // for spans /// match callsite_enabled() { /// ALWAYS => (), diff --git a/tracing-subscriber/src/filter/subscriber_filters/mod.rs b/tracing-subscriber/src/filter/subscriber_filters/mod.rs index ff0fb49e57..9b84477953 100644 --- a/tracing-subscriber/src/filter/subscriber_filters/mod.rs +++ b/tracing-subscriber/src/filter/subscriber_filters/mod.rs @@ -306,7 +306,7 @@ pub trait FilterExt: subscribe::Filter { /// /// Consider a normal filter defined as: /// - /// ```ignore (psuedo-code) + /// ```ignore (pseudo-code) /// // for spans /// match callsite_enabled() { /// ALWAYS => on_span(), @@ -323,7 +323,7 @@ pub trait FilterExt: subscribe::Filter { /// /// and an inverted filter defined as: /// - /// ```ignore (psuedo-code) + /// ```ignore (pseudo-code) /// // for spans /// match callsite_enabled() { /// ALWAYS => (),