Skip to content

Commit e0b3f79

Browse files
guswynnhawkw
andcommitted
subscriber: fix max_level_hint for empty Option/Vec Layer 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 `Layer`s in it ## Solution Change the default hints to `Some(OFF)` Co-authored-by: Eliza Weisman <eliza@buoyant.io>
1 parent 0012a41 commit e0b3f79

File tree

4 files changed

+74
-2
lines changed

4 files changed

+74
-2
lines changed

tracing-subscriber/src/layer/mod.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1490,7 +1490,11 @@ where
14901490
fn max_level_hint(&self) -> Option<LevelFilter> {
14911491
match self {
14921492
Some(ref inner) => inner.max_level_hint(),
1493-
None => None,
1493+
None => {
1494+
// There is no inner layer, so this layer will
1495+
// never enable anything.
1496+
Some(LevelFilter::OFF)
1497+
}
14941498
}
14951499
}
14961500

@@ -1701,7 +1705,8 @@ feature! {
17011705
}
17021706

17031707
fn max_level_hint(&self) -> Option<LevelFilter> {
1704-
let mut max_level = LevelFilter::ERROR;
1708+
// Default to `OFF` if there are no inner layers.
1709+
let mut max_level = LevelFilter::OFF;
17051710
for l in self {
17061711
// NOTE(eliza): this is slightly subtle: if *any* layer
17071712
// returns `None`, we have to return `None`, assuming there is

tracing-subscriber/tests/layer_filters/vec.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,3 +111,10 @@ fn all_filtered_max_level_hint() {
111111

112112
assert_eq!(subscriber.max_level_hint(), Some(LevelFilter::DEBUG));
113113
}
114+
115+
#[test]
116+
fn empty_vec() {
117+
// Just a None means everything is off
118+
let subscriber = tracing_subscriber::registry().with(Vec::<ExpectLayer>::new());
119+
assert_eq!(subscriber.max_level_hint(), Some(LevelFilter::OFF));
120+
}

tracing-subscriber/tests/option.rs

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
#![cfg(feature = "registry")]
2+
use tracing::level_filters::LevelFilter;
3+
use tracing::Subscriber;
4+
use tracing_subscriber::prelude::*;
5+
6+
// This test is just used to compare to the tests below
7+
#[test]
8+
fn just_layer() {
9+
let subscriber = tracing_subscriber::registry().with(LevelFilter::INFO);
10+
assert_eq!(subscriber.max_level_hint(), Some(LevelFilter::INFO));
11+
}
12+
13+
#[test]
14+
fn subscriber_and_option_some_layer() {
15+
let subscriber = tracing_subscriber::registry()
16+
.with(LevelFilter::INFO)
17+
.with(Some(LevelFilter::DEBUG));
18+
assert_eq!(subscriber.max_level_hint(), Some(LevelFilter::DEBUG));
19+
}
20+
21+
#[test]
22+
fn subscriber_and_option_none_layer() {
23+
// None means the other layer takes control
24+
let subscriber = tracing_subscriber::registry()
25+
.with(LevelFilter::ERROR)
26+
.with(None::<LevelFilter>);
27+
assert_eq!(subscriber.max_level_hint(), Some(LevelFilter::ERROR));
28+
}
29+
30+
#[test]
31+
fn just_option_some_layer() {
32+
// Just a None means everything is off
33+
let subscriber = tracing_subscriber::registry().with(None::<LevelFilter>);
34+
assert_eq!(subscriber.max_level_hint(), Some(LevelFilter::OFF));
35+
}
36+
37+
#[test]
38+
fn just_option_none_layer() {
39+
let subscriber = tracing_subscriber::registry().with(Some(LevelFilter::ERROR));
40+
assert_eq!(subscriber.max_level_hint(), Some(LevelFilter::ERROR));
41+
}

tracing-subscriber/tests/vec.rs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
#![cfg(feature = "registry")]
2+
use tracing::level_filters::LevelFilter;
3+
use tracing::Subscriber;
4+
use tracing_subscriber::prelude::*;
5+
6+
#[test]
7+
fn just_empty_vec() {
8+
// Just a None means everything is off
9+
let subscriber = tracing_subscriber::registry().with(Vec::<LevelFilter>::new());
10+
assert_eq!(subscriber.max_level_hint(), Some(LevelFilter::OFF));
11+
}
12+
13+
#[test]
14+
fn layer_and_empty_vec() {
15+
let subscriber = tracing_subscriber::registry()
16+
.with(LevelFilter::INFO)
17+
.with(Vec::<LevelFilter>::new());
18+
assert_eq!(subscriber.max_level_hint(), Some(LevelFilter::INFO));
19+
}

0 commit comments

Comments
 (0)