subscriber: unify span traversal (#1431)

nightkr · web-flow · commit 7a012603eaee · 2021-06-22T10:33:42.000-07:00
## Motivation Fixes #1429 ## Solution Implemented as described in #1429, and migrated all internal uses of the deprecated methods. All tests passed both before and after the migration (9ec8130). - Add a new method `SpanRef::scope(&self)` that returns a leaf-to-root `Iterator`, including the specified leaf - Add a new method `Scope::from_root(self)` (where `Scope` is the type returned by `SpanRef::scope` defined earlier) that returns a root-to-leaf `Iterator` that ends at the current leaf (in other words: it's essentially the same as `Scope::collect::<Vec<_>>().into_iter().rev()`) - Deprecate all existing iterators, since they can be replaced by the new unified mechanism: - `Span::parents` is equivalent to `Span::scope().skip(1)` (although the `skip` is typically a bug) - `Span::from_root` is equivalent to `Span::scope().skip(1).from_root()` (although the `skip` is typically a bug) - `Context::scope` is equivalent to `Context::lookup_current().scope().from_root()` (although the `lookup_current` is sometimes a bug, see also #1428)
diff --git a/examples/examples/sloggish/sloggish_subscriber.rs b/examples/examples/sloggish/sloggish_subscriber.rs
@@ -133,7 +133,6 @@ impl<'a> Visit for Event<'a> {
                 Style::new().bold().paint(format!("{:?}", value))
             )
             .unwrap();
-            self.comma = true;
         } else {
             write!(
                 &mut self.stderr,
@@ -142,8 +141,8 @@ impl<'a> Visit for Event<'a> {
                 value
             )
             .unwrap();
-            self.comma = true;
         }
+        self.comma = true;
     }
 }
 
diff --git a/tracing-core/src/metadata.rs b/tracing-core/src/metadata.rs
@@ -829,10 +829,10 @@ mod tests {
             (LevelFilter::TRACE, Some(Level::TRACE)),
         ];
         for (filter, level) in mapping.iter() {
-            assert_eq!(filter.clone().into_level(), *level);
+            assert_eq!(filter.into_level(), *level);
             match level {
                 Some(level) => {
-                    let actual: LevelFilter = level.clone().into();
+                    let actual: LevelFilter = (*level).into();
                     assert_eq!(actual, *filter);
                 }
                 None => {
diff --git a/tracing-error/Cargo.toml b/tracing-error/Cargo.toml
@@ -38,7 +38,7 @@ default = ["traced-error"]
 traced-error = []
 
 [dependencies]
-tracing-subscriber = { path = "../tracing-subscriber", version = "0.2.7", default-features = false, features = ["registry", "fmt"] }
+tracing-subscriber = { path = "../tracing-subscriber", version = "0.2.19", default-features = false, features = ["registry", "fmt"] }
 tracing = { path = "../tracing", version = "0.1.12", default-features = false, features = ["std"] }
 
 [badges]
diff --git a/tracing-error/src/layer.rs b/tracing-error/src/layer.rs
@@ -90,8 +90,7 @@ where
         let span = subscriber
             .span(id)
             .expect("registry should have a span for the current ID");
-        let parents = span.parents();
-        for span in std::iter::once(span).chain(parents) {
+        for span in span.scope() {
             let cont = if let Some(fields) = span.extensions().get::<FormattedFields<F>>() {
                 f(span.metadata(), fields.fields.as_str())
             } else {
diff --git a/tracing-flame/Cargo.toml b/tracing-flame/Cargo.toml
@@ -25,7 +25,7 @@ default = ["smallvec"]
 smallvec = ["tracing-subscriber/smallvec"]
 
 [dependencies]
-tracing-subscriber = { path = "../tracing-subscriber", version = "0.2.3", default-features = false, features = ["registry", "fmt"] }
+tracing-subscriber = { path = "../tracing-subscriber", version = "0.2.19", default-features = false, features = ["registry", "fmt"] }
 tracing = { path = "../tracing", version = "0.1.12", default-features = false, features = ["std"] }
 lazy_static = "1.3.0"
 
diff --git a/tracing-flame/src/lib.rs b/tracing-flame/src/lib.rs
@@ -398,12 +398,10 @@ where
 
         let first = ctx.span(id).expect("expected: span id exists in registry");
 
-        if !self.config.empty_samples && first.from_root().count() == 0 {
+        if !self.config.empty_samples && first.parent().is_none() {
             return;
         }
 
-        let parents = first.from_root();
-
         let mut stack = String::new();
 
         if !self.config.threads_collapsed {
@@ -412,9 +410,12 @@ where
             stack += "all-threads";
         }
 
-        for parent in parents {
-            stack += "; ";
-            write(&mut stack, parent, &self.config).expect("expected: write to String never fails");
+        if let Some(second) = first.parent() {
+            for parent in second.scope().from_root() {
+                stack += "; ";
+                write(&mut stack, parent, &self.config)
+                    .expect("expected: write to String never fails");
+            }
         }
 
         write!(&mut stack, " {}", samples.as_nanos())
@@ -444,28 +445,22 @@ where
 
         let samples = self.time_since_last_event();
         let first = expect!(ctx.span(&id), "expected: span id exists in registry");
-        let parents = first.from_root();
 
         let mut stack = String::new();
         if !self.config.threads_collapsed {
             THREAD_NAME.with(|name| stack += name.as_str());
         } else {
             stack += "all-threads";
         }
-        stack += "; ";
 
-        for parent in parents {
+        for parent in first.scope().from_root() {
+            stack += "; ";
             expect!(
                 write(&mut stack, parent, &self.config),
                 "expected: write to String never fails"
             );
-            stack += "; ";
         }
 
-        expect!(
-            write(&mut stack, first, &self.config),
-            "expected: write to String never fails"
-        );
         expect!(
             write!(&mut stack, " {}", samples.as_nanos()),
             "expected: write to String never fails"
diff --git a/tracing-journald/Cargo.toml b/tracing-journald/Cargo.toml
@@ -16,4 +16,4 @@ keywords = ["tracing", "journald"]
 
 [dependencies]
 tracing-core = { path = "../tracing-core", version = "0.1.10" }
-tracing-subscriber = { path = "../tracing-subscriber", version = "0.2.5" }
+tracing-subscriber = { path = "../tracing-subscriber", version = "0.2.19" }
diff --git a/tracing-journald/src/lib.rs b/tracing-journald/src/lib.rs
@@ -126,7 +126,7 @@ where
         let span = ctx.span(id).expect("unknown span");
         let mut buf = Vec::with_capacity(256);
 
-        let depth = span.parents().count();
+        let depth = span.scope().skip(1).count();
 
         writeln!(buf, "S{}_NAME", depth).unwrap();
         put_value(&mut buf, span.name().as_bytes());
@@ -143,7 +143,7 @@ where
 
     fn on_record(&self, id: &Id, values: &Record, ctx: Context<S>) {
         let span = ctx.span(id).expect("unknown span");
-        let depth = span.parents().count();
+        let depth = span.scope().skip(1).count();
         let mut exts = span.extensions_mut();
         let buf = &mut exts.get_mut::<SpanFields>().expect("missing fields").0;
         values.record(&mut SpanVisitor {
@@ -157,7 +157,11 @@ where
         let mut buf = Vec::with_capacity(256);
 
         // Record span fields
-        for span in ctx.scope() {
+        for span in ctx
+            .lookup_current()
+            .into_iter()
+            .flat_map(|span| span.scope().from_root())
+        {
             let exts = span.extensions();
             let fields = exts.get::<SpanFields>().expect("missing fields");
             buf.extend_from_slice(&fields.0);
diff --git a/tracing-subscriber/Cargo.toml b/tracing-subscriber/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "tracing-subscriber"
-version = "0.2.18"
+version = "0.2.19"
 authors = [
     "Eliza Weisman <eliza@buoyant.io>",
     "David Barsky <me@davidbarsky.com>",
diff --git a/tracing-subscriber/src/filter/env/mod.rs b/tracing-subscriber/src/filter/env/mod.rs
@@ -271,7 +271,7 @@ impl EnvFilter {
                     let bold = Style::new().bold();
                     let mut warning = Color::Yellow.paint("warning");
                     warning.style_ref_mut().is_bold = true;
-                    format!("{}{} {}", warning, bold.clone().paint(":"), bold.paint(msg))
+                    format!("{}{} {}", warning, bold.paint(":"), bold.paint(msg))
                 };
                 eprintln!("{}", msg);
             };
@@ -308,7 +308,6 @@ impl EnvFilter {
                 };
                 let level = directive
                     .level
-                    .clone()
                     .into_level()
                     .expect("=off would not have enabled any filters");
                 ctx(&format!(
@@ -396,8 +395,8 @@ impl<S: Subscriber> Layer<S> for EnvFilter {
             return Some(LevelFilter::TRACE);
         }
         std::cmp::max(
-            self.statics.max_level.clone().into(),
-            self.dynamics.max_level.clone().into(),
+            self.statics.max_level.into(),
+            self.dynamics.max_level.into(),
         )
     }
 
diff --git a/tracing-subscriber/src/filter/level.rs b/tracing-subscriber/src/filter/level.rs
@@ -22,6 +22,6 @@ impl<S: Subscriber> crate::Layer<S> for LevelFilter {
     }
 
     fn max_level_hint(&self) -> Option<LevelFilter> {
-        self.clone().into()
+        Some(*self)
     }
 }
diff --git a/tracing-subscriber/src/fmt/fmt_layer.rs b/tracing-subscriber/src/fmt/fmt_layer.rs
@@ -1,7 +1,7 @@
 use crate::{
     field::RecordFields,
     fmt::{format, FormatEvent, FormatFields, MakeWriter, TestWriter},
-    layer::{self, Context, Scope},
+    layer::{self, Context},
     registry::{LookupSpan, SpanRef},
 };
 use format::{FmtSpan, TimingDisplay};
@@ -804,8 +804,10 @@ where
         F: FnMut(&SpanRef<'_, S>) -> Result<(), E>,
     {
         // visit all the current spans
-        for span in self.ctx.scope() {
-            f(&span)?;
+        if let Some(leaf) = self.ctx.lookup_current() {
+            for span in leaf.scope().from_root() {
+                f(&span)?;
+            }
         }
         Ok(())
     }
@@ -864,7 +866,12 @@ where
     /// the current span.
     ///
     /// [stored data]: ../registry/struct.SpanRef.html
-    pub fn scope(&self) -> Scope<'_, S>
+    #[deprecated(
+        note = "wraps layer::Context::scope, which is deprecated",
+        since = "0.2.19"
+    )]
+    #[allow(deprecated)]
+    pub fn scope(&self) -> crate::layer::Scope<'_, S>
     where
         S: for<'lookup> LookupSpan<'lookup>,
     {
diff --git a/tracing-subscriber/src/fmt/format/json.rs b/tracing-subscriber/src/fmt/format/json.rs
@@ -98,8 +98,10 @@ where
         use serde::ser::SerializeSeq;
         let mut serializer = serializer_o.serialize_seq(None)?;
 
-        for span in self.0.scope() {
-            serializer.serialize_element(&SerializableSpan(&span, self.1))?;
+        if let Some(leaf_span) = self.0.lookup_current() {
+            for span in leaf_span.scope().from_root() {
+                serializer.serialize_element(&SerializableSpan(&span, self.1))?;
+            }
         }
 
         serializer.end()
diff --git a/tracing-subscriber/src/fmt/format/mod.rs b/tracing-subscriber/src/fmt/format/mod.rs
@@ -7,10 +7,7 @@ use crate::{
     registry::LookupSpan,
 };
 
-use std::{
-    fmt::{self, Write},
-    iter,
-};
+use std::fmt::{self, Write};
 use tracing_core::{
     field::{self, Field, Visit},
     span, Event, Level, Subscriber,
@@ -861,9 +858,7 @@ where
             .and_then(|id| self.ctx.ctx.span(&id))
             .or_else(|| self.ctx.ctx.lookup_current());
 
-        let scope = span
-            .into_iter()
-            .flat_map(|span| span.from_root().chain(iter::once(span)));
+        let scope = span.into_iter().flat_map(|span| span.scope().from_root());
 
         for span in scope {
             seen = true;
@@ -933,9 +928,7 @@ where
             .and_then(|id| self.ctx.ctx.span(&id))
             .or_else(|| self.ctx.ctx.lookup_current());
 
-        let scope = span
-            .into_iter()
-            .flat_map(|span| span.from_root().chain(iter::once(span)));
+        let scope = span.into_iter().flat_map(|span| span.scope().from_root());
 
         for span in scope {
             write!(f, "{}", bold.paint(span.metadata().name()))?;
@@ -1510,27 +1503,27 @@ pub(super) mod test {
     #[test]
     fn fmt_span_combinations() {
         let f = FmtSpan::NONE;
-        assert_eq!(f.contains(FmtSpan::NEW), false);
-        assert_eq!(f.contains(FmtSpan::ENTER), false);
-        assert_eq!(f.contains(FmtSpan::EXIT), false);
-        assert_eq!(f.contains(FmtSpan::CLOSE), false);
+        assert!(!f.contains(FmtSpan::NEW));
+        assert!(!f.contains(FmtSpan::ENTER));
+        assert!(!f.contains(FmtSpan::EXIT));
+        assert!(!f.contains(FmtSpan::CLOSE));
 
         let f = FmtSpan::ACTIVE;
-        assert_eq!(f.contains(FmtSpan::NEW), false);
-        assert_eq!(f.contains(FmtSpan::ENTER), true);
-        assert_eq!(f.contains(FmtSpan::EXIT), true);
-        assert_eq!(f.contains(FmtSpan::CLOSE), false);
+        assert!(!f.contains(FmtSpan::NEW));
+        assert!(f.contains(FmtSpan::ENTER));
+        assert!(f.contains(FmtSpan::EXIT));
+        assert!(!f.contains(FmtSpan::CLOSE));
 
         let f = FmtSpan::FULL;
-        assert_eq!(f.contains(FmtSpan::NEW), true);
-        assert_eq!(f.contains(FmtSpan::ENTER), true);
-        assert_eq!(f.contains(FmtSpan::EXIT), true);
-        assert_eq!(f.contains(FmtSpan::CLOSE), true);
+        assert!(f.contains(FmtSpan::NEW));
+        assert!(f.contains(FmtSpan::ENTER));
+        assert!(f.contains(FmtSpan::EXIT));
+        assert!(f.contains(FmtSpan::CLOSE));
 
         let f = FmtSpan::NEW | FmtSpan::CLOSE;
-        assert_eq!(f.contains(FmtSpan::NEW), true);
-        assert_eq!(f.contains(FmtSpan::ENTER), false);
-        assert_eq!(f.contains(FmtSpan::EXIT), false);
-        assert_eq!(f.contains(FmtSpan::CLOSE), true);
+        assert!(f.contains(FmtSpan::NEW));
+        assert!(!f.contains(FmtSpan::ENTER));
+        assert!(!f.contains(FmtSpan::EXIT));
+        assert!(f.contains(FmtSpan::CLOSE));
     }
 }
diff --git a/tracing-subscriber/src/fmt/format/pretty.rs b/tracing-subscriber/src/fmt/format/pretty.rs
@@ -5,10 +5,7 @@ use crate::{
     registry::LookupSpan,
 };
 
-use std::{
-    fmt::{self, Write},
-    iter,
-};
+use std::fmt::{self, Write};
 use tracing_core::{
     field::{self, Field},
     Event, Level, Subscriber,
@@ -187,10 +184,7 @@ where
             .and_then(|id| ctx.span(&id))
             .or_else(|| ctx.lookup_current());
 
-        let scope = span.into_iter().flat_map(|span| {
-            let parents = span.parents();
-            iter::once(span).chain(parents)
-        });
+        let scope = span.into_iter().flat_map(|span| span.scope());
 
         for span in scope {
             let meta = span.metadata();
diff --git a/tracing-subscriber/src/layer.rs b/tracing-subscriber/src/layer.rs
diff --git a/tracing-subscriber/src/registry/mod.rs b/tracing-subscriber/src/registry/mod.rs

Original file line number	Diff line number	Diff line change
`@@ -133,7 +133,6 @@ impl<'a> Visit for Event<'a> {`
`133`	`133`	`Style::new().bold().paint(format!("{:?}", value))`
`134`	`134`	`)`
`135`	`135`	`.unwrap();`
`136`		`- self.comma = true;`
`137`	`136`	`} else {`
`138`	`137`	`write!(`
`139`	`138`	`&mut self.stderr,`
`@@ -142,8 +141,8 @@ impl<'a> Visit for Event<'a> {`
`142`	`141`	`value`
`143`	`142`	`)`
`144`	`143`	`.unwrap();`
`145`		`- self.comma = true;`
`146`	`144`	`}`
	`145`	`+ self.comma = true;`
`147`	`146`	`}`
`148`	`147`	`}`
`149`	`148`