From c322ec181c3e971ea200a73eeffa5030f3f929fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Tue, 22 Jun 2021 19:33:42 +0200 Subject: [PATCH] subscriber: unify span traversal (#1431) ## 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::>().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) --- tracing-subscriber/src/fmt/fmt_layer.rs | 15 ++-- tracing-subscriber/src/layer.rs | 96 +++++++++++++++++-------- 2 files changed, 78 insertions(+), 33 deletions(-) diff --git a/tracing-subscriber/src/fmt/fmt_layer.rs b/tracing-subscriber/src/fmt/fmt_layer.rs index eeddabd471..ab1806d4dd 100644 --- 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/layer.rs b/tracing-subscriber/src/layer.rs index e4322e24a4..03391e4fa6 100644 --- a/tracing-subscriber/src/layer.rs +++ b/tracing-subscriber/src/layer.rs @@ -580,9 +580,24 @@ pub struct Identity { /// [`Context::scope`]: struct.Context.html#method.scope #[cfg(feature = "registry")] #[cfg_attr(docsrs, doc(cfg(feature = "registry")))] -pub struct Scope<'a, L: LookupSpan<'a>>( - Option, std::iter::Once>>>, -); +#[deprecated(note = "renamed to crate::registry::ScopeFromRoot", since = "0.2.19")] +#[derive(Debug)] +pub struct Scope<'a, L>(std::iter::Flatten>>) +where + L: LookupSpan<'a>; + +#[cfg(feature = "registry")] +#[allow(deprecated)] +impl<'a, L> Iterator for Scope<'a, L> +where + L: LookupSpan<'a>, +{ + type Item = SpanRef<'a, L>; + + fn next(&mut self) -> Option { + self.0.next() + } +} // === impl Layered === @@ -1096,15 +1111,59 @@ where /// [stored data]: ../registry/struct.SpanRef.html #[cfg(feature = "registry")] #[cfg_attr(docsrs, doc(cfg(feature = "registry")))] + #[deprecated( + note = "equivalent to `self.current_span().id().and_then(|id| self.span_scope(id).from_root())` but consider passing an explicit ID instead of relying on the contextual span", + since = "0.2.19" + )] + #[allow(deprecated)] pub fn scope(&self) -> Scope<'_, S> where S: for<'lookup> registry::LookupSpan<'lookup>, { - let scope = self.lookup_current().map(|span| { - let parents = span.from_root(); - parents.chain(std::iter::once(span)) - }); - Scope(scope) + Scope( + self.lookup_current() + .as_ref() + .map(registry::SpanRef::scope) + .map(registry::Scope::from_root) + .into_iter() + .flatten(), + ) + } + + /// Returns an iterator over the [stored data] for all the spans in the + /// current context, starting with the specified span and ending with the + /// root of the trace tree and ending with the current span. + /// + ///
+ ///
ⓘNote
+ ///
+ ///
+ /// 
+    /// Note: Compared to scope this
+    /// returns the spans in reverse order (from leaf to root). Use
+    /// Scope::from_root
+    /// in case root-to-leaf ordering is desired.
+    ///
+ /// + ///
+ ///
ⓘNote
+ ///
+ ///
+ /// 
+    /// Note: This requires the wrapped subscriber to implement the
+    /// LookupSpan trait.
+    /// See the documentation on Context's
+    /// declaration for details.
+    ///
+ /// + /// [stored data]: ../registry/struct.SpanRef.html + #[cfg(feature = "registry")] + #[cfg_attr(docsrs, doc(cfg(feature = "registry")))] + pub fn span_scope(&self, id: &span::Id) -> Option> + where + S: for<'lookup> registry::LookupSpan<'lookup>, + { + Some(self.span(id)?.scope()) } } @@ -1133,27 +1192,6 @@ impl Identity { } } -// === impl Scope === - -#[cfg(feature = "registry")] -#[cfg_attr(docsrs, doc(cfg(feature = "registry")))] -impl<'a, L: LookupSpan<'a>> Iterator for Scope<'a, L> { - type Item = SpanRef<'a, L>; - - #[inline] - fn next(&mut self) -> Option { - self.0.as_mut()?.next() - } -} - -#[cfg(feature = "registry")] -#[cfg_attr(docsrs, doc(cfg(feature = "registry")))] -impl<'a, L: LookupSpan<'a>> std::fmt::Debug for Scope<'a, L> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.pad("Scope { .. }") - } -} - #[cfg(test)] pub(crate) mod tests { use super::*;