From 9284e1045ea9a547f6d186c80ce26c3ce95684a6 Mon Sep 17 00:00:00 2001 From: Gus Wynn Date: Wed, 15 Jun 2022 11:35:43 -0700 Subject: [PATCH] subscriber: implement `Filter` for `reload::Layer` (#2159) ## Motivation The `reload` layer doesn't (and can't) implement downcasting correctly, which breaks certain layers like the opentelemetry one. ## Solution Most uses of the `reload` module (including mine) are just to change the filter. Therefore, this PR implements `Filter` for `reload::Layer` to allow users to not need to wrap the whole layer trait. Another advantage of this is that the common-case critical sections are shortened. --- tracing-subscriber/src/reload.rs | 97 ++++++++++++++++++++++++------ tracing-subscriber/tests/reload.rs | 59 ++++++++++++++++++ 2 files changed, 136 insertions(+), 20 deletions(-) diff --git a/tracing-subscriber/src/reload.rs b/tracing-subscriber/src/reload.rs index 422917a413..b36be35878 100644 --- a/tracing-subscriber/src/reload.rs +++ b/tracing-subscriber/src/reload.rs @@ -1,8 +1,8 @@ //! Wrapper for a `Layer` to allow it to be dynamically reloaded. //! //! This module provides a [`Layer` type] which wraps another type implementing -//! the [`Layer` trait], allowing the wrapped type to be replaced with another -//! instance of that type at runtime. +//! the [`Layer`][layer-trait] or [`Filter`] traits, allowing the wrapped type +//! to be replaced with another instance of that type at runtime. //! //! This can be used in cases where a subset of `Subscriber` functionality //! should be dynamically reconfigured, such as when filtering directives may @@ -52,8 +52,17 @@ //! info!("This will be logged"); //! ``` //! +//! ## Note +//! +//! The [`Layer`][layer-trait] implementation is unable to implement downcasting functionality, +//! so certain `Layer`s will fail to reload if wrapped in a `reload::Layer`. +//! +//! If you only want to be able to dynamically change the +//! [`Filter`] on your layer, prefer wrapping that [`Filter`] in the `reload::Layer`. +//! //! [`Layer` type]: Layer -//! [`Layer` trait]: super::layer::Layer +//! [layer-trait]: super::layer::Layer +//! [`Filter`]: super::layer::Filter use crate::layer; use crate::sync::RwLock; @@ -68,7 +77,11 @@ use tracing_core::{ Event, Metadata, }; -/// Wraps a `Layer`, allowing it to be reloaded dynamically at runtime. +/// Wraps a [`Layer`] or [`Filter`], allowing it to be reloaded dynamically at +/// runtime. +/// +/// [`Filter`]: crate::layer::Filter +/// [`Layer`]: crate::Layer #[derive(Debug)] pub struct Layer { // TODO(eliza): this once used a `crossbeam_util::ShardedRwLock`. We may @@ -160,13 +173,56 @@ where } } -impl Layer +#[cfg(all(feature = "registry", feature = "std"))] +#[cfg_attr(docsrs, doc(cfg(all(feature = "registry", feature = "std"))))] +impl crate::layer::Filter for Layer where - L: crate::Layer + 'static, + F: crate::layer::Filter + 'static, S: Subscriber, { - /// Wraps the given `Layer`, returning a `Layer` and a `Handle` that allows + #[inline] + fn callsite_enabled(&self, metadata: &'static Metadata<'static>) -> Interest { + try_lock!(self.inner.read(), else return Interest::sometimes()).callsite_enabled(metadata) + } + + #[inline] + fn enabled(&self, metadata: &Metadata<'_>, ctx: &layer::Context<'_, S>) -> bool { + try_lock!(self.inner.read(), else return false).enabled(metadata, ctx) + } + + #[inline] + fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id, ctx: layer::Context<'_, S>) { + try_lock!(self.inner.read()).on_new_span(attrs, id, ctx) + } + + #[inline] + fn on_record(&self, span: &span::Id, values: &span::Record<'_>, ctx: layer::Context<'_, S>) { + try_lock!(self.inner.read()).on_record(span, values, ctx) + } + + #[inline] + fn on_enter(&self, id: &span::Id, ctx: layer::Context<'_, S>) { + try_lock!(self.inner.read()).on_enter(id, ctx) + } + + #[inline] + fn on_exit(&self, id: &span::Id, ctx: layer::Context<'_, S>) { + try_lock!(self.inner.read()).on_exit(id, ctx) + } + + #[inline] + fn on_close(&self, id: span::Id, ctx: layer::Context<'_, S>) { + try_lock!(self.inner.read()).on_close(id, ctx) + } +} + +impl Layer { + /// Wraps the given `Subscribe` or `Filter`, + /// returning a subscriber or filter and a `Handle` that allows /// the inner type to be modified at runtime. + /// + /// [`Filter`]: crate::subscribe::Filter + /// [`Subscribe`]: crate::Subscribe pub fn new(inner: L) -> (Self, Handle) { let this = Self { inner: Arc::new(RwLock::new(inner)), @@ -187,20 +243,21 @@ where // ===== impl Handle ===== -impl Handle -where - L: crate::Layer + 'static, - S: Subscriber, -{ - /// Replace the current layer with the provided `new_layer`. +impl Handle { + /// Replace the current subscriber or filter with the provided `new_value`. + /// + /// [`Handle::reload`] cannot be used with the [`Filtered`] `Layer`; use + /// [`Handle::modify`] instead (see [this issue] for additional details). + /// + /// However, if the _only_ the [`Filter`] needs to be modified, + /// use `reload::Layer` to wrap the `Filter` directly. /// - /// **Warning:** The [`Filtered`](crate::filter::Filtered) type currently can't be changed - /// at runtime via the [`Handle::reload`] method. - /// Use the [`Handle::modify`] method to change the filter instead. - /// (see ) - pub fn reload(&self, new_layer: impl Into) -> Result<(), Error> { - self.modify(|layer| { - *layer = new_layer.into(); + /// [`Filtered`]: crate::filter::Filtered + /// [`Filter`]: crate::layer::Filter + /// [this issue]: https://github.com/tokio-rs/tracing/issues/1629 + pub fn reload(&self, new_value: impl Into) -> Result<(), Error> { + self.modify(|object| { + *object = new_value.into(); }) } diff --git a/tracing-subscriber/tests/reload.rs b/tracing-subscriber/tests/reload.rs index 5fe422e085..a89915d36b 100644 --- a/tracing-subscriber/tests/reload.rs +++ b/tracing-subscriber/tests/reload.rs @@ -29,6 +29,17 @@ impl Subscriber for NopSubscriber { fn exit(&self, _: &Id) {} } +pub struct NopSubscriber; +impl tracing_subscriber::Subscribe for NopSubscriber { + fn register_callsite(&self, _m: &Metadata<'_>) -> Interest { + Interest::sometimes() + } + + fn enabled(&self, _m: &Metadata<'_>, _: subscribe::Context<'_, S>) -> bool { + true + } +} + #[test] fn reload_handle() { static FILTER1_CALLS: AtomicUsize = AtomicUsize::new(0); @@ -79,3 +90,51 @@ fn reload_handle() { assert_eq!(FILTER2_CALLS.load(Ordering::SeqCst), 1); }) } + +#[test] +fn reload_filter() { + static FILTER1_CALLS: AtomicUsize = AtomicUsize::new(0); + static FILTER2_CALLS: AtomicUsize = AtomicUsize::new(0); + + enum Filter { + One, + Two, + } + + impl tracing_subscriber::subscribe::Filter for Filter { + fn enabled(&self, m: &Metadata<'_>, _: &subscribe::Context<'_, S>) -> bool { + println!("ENABLED: {:?}", m); + match self { + Filter::One => FILTER1_CALLS.fetch_add(1, Ordering::SeqCst), + Filter::Two => FILTER2_CALLS.fetch_add(1, Ordering::SeqCst), + }; + true + } + } + fn event() { + tracing::trace!("my event"); + } + + let (filter, handle) = Subscriber::new(Filter::One); + + let dispatcher = tracing_core::dispatch::Dispatch::new( + tracing_subscriber::registry().with(NopSubscriber.with_filter(filter)), + ); + + tracing_core::dispatch::with_default(&dispatcher, || { + assert_eq!(FILTER1_CALLS.load(Ordering::SeqCst), 0); + assert_eq!(FILTER2_CALLS.load(Ordering::SeqCst), 0); + + event(); + + assert_eq!(FILTER1_CALLS.load(Ordering::SeqCst), 1); + assert_eq!(FILTER2_CALLS.load(Ordering::SeqCst), 0); + + handle.reload(Filter::Two).expect("should reload"); + + event(); + + assert_eq!(FILTER1_CALLS.load(Ordering::SeqCst), 1); + assert_eq!(FILTER2_CALLS.load(Ordering::SeqCst), 1); + }) +}