Skip to content

Commit

Permalink
subscriber: add Filter::event_enabled (#2245)
Browse files Browse the repository at this point in the history
## Motivation

Like for `Subscriber` and `Layer`, allow per-layer `Filter`s to filter
based on event fields.

## Solution

Add `Filter::event_enabled`, plumb it through the combinator
implementations, and call it from `Filtered`.

The bit I'm the least confident about is the check in `Registry`'s
implementation, but I *think* it matches what `event` is doing and
everything seems to function correctly.
  • Loading branch information
CAD97 authored and hawkw committed Jul 29, 2022
1 parent 27ffce2 commit 09da422
Show file tree
Hide file tree
Showing 6 changed files with 151 additions and 0 deletions.
27 changes: 27 additions & 0 deletions tracing-subscriber/src/filter/layer_filters/combinator.rs
Expand Up @@ -137,6 +137,11 @@ where
cmp::min(self.a.max_level_hint(), self.b.max_level_hint())
}

#[inline]
fn event_enabled(&self, event: &tracing_core::Event<'_>, cx: &Context<'_, S>) -> bool {
self.a.event_enabled(event, cx) && self.b.event_enabled(event, cx)
}

#[inline]
fn on_new_span(&self, attrs: &Attributes<'_>, id: &Id, ctx: Context<'_, S>) {
self.a.on_new_span(attrs, id, ctx.clone());
Expand Down Expand Up @@ -324,6 +329,11 @@ where
Some(cmp::max(self.a.max_level_hint()?, self.b.max_level_hint()?))
}

#[inline]
fn event_enabled(&self, event: &tracing_core::Event<'_>, cx: &Context<'_, S>) -> bool {
self.a.event_enabled(event, cx) || self.b.event_enabled(event, cx)
}

#[inline]
fn on_new_span(&self, attrs: &Attributes<'_>, id: &Id, ctx: Context<'_, S>) {
self.a.on_new_span(attrs, id, ctx.clone());
Expand Down Expand Up @@ -393,7 +403,16 @@ 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.
///
/// [`Filter`]: crate::layer::Filter
/// [`enabled`]: crate::layer::Filter::enabled
/// [`event_enabled`]: crate::layer::Filter::event_enabled
/// [`callsite_enabled`]: crate::layer::Filter::callsite_enabled
pub(crate) fn new(a: A) -> Self {
Self { a, _s: PhantomData }
}
Expand Down Expand Up @@ -421,6 +440,14 @@ where
None
}

#[inline]
fn event_enabled(&self, event: &tracing_core::Event<'_>, cx: &Context<'_, S>) -> bool {
// Never disable based on event_enabled; we "disabled" it in `enabled`,
// so the `not` has already been applied and filtered this not out.
let _ = (event, cx);
true
}

#[inline]
fn on_new_span(&self, attrs: &Attributes<'_>, id: &Id, ctx: Context<'_, S>) {
self.a.on_new_span(attrs, id, ctx);
Expand Down
35 changes: 35 additions & 0 deletions tracing-subscriber/src/filter/layer_filters/mod.rs
Expand Up @@ -298,6 +298,17 @@ pub trait FilterExt<S>: layer::Filter<S> {

/// 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.
///
/// [`Filter`]: crate::subscribe::Filter
/// [`enabled`]: crate::subscribe::Filter::enabled
/// [`event_enabled`]: crate::subscribe::Filter::event_enabled
/// [`callsite_enabled`]: crate::subscribe::Filter::callsite_enabled
fn not(self) -> combinator::Not<Self, S>
where
Self: Sized,
Expand Down Expand Up @@ -643,6 +654,22 @@ where
}
}

fn event_enabled(&self, event: &Event<'_>, cx: Context<'_, S>) -> bool {
let cx = cx.with_filter(self.id());
let enabled = FILTERING
.with(|filtering| filtering.and(self.id(), || self.filter.event_enabled(event, &cx)));

if enabled {
// If the filter enabled this event, ask the wrapped subscriber if
// _it_ wants it --- it might have a global filter.
self.layer.event_enabled(event, cx)
} else {
// Otherwise, return `true`. See the comment in `enabled` for why this
// is necessary.
true
}
}

fn on_event(&self, event: &Event<'_>, cx: Context<'_, S>) {
self.did_enable(|| {
self.layer.on_event(event, cx.with_filter(self.id()));
Expand Down Expand Up @@ -1014,6 +1041,14 @@ impl FilterState {
}
}

/// Run a second filtering pass, e.g. for Subscribe::event_enabled.
fn and(&self, filter: FilterId, f: impl FnOnce() -> bool) -> bool {
let map = self.enabled.get();
let enabled = map.is_enabled(filter) && f();
self.enabled.set(map.set(filter, enabled));
enabled
}

/// Clears the current in-progress filter state.
///
/// This resets the [`FilterMap`] and current [`Interest`] as well as
Expand Down
20 changes: 20 additions & 0 deletions tracing-subscriber/src/layer/mod.rs
Expand Up @@ -1352,6 +1352,26 @@ feature! {
Interest::sometimes()
}

/// Called before the filtered [`Layer]'s [`on_event`], to determine if
/// `on_event` should be called.
///
/// This gives a chance to filter events based on their fields. Note,
/// however, that this *does not* override [`enabled`], and is not even
/// called if [`enabled`] returns `false`.
///
/// ## Default Implementation
///
/// By default, this method returns `true`, indicating that no events are
/// filtered out based on their fields.
///
/// [`enabled`]: crate::layer::Filter::enabled
/// [`on_event`]: crate::layer::Layer::on_event
#[inline] // collapse this to a constant please mrs optimizer
fn event_enabled(&self, event: &Event<'_>, cx: &Context<'_, S>) -> bool {
let _ = (event, cx);
true
}

/// Returns an optional hint of the highest [verbosity level][level] that
/// this `Filter` will enable.
///
Expand Down
7 changes: 7 additions & 0 deletions tracing-subscriber/src/registry/sharded.rs
Expand Up @@ -275,6 +275,13 @@ impl Subscriber for Registry {

fn record_follows_from(&self, _span: &span::Id, _follows: &span::Id) {}

fn event_enabled(&self, _event: &Event<'_>) -> bool {
if self.has_per_layer_filters() {
return FilterState::event_enabled();
}
true
}

/// This is intentionally not implemented, as recording events
/// is the responsibility of layers atop of this registry.
fn event(&self, _: &Event<'_>) {}
Expand Down
1 change: 1 addition & 0 deletions tracing-subscriber/tests/layer_filters/main.rs
Expand Up @@ -5,6 +5,7 @@ use self::support::*;
mod boxed;
mod downcast_raw;
mod filter_scopes;
mod per_event;
mod targets;
mod trees;
mod vec;
Expand Down
61 changes: 61 additions & 0 deletions tracing-subscriber/tests/layer_filters/per_event.rs
@@ -0,0 +1,61 @@
use crate::support::*;
use tracing::Level;
use tracing_subscriber::{field::Visit, layer::Filter, prelude::*};

struct FilterEvent;

impl<S> Filter<S> for FilterEvent {
fn enabled(
&self,
_meta: &tracing::Metadata<'_>,
_cx: &tracing_subscriber::layer::Context<'_, S>,
) -> bool {
true
}

fn event_enabled(
&self,
event: &tracing::Event<'_>,
_cx: &tracing_subscriber::layer::Context<'_, S>,
) -> bool {
struct ShouldEnable(bool);
impl Visit for ShouldEnable {
fn record_bool(&mut self, field: &tracing_core::Field, value: bool) {
if field.name() == "enable" {
self.0 = value;
}
}

fn record_debug(
&mut self,
_field: &tracing_core::Field,
_value: &dyn core::fmt::Debug,
) {
}
}
let mut should_enable = ShouldEnable(false);
event.record(&mut should_enable);
should_enable.0
}
}

#[test]
fn per_subscriber_event_field_filtering() {
let (expect, handle) = layer::mock()
.event(event::mock().at_level(Level::TRACE))
.event(event::mock().at_level(Level::INFO))
.done()
.run_with_handle();

let _subscriber = tracing_subscriber::registry()
.with(expect.with_filter(FilterEvent))
.set_default();

tracing::trace!(enable = true, "hello trace");
tracing::debug!("hello debug");
tracing::info!(enable = true, "hello info");
tracing::warn!(enable = false, "hello warn");
tracing::error!("hello error");

handle.assert_finished();
}

0 comments on commit 09da422

Please sign in to comment.