Skip to content

Commit

Permalink
subscriber: fix max_level_hint for empty Option/Vec Subscribe
Browse files Browse the repository at this point in the history
… impls (#2195)

## Motivation

These are incorrect: currently, when you have a `None` layer, the `None`
hint it returns causes the default `TRACE` to win, which is inaccurate.
Similarly, `Vec` should default to `OFF` if it has no `Subscribe`s in it

## Solution

Change the default hints to `Some(OFF)`

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
  • Loading branch information
guswynn and hawkw authored Jul 1, 2022
1 parent 665ad11 commit 2c568b5
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 2 deletions.
9 changes: 7 additions & 2 deletions tracing-subscriber/src/subscribe/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1485,7 +1485,11 @@ where
fn max_level_hint(&self) -> Option<LevelFilter> {
match self {
Some(ref inner) => inner.max_level_hint(),
None => None,
None => {
// There is no inner subscriber, so this subscriber will
// never enable anything.
Some(LevelFilter::OFF)
}
}
}

Expand Down Expand Up @@ -1690,7 +1694,8 @@ feature! {
}

fn max_level_hint(&self) -> Option<LevelFilter> {
let mut max_level = LevelFilter::ERROR;
// Default to `OFF` if there are no underlying subscribers
let mut max_level = LevelFilter::OFF;
for s in self {
// NOTE(eliza): this is slightly subtle: if *any* subscriber
// returns `None`, we have to return `None`, assuming there is
Expand Down
41 changes: 41 additions & 0 deletions tracing-subscriber/tests/option.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#![cfg(feature = "registry")]
use tracing::level_filters::LevelFilter;
use tracing::Collect;
use tracing_subscriber::prelude::*;

// This test is just used to compare to the tests below
#[test]
fn just_subscriber() {
let collector = tracing_subscriber::registry().with(LevelFilter::INFO);
assert_eq!(collector.max_level_hint(), Some(LevelFilter::INFO));
}

#[test]
fn subscriber_and_option_some_subscriber() {
let collector = tracing_subscriber::registry()
.with(LevelFilter::INFO)
.with(Some(LevelFilter::DEBUG));
assert_eq!(collector.max_level_hint(), Some(LevelFilter::DEBUG));
}

#[test]
fn subscriber_and_option_none_subscriber() {
// None means the other layer takes control
let collector = tracing_subscriber::registry()
.with(LevelFilter::ERROR)
.with(None::<LevelFilter>);
assert_eq!(collector.max_level_hint(), Some(LevelFilter::ERROR));
}

#[test]
fn just_option_some_subscriber() {
// Just a None means everything is off
let collector = tracing_subscriber::registry().with(None::<LevelFilter>);
assert_eq!(collector.max_level_hint(), Some(LevelFilter::OFF));
}

#[test]
fn just_option_none_subscriber() {
let collector = tracing_subscriber::registry().with(Some(LevelFilter::ERROR));
assert_eq!(collector.max_level_hint(), Some(LevelFilter::ERROR));
}
7 changes: 7 additions & 0 deletions tracing-subscriber/tests/subscriber_filters/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,10 @@ fn all_filtered_max_level_hint() {

assert_eq!(collector.max_level_hint(), Some(LevelFilter::DEBUG));
}

#[test]
fn empty_vec() {
// Just a None means everything is off
let collector = tracing_subscriber::registry().with(Vec::<ExpectSubscriber>::new());
assert_eq!(collector.max_level_hint(), Some(LevelFilter::OFF));
}
19 changes: 19 additions & 0 deletions tracing-subscriber/tests/vec.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#![cfg(feature = "registry")]
use tracing::level_filters::LevelFilter;
use tracing::Collect;
use tracing_subscriber::prelude::*;

#[test]
fn just_empty_vec() {
// Just a None means everything is off
let collector = tracing_subscriber::registry().with(Vec::<LevelFilter>::new());
assert_eq!(collector.max_level_hint(), Some(LevelFilter::OFF));
}

#[test]
fn subscriber_and_empty_vec() {
let collector = tracing_subscriber::registry()
.with(LevelFilter::INFO)
.with(Vec::<LevelFilter>::new());
assert_eq!(collector.max_level_hint(), Some(LevelFilter::INFO));
}

0 comments on commit 2c568b5

Please sign in to comment.