From 2c568b5a106664c3e2bffd56964fe46a4b8e4c4c Mon Sep 17 00:00:00 2001
From: Gus Wynn <guswynn@gmail.com>
Date: Thu, 30 Jun 2022 17:15:07 -0700
Subject: [PATCH] subscriber: fix `max_level_hint` for empty `Option`/`Vec`
 `Subscribe` 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>
---
 tracing-subscriber/src/subscribe/mod.rs       |  9 +++-
 tracing-subscriber/tests/option.rs            | 41 +++++++++++++++++++
 .../tests/subscriber_filters/vec.rs           |  7 ++++
 tracing-subscriber/tests/vec.rs               | 19 +++++++++
 4 files changed, 74 insertions(+), 2 deletions(-)
 create mode 100644 tracing-subscriber/tests/option.rs
 create mode 100644 tracing-subscriber/tests/vec.rs

diff --git a/tracing-subscriber/src/subscribe/mod.rs b/tracing-subscriber/src/subscribe/mod.rs
index 0d98cb52ce..6c1878b177 100644
--- a/tracing-subscriber/src/subscribe/mod.rs
+++ b/tracing-subscriber/src/subscribe/mod.rs
@@ -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)
+            }
         }
     }
 
@@ -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
diff --git a/tracing-subscriber/tests/option.rs b/tracing-subscriber/tests/option.rs
new file mode 100644
index 0000000000..a0f127e286
--- /dev/null
+++ b/tracing-subscriber/tests/option.rs
@@ -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));
+}
diff --git a/tracing-subscriber/tests/subscriber_filters/vec.rs b/tracing-subscriber/tests/subscriber_filters/vec.rs
index a0a971cf51..4823a5e4a0 100644
--- a/tracing-subscriber/tests/subscriber_filters/vec.rs
+++ b/tracing-subscriber/tests/subscriber_filters/vec.rs
@@ -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));
+}
diff --git a/tracing-subscriber/tests/vec.rs b/tracing-subscriber/tests/vec.rs
new file mode 100644
index 0000000000..4ea9c3db87
--- /dev/null
+++ b/tracing-subscriber/tests/vec.rs
@@ -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));
+}