Remove default impl of Collect::current_span #1268

lenaschoenburg · 2021-02-25T20:58:18Z

Motivation

This removes the default implementation of tracing_core::Collect::current_span, which implements part one of #1262.

Solution

Removes the default implementation
Makes tracing_core::span::Current::unknown public so that implementers of Collect can use it if they didn't implement current_span() before.
Copy the old default implementation to all implementers of Collect

lenaschoenburg · 2021-02-25T20:58:32Z

We could also add a re-export of tracing_core::span::Current as tracing::span::Current which would look a bit nicer when implementing Collect.

lenaschoenburg · 2021-02-25T21:07:59Z

I keep forgetting to run cargo check --all --bins --tests --benches so there are a bunch of places still left that now need to implement current_span.

davidbarsky · 2021-02-26T17:02:23Z

tracing/tests/support/collector.rs

+    fn current_span(&self) -> tracing_core::span::Current {
+        todo!()
+    }


Why's this one todo'd, whereas all the others are unknown?

the test collector probably should actually implement this, it knows the ID of the current span:

tracing/tracing/tests/support/collector.rs

Line 44 in f867e83

current: Mutex<Vec<Id>>,

and it can look up spans by ID:

tracing/tracing/tests/support/collector.rs

Line 42 in f867e83

spans: Mutex<HashMap<Id, SpanState>>,

similar to the sloggish example collector, we would need to add the metadata to the stored span data to implement this properly. in this case, the stored span data is the SpanState struct:

tracing/tracing/tests/support/collector.rs

Lines 36 to 39 in f867e83

struct SpanState {

name: &'static str,

refs: usize,

}

we could pretty easily add a &'static Metadata<'static> to that, allowing us to actually return the current span.

i think this is somewhat important; even though none of the tests currently use the mock collector's current_span method, it should probably implement the behavior correctly so that we're not surprised when writing future tests that expect it to work...

hawkw

this feels like a good start. i think there's a few places where we probably ought to add real current_span implementations instead of just returning unknown. once that's taken care of, I'll be happy to merge this!

hawkw · 2021-02-26T19:07:12Z

examples/examples/sloggish/sloggish_collector.rs

@@ -265,4 +266,8 @@ impl Collect for SloggishCollector {
        // TODO: GC unneeded spans.
        false
    }
+
+    fn current_span(&self) -> Current {
+        Current::unknown()


in this example, the collector does know the current span's ID:

tracing/examples/examples/sloggish/sloggish_collector.rs

Line 58 in f867e83

current: CurrentSpanPerThread,

so, maybe this should return that span? this would require a slightly larger change, modifying the Span struct to also include a &'static Metadata<'static> that's stored in new_span, but then we could write something like this:

Suggested change

Current::unknown()

if let Some(id) = self.current.id() {

let metadata = self.spans

.lock()

.unwrap()

.get(id)

.unwrap_or_else(|| panic!("no metadata stored for span with ID {:?}", id))

.metadata;

return Current::new(id, metadata);

}

Current::none()

tracing-core/src/collect.rs

hawkw · 2021-02-26T19:09:13Z

tracing-core/src/dispatch.rs

@@ -853,6 +857,10 @@ impl Collect for NoCollector {

    fn enter(&self, _span: &span::Id) {}
    fn exit(&self, _span: &span::Id) {}
+
+    fn current_span(&self) -> span::Current {
+        span::Current::unknown()


nit, this doesn't really matter: i think it might be more correct for the no-op collector to return none rather than unknown — the intended semantics is that all spans are disabled when there's no collector, so we are never in a span. with that said, it doesn't really make a difference in practice.

hawkw · 2021-02-26T19:13:20Z

tracing/tests/support/collector.rs

+    fn current_span(&self) -> tracing_core::span::Current {
+        todo!()
+    }


the test collector probably should actually implement this, it knows the ID of the current span:

tracing/tracing/tests/support/collector.rs

Line 44 in f867e83

current: Mutex<Vec<Id>>,

and it can look up spans by ID:

tracing/tracing/tests/support/collector.rs

Line 42 in f867e83

spans: Mutex<HashMap<Id, SpanState>>,

similar to the sloggish example collector, we would need to add the metadata to the stored span data to implement this properly. in this case, the stored span data is the SpanState struct:

tracing/tracing/tests/support/collector.rs

Lines 36 to 39 in f867e83

struct SpanState {

name: &'static str,

refs: usize,

}

we could pretty easily add a &'static Metadata<'static> to that, allowing us to actually return the current span.

i think this is somewhat important; even though none of the tests currently use the mock collector's current_span method, it should probably implement the behavior correctly so that we're not surprised when writing future tests that expect it to work...

lenaschoenburg · 2021-02-26T21:23:30Z

Sorry, I should have marked this as WIP earlier. I'll try to implement current_span properly where it makes sense but it might take me a while. I don't want to take up too much of your resources or block the release of 0.2 so feel free work on it yourself in the meantime.

lenaschoenburg · 2021-02-27T14:58:56Z

Okay, I think I implemented all suggestions. From my side there is only one open question: #1268 (comment)

davidbarsky · 2021-03-21T14:14:32Z

@dignati Sorry for the delay in responding.

We could also add a re-export of tracing_core::span::Current as tracing::span::Current which would look a bit nicer when implementing Collect.

I think we can keep this as-is for now and not worry about it. If you could rebase again the master branch (or I can do it for you!), then I think we're clear to merge this in.

When we remove the default implementation of `Collect::current_span` implementers of `Collect` that previously relied on the default behaviour will need to use this method.

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

lenaschoenburg · 2021-03-21T20:12:35Z

No worries! I rebased without any conflicts.

lenaschoenburg · 2021-06-07T20:58:21Z

Is this PR still relevant? I can rebase again if needed, otherwise feel free to close this.

hawkw

okay, this looks good to me --- thanks for the ping!

hawkw · 2021-06-07T23:28:47Z

Ah, it looks like, post-rebase, the build is failing now because new Collect impls were added on master --- for Box<dyn Collect + ...> and Arc<dyn Collect + ...>. The new impls are here:

tracing/tracing-core/src/collect.rs

Lines 559 to 691 in 44d3558

    
           #[cfg(feature = "alloc")] 
        
           impl Collect for alloc::boxed::Box<dyn Collect + Send + Sync + 'static> { 
        
               #[inline] 
        
               fn register_callsite(&self, metadata: &'static Metadata<'static>) -> Interest { 
        
                   self.as_ref().register_callsite(metadata) 
        
               } 
        
               #[inline] 
        
               fn enabled(&self, metadata: &Metadata<'_>) -> bool { 
        
                   self.as_ref().enabled(metadata) 
        
               } 
        
               #[inline] 
        
               fn max_level_hint(&self) -> Option<LevelFilter> { 
        
                   self.as_ref().max_level_hint() 
        
               } 
        
               #[inline] 
        
               fn new_span(&self, span: &span::Attributes<'_>) -> span::Id { 
        
                   self.as_ref().new_span(span) 
        
               } 
        
               #[inline] 
        
               fn record(&self, span: &span::Id, values: &span::Record<'_>) { 
        
                   self.as_ref().record(span, values) 
        
               } 
        
               #[inline] 
        
               fn record_follows_from(&self, span: &span::Id, follows: &span::Id) { 
        
                   self.as_ref().record_follows_from(span, follows) 
        
               } 
        
               #[inline] 
        
               fn event(&self, event: &Event<'_>) { 
        
                   self.as_ref().event(event) 
        
               } 
        
               #[inline] 
        
               fn enter(&self, span: &span::Id) { 
        
                   self.as_ref().enter(span) 
        
               } 
        
               #[inline] 
        
               fn exit(&self, span: &span::Id) { 
        
                   self.as_ref().exit(span) 
        
               } 
        
               #[inline] 
        
               fn clone_span(&self, id: &span::Id) -> span::Id { 
        
                   self.as_ref().clone_span(id) 
        
               } 
        
               #[inline] 
        
               fn try_close(&self, id: span::Id) -> bool { 
        
                   self.as_ref().try_close(id) 
        
               } 
        
               #[inline] 
        
               unsafe fn downcast_raw(&self, id: TypeId) -> Option<NonNull<()>> { 
        
                   if id == TypeId::of::<Self>() { 
        
                       return Some(NonNull::from(self).cast()); 
        
                   } 
        
                   self.as_ref().downcast_raw(id) 
        
               } 
        
           } 
        
           #[cfg(feature = "alloc")] 
        
           impl Collect for alloc::sync::Arc<dyn Collect + Send + Sync + 'static> { 
        
               #[inline] 
        
               fn register_callsite(&self, metadata: &'static Metadata<'static>) -> Interest { 
        
                   self.as_ref().register_callsite(metadata) 
        
               } 
        
               #[inline] 
        
               fn enabled(&self, metadata: &Metadata<'_>) -> bool { 
        
                   self.as_ref().enabled(metadata) 
        
               } 
        
               #[inline] 
        
               fn max_level_hint(&self) -> Option<LevelFilter> { 
        
                   self.as_ref().max_level_hint() 
        
               } 
        
               #[inline] 
        
               fn new_span(&self, span: &span::Attributes<'_>) -> span::Id { 
        
                   self.as_ref().new_span(span) 
        
               } 
        
               #[inline] 
        
               fn record(&self, span: &span::Id, values: &span::Record<'_>) { 
        
                   self.as_ref().record(span, values) 
        
               } 
        
               #[inline] 
        
               fn record_follows_from(&self, span: &span::Id, follows: &span::Id) { 
        
                   self.as_ref().record_follows_from(span, follows) 
        
               } 
        
               #[inline] 
        
               fn event(&self, event: &Event<'_>) { 
        
                   self.as_ref().event(event) 
        
               } 
        
               #[inline] 
        
               fn enter(&self, span: &span::Id) { 
        
                   self.as_ref().enter(span) 
        
               } 
        
               #[inline] 
        
               fn exit(&self, span: &span::Id) { 
        
                   self.as_ref().exit(span) 
        
               } 
        
               #[inline] 
        
               fn clone_span(&self, id: &span::Id) -> span::Id { 
        
                   self.as_ref().clone_span(id) 
        
               } 
        
               #[inline] 
        
               fn try_close(&self, id: span::Id) -> bool { 
        
                   self.as_ref().try_close(id) 
        
               } 
        
               #[inline] 
        
               unsafe fn downcast_raw(&self, id: TypeId) -> Option<NonNull<()>> { 
        
                   if id == TypeId::of::<Self>() { 
        
                       return Some(NonNull::from(self).cast()); 
        
                   } 
        
                   self.as_ref().downcast_raw(id) 
        
               } 
        
           }

We should add current_span methods to those new Collect impls, and then CI should pass again.

davidbarsky · 2021-06-21T16:14:59Z

Thanks for working on this and sorry for the delay in merging!

lenaschoenburg requested review from carllerche, davidbarsky, hawkw and a team as code owners February 25, 2021 20:58

lenaschoenburg changed the title ~~Remove current span default impl~~ Remove default impl of Collect::current_span Feb 25, 2021

lenaschoenburg force-pushed the remove-current-span-default-impl branch from 9a832b7 to 2c64332 Compare February 25, 2021 21:23

davidbarsky reviewed Feb 26, 2021

View reviewed changes

hawkw requested changes Feb 26, 2021

View reviewed changes

lenaschoenburg force-pushed the remove-current-span-default-impl branch from 5639206 to 87a6b13 Compare February 26, 2021 21:18

lenaschoenburg marked this pull request as draft February 26, 2021 22:08

lenaschoenburg force-pushed the remove-current-span-default-impl branch 2 times, most recently from 654b790 to 4eeb779 Compare February 27, 2021 14:31

lenaschoenburg marked this pull request as ready for review February 27, 2021 14:57

lenaschoenburg and others added 9 commits March 21, 2021 19:51

Make Current::unknown public

0f7fa61

When we remove the default implementation of `Collect::current_span` implementers of `Collect` that previously relied on the default behaviour will need to use this method.

Remove default implementation of current_span

ad67ccb

Add current_span impl for log-with-trace example

44af1a8

Add current_span impl for static-max-level-features test

36b4e7a

core: fix docstring for Current::new

dcb2daf

tracing: return current span for MockCollector

067525d

examples: return current span in SloggishCollector

05153c2

core: add doc link to Current::unknown()

d4d221f

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

core: NoCollectorreturns none instead of unknown current span

76e6462

lenaschoenburg force-pushed the remove-current-span-default-impl branch from 1931e2d to 76e6462 Compare March 21, 2021 19:54

lenaschoenburg requested a review from hawkw March 23, 2021 12:57

Merge branch 'master' into remove-current-span-default-impl

881be33

hawkw approved these changes Jun 7, 2021

View reviewed changes

lenaschoenburg and others added 3 commits June 12, 2021 11:07

core: Add current_span impl for Box and Arc

740ffa8

tracing: Fix already imported´span::Id´

fa68f95

Merge branch 'master' into remove-current-span-default-impl

1255d70

davidbarsky approved these changes Jun 14, 2021

View reviewed changes

davidbarsky merged commit 944abac into tokio-rs:master Jun 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove default impl of Collect::current_span #1268

Remove default impl of Collect::current_span #1268

lenaschoenburg commented Feb 25, 2021

lenaschoenburg commented Feb 25, 2021

lenaschoenburg commented Feb 25, 2021

davidbarsky Feb 26, 2021

hawkw Feb 26, 2021

hawkw left a comment

hawkw Feb 26, 2021

hawkw Feb 26, 2021

hawkw Feb 26, 2021

lenaschoenburg commented Feb 26, 2021

lenaschoenburg commented Feb 27, 2021

davidbarsky commented Mar 21, 2021

lenaschoenburg commented Mar 21, 2021

lenaschoenburg commented Jun 7, 2021

hawkw left a comment

hawkw commented Jun 7, 2021

davidbarsky commented Jun 21, 2021

-        Current::unknown()
+        if let Some(id) = self.current.id() {
+            let metadata = self.spans
+                .lock()
+                .unwrap()
+                .get(id)
+                .unwrap_or_else(|| panic!("no metadata stored for span with ID {:?}", id))
+                .metadata;
+            return Current::new(id, metadata);
+        }
+        Current::none()

Remove default impl of Collect::current_span #1268

Remove default impl of Collect::current_span #1268

Conversation

lenaschoenburg commented Feb 25, 2021

Motivation

Solution

lenaschoenburg commented Feb 25, 2021

lenaschoenburg commented Feb 25, 2021

davidbarsky Feb 26, 2021

Choose a reason for hiding this comment

hawkw Feb 26, 2021

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw Feb 26, 2021

Choose a reason for hiding this comment

hawkw Feb 26, 2021

Choose a reason for hiding this comment

hawkw Feb 26, 2021

Choose a reason for hiding this comment

lenaschoenburg commented Feb 26, 2021

lenaschoenburg commented Feb 27, 2021

davidbarsky commented Mar 21, 2021

lenaschoenburg commented Mar 21, 2021

lenaschoenburg commented Jun 7, 2021

hawkw left a comment

Choose a reason for hiding this comment

hawkw commented Jun 7, 2021

davidbarsky commented Jun 21, 2021