diff --git a/tracing-subscriber/src/fmt/mod.rs b/tracing-subscriber/src/fmt/mod.rs index 936ecd6400..ec1d899e47 100644 --- a/tracing-subscriber/src/fmt/mod.rs +++ b/tracing-subscriber/src/fmt/mod.rs @@ -480,9 +480,9 @@ where #[must_use = "you may want to use `try_init` or similar to actually install the collector."] pub fn finish(self) -> Collector { let collector = self.inner.with_collector(Registry::default()); - Collector { - inner: self.filter.with_collector(collector), - } + let mut inner = self.filter.with_collector(collector); + inner.inner_is_registry = true; + Collector { inner } } /// Install this collector as the global default if one is diff --git a/tracing-subscriber/src/subscribe/layered.rs b/tracing-subscriber/src/subscribe/layered.rs index 787472dea7..c8ca1195b3 100644 --- a/tracing-subscriber/src/subscribe/layered.rs +++ b/tracing-subscriber/src/subscribe/layered.rs @@ -39,9 +39,8 @@ pub struct Layered { // level hints when per-subscriber filters are in use. /// Is `self.inner` a `Registry`? /// - /// If so, when combining `Interest`s, we want to "bubble up" its - /// `Interest`. - inner_is_registry: bool, + /// If so, we ignore the subscriber when returning a `max_level_hint` + pub(crate) inner_is_registry: bool, /// Does `self.subscriber` have per-subscriber filters? /// @@ -482,6 +481,8 @@ where return outer_hint; } + // This is the same as the fallthrough, but we want it to happen + // before we check the individual cases below. if self.has_subscriber_filter && self.inner_has_subscriber_filter { return Some(cmp::max(outer_hint?, inner_hint?)); } @@ -494,7 +495,9 @@ where return None; } - cmp::max(outer_hint, inner_hint) + // `None` is basically == `Some(TRACE)` so we have to short + // circuit, instead of using `Option`s `PartialOrd` impl. + Some(cmp::max(outer_hint?, inner_hint?)) } } diff --git a/tracing-subscriber/tests/option.rs b/tracing-subscriber/tests/option.rs index a0f127e286..2079c5185e 100644 --- a/tracing-subscriber/tests/option.rs +++ b/tracing-subscriber/tests/option.rs @@ -1,7 +1,22 @@ #![cfg(feature = "registry")] -use tracing::level_filters::LevelFilter; -use tracing::Collect; -use tracing_subscriber::prelude::*; +use tracing_core::{collect::Interest, Collect, LevelFilter, Metadata}; +use tracing_subscriber::{prelude::*, subscribe}; + +// A basic layer that returns its inner for `max_level_hint` +struct BasicLayer(Option); +impl tracing_subscriber::Subscribe for BasicLayer { + fn register_callsite(&self, _m: &Metadata<'_>) -> Interest { + Interest::sometimes() + } + + fn enabled(&self, _m: &Metadata<'_>, _: subscribe::Context<'_, C>) -> bool { + true + } + + fn max_level_hint(&self) -> Option { + self.0 + } +} // This test is just used to compare to the tests below #[test] @@ -39,3 +54,26 @@ fn just_option_none_subscriber() { let collector = tracing_subscriber::registry().with(Some(LevelFilter::ERROR)); assert_eq!(collector.max_level_hint(), Some(LevelFilter::ERROR)); } + +/// Test that the `Layered` impl fallthrough the `max_level_hint` and `None` doesn't disable +/// everything, as well as ensuring the `Some` case also works. +#[test] +fn layered_fallthrough() { + // None means the other layer takes control + let subscriber = tracing_subscriber::registry() + .with(BasicLayer(None)) + .with(None::); + assert_eq!(subscriber.max_level_hint(), None); + + // The `None`-returning layer still wins + let subscriber = tracing_subscriber::registry() + .with(BasicLayer(None)) + .with(Some(LevelFilter::ERROR)); + assert_eq!(subscriber.max_level_hint(), None); + + // Check that we aren't doing anything truly wrong + let subscriber = tracing_subscriber::registry() + .with(BasicLayer(Some(LevelFilter::DEBUG))) + .with(None::); + assert_eq!(subscriber.max_level_hint(), Some(LevelFilter::DEBUG)); +}