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.

Author: guswynn

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<L, S> {
     // TODO(eliza): this once used a `crossbeam_util::ShardedRwLock`. We may
@@ -160,13 +173,56 @@ where
     }
 }
 
-impl<L, S> Layer<L, S>
+#[cfg(all(feature = "registry", feature = "std"))]
+#[cfg_attr(docsrs, doc(cfg(all(feature = "registry", feature = "std"))))]
+impl<F, S> crate::layer::Filter<S> for Layer<F, S>
 where
-    L: crate::Layer<S> + 'static,
+    F: crate::layer::Filter<S> + '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<L, S> Layer<L, S> {
+    /// 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<L, S>) {
         let this = Self {
             inner: Arc::new(RwLock::new(inner)),
@@ -187,20 +243,21 @@ where
 
 // ===== impl Handle =====
 
-impl<L, S> Handle<L, S>
-where
-    L: crate::Layer<S> + 'static,
-    S: Subscriber,
-{
-    /// Replace the current layer with the provided `new_layer`.
+impl<L, S> Handle<L, S> {
+    /// 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 <https://github.com/tokio-rs/tracing/issues/1629>)
-    pub fn reload(&self, new_layer: impl Into<L>) -> 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<L>) -> Result<(), Error> {
+        self.modify(|object| {
+            *object = new_value.into();
         })
     }
 

tracing-subscriber/tests/reload.rs

@@ -29,6 +29,17 @@ impl Subscriber for NopSubscriber {
     fn exit(&self, _: &Id) {}
 }
 
+pub struct NopSubscriber;
+impl<S: Collect> tracing_subscriber::Subscribe<S> 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<S: Collect> tracing_subscriber::subscribe::Filter<S> 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);
+    })
+}