From 0871f770d60ebc9575c3a6ade3992465f82d9b56 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 13 Mar 2019 14:11:03 -0700 Subject: [PATCH 1/5] trace: Remove manual close API from spans In practice, there wasn't really a use-case for this, and it complicates the implementation. We can always add it back later. Signed-off-by: Eliza Weisman ' --- tokio-trace/src/span.rs | 81 ++++++--------------------------------- tokio-trace/tests/span.rs | 54 +------------------------- 2 files changed, 12 insertions(+), 123 deletions(-) diff --git a/tokio-trace/src/span.rs b/tokio-trace/src/span.rs index 978cdb1e7a6..2e10e5a8d01 100644 --- a/tokio-trace/src/span.rs +++ b/tokio-trace/src/span.rs @@ -91,29 +91,17 @@ //! # } //! ``` //! -//! A span may be explicitly closed before when the span handle is dropped by -//! calling the [`Span::close`] method. Doing so will drop that handle the next +//! A span may be explicitly closed by dropping a handle to it, if it is the only +//! handle to that span. //! time it is exited. For example: //! ``` //! # #[macro_use] extern crate tokio_trace; //! # fn main() { //! use tokio_trace::Span; //! -//! let mut my_span = span!("my_span"); -//! // Signal to my_span that it should close when it exits -//! my_span.close(); -//! my_span.enter(|| { -//! // ... -//! }); // --> Subscriber::exit(my_span); Subscriber::drop_span(my_span) -//! -//! // The handle to `my_span` still exists, but it now knows that the span was -//! // closed while it was executing. -//! my_span.is_closed(); // ==> true -//! -//! // Attempting to enter the span using the handle again will do nothing. -//! my_span.enter(|| { -//! // no-op -//! }); +//! let my_span = span!("my_span"); +//! // Drop the handle to the span. +//! drop(my_span); // --> Subscriber::drop_span(my_span) //! # } //! ``` //! However, if multiple handles exist, the span can still be re-entered even if @@ -159,13 +147,6 @@ pub struct Span<'a> { /// /// If this is `None`, then the span has either closed or was never enabled. inner: Option>, - - /// Set to `true` when the span closes. - /// - /// This allows us to distinguish if `inner` is `None` because the span was - /// never enabled (and thus the inner state was never created), or if the - /// previously entered, but it is now closed. - is_closed: bool, } /// A handle representing the capacity to enter a span which is known to exist. @@ -184,10 +165,6 @@ pub(crate) struct Inner<'a> { /// `id`. subscriber: Dispatch, - /// A flag indicating that the span has been instructed to close when - /// possible. - closed: bool, - meta: &'a Metadata<'a>, } @@ -278,7 +255,6 @@ impl<'a> Span<'a> { }); Self { inner, - is_closed: false, } } @@ -295,7 +271,7 @@ impl<'a> Span<'a> { Some(inner) => { let guard = inner.enter(); let result = f(); - self.inner = guard.exit(); + self.inner = Some(guard.exit()); result } None => f(), @@ -351,29 +327,11 @@ impl<'a> Span<'a> { self } - /// Closes this span handle, dropping its internal state. - /// - /// Once this function has been called, subsequent calls to `enter` on this - /// handle will no longer enter the span. If this is the final handle with - /// the potential to enter that span, the subscriber may consider the span to - /// have ended. - pub fn close(&mut self) { - if let Some(mut inner) = self.inner.take() { - inner.close(); - } - self.is_closed = true; - } - - /// Returns `true` if this span is closed. - pub fn is_closed(&self) -> bool { - self.is_closed - } - /// Returns `true` if this span was disabled by the subscriber and does not /// exist. #[inline] pub fn is_disabled(&self) -> bool { - self.inner.is_none() && !self.is_closed + self.inner.is_none() } /// Indicates that the span with the given ID has an indirect causal @@ -420,7 +378,7 @@ impl<'a> fmt::Debug for Span<'a> { } } -impl<'a> Into> for &'a Span<'a> { +impl<'a> Into> for &'a Span { fn into(self) -> Option { self.id() } @@ -429,14 +387,6 @@ impl<'a> Into> for &'a Span<'a> { // ===== impl Inner ===== impl<'a> Inner<'a> { - /// Indicates that this handle will not be reused to enter the span again. - /// - /// After calling `close`, the `Entered` guard returned by `self.enter()` - /// will _drop_ this handle when it is exited. - fn close(&mut self) { - self.closed = true; - } - /// Enters the span, returning a guard that may be used to exit the span and /// re-enter the prior span. /// @@ -487,7 +437,6 @@ impl<'a> Inner<'a> { Inner { id, subscriber: subscriber.clone(), - closed: false, meta, } } @@ -516,7 +465,6 @@ impl<'a> Clone for Inner<'a> { Inner { id: self.subscriber.clone_span(&self.id), subscriber: self.subscriber.clone(), - closed: self.closed, meta: self.meta, } } @@ -526,16 +474,9 @@ impl<'a> Clone for Inner<'a> { impl<'a> Entered<'a> { /// Exit the `Entered` guard, returning an `Inner` handle that may be used - /// to re-enter the span, or `None` if the span closed while performing the - /// exit. - fn exit(self) -> Option> { + /// to re-enter the span. + fn exit(self) -> Inner<'a> { self.inner.subscriber.exit(&self.inner.id); - if self.inner.closed { - // Dropping `inner` will allow it to perform the closure if - // able. - None - } else { - Some(self.inner) - } + self.inner } } diff --git a/tokio-trace/tests/span.rs b/tokio-trace/tests/span.rs index 5627a4e3d54..3d5ad2de809 100644 --- a/tokio-trace/tests/span.rs +++ b/tokio-trace/tests/span.rs @@ -10,33 +10,6 @@ use tokio_trace::{ Level, Span, }; -#[test] -fn closed_handle_cannot_be_entered() { - let subscriber = subscriber::mock() - .enter(span::mock().named("foo")) - .drop_span(span::mock().named("bar")) - .enter(span::mock().named("bar")) - .exit(span::mock().named("bar")) - .drop_span(span::mock().named("bar")) - .exit(span::mock().named("foo")) - .run(); - - with_default(subscriber, || { - span!("foo").enter(|| { - let bar = span!("bar"); - let mut another_bar = bar.clone(); - drop(bar); - - another_bar.enter(|| {}); - - another_bar.close(); - // After we close `another_bar`, it should close and not be - // re-entered. - another_bar.enter(|| {}); - }); - }); -} - #[test] fn handles_to_the_same_span_are_equal() { // Create a mock subscriber that will return `true` on calls to @@ -272,40 +245,15 @@ fn span_closes_when_exited() { .run_with_handle(); with_default(subscriber, || { let mut foo = span!("foo"); - assert!(!foo.is_closed()); foo.enter(|| {}); - assert!(!foo.is_closed()); - foo.close(); - assert!(foo.is_closed()); - - // Now that `foo` has closed, entering it should do nothing. - foo.enter(|| {}); - assert!(foo.is_closed()); + drop(foo); }); handle.assert_finished(); } -#[test] -fn entering_a_closed_span_again_is_a_no_op() { - let (subscriber, handle) = subscriber::mock() - .drop_span(span::mock().named("foo")) - .done() - .run_with_handle(); - with_default(subscriber, || { - let mut foo = span!("foo"); - - foo.close(); - foo.enter(|| { - // This should do nothing. - }); - assert!(foo.is_closed()); - }); - - handle.assert_finished(); -} #[test] fn moved_field() { From 4d953eb7190db970fee082d7ea9f625c54d196c8 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 13 Mar 2019 14:12:45 -0700 Subject: [PATCH 2/5] trace: Remove generic lifetime from `Span` Again, there wasn't actually a use-case for spans with metadata that doesn't live for the static lifetime, and it made using `Span`s in other types somewhat more challenging. It's also possible to implement an alternative API for non-static spans on top of the `tokio-trace-core` primitives. Signed-off-by: Eliza Weisman --- tokio-trace/src/span.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/tokio-trace/src/span.rs b/tokio-trace/src/span.rs index 2e10e5a8d01..596d50474d7 100644 --- a/tokio-trace/src/span.rs +++ b/tokio-trace/src/span.rs @@ -142,11 +142,11 @@ use { /// span will silently do nothing. Thus, the handle can be used in the same /// manner regardless of whether or not the trace is currently being collected. #[derive(Clone, PartialEq, Hash)] -pub struct Span<'a> { +pub struct Span { /// A handle used to enter the span when it is not executing. /// /// If this is `None`, then the span has either closed or was never enabled. - inner: Option>, + inner: Option>, } /// A handle representing the capacity to enter a span which is known to exist. @@ -184,7 +184,7 @@ struct Entered<'a> { // ===== impl Span ===== -impl<'a> Span<'a> { +impl Span { /// Constructs a new `Span` with the given [metadata] and set of [field /// values]. /// @@ -199,7 +199,7 @@ impl<'a> Span<'a> { /// [field values]: ::field::ValueSet /// [`follows_from`]: ::span::Span::follows_from #[inline] - pub fn new(meta: &'a Metadata<'a>, values: &field::ValueSet) -> Span<'a> { + pub fn new(meta: &'static Metadata<'static>, values: &field::ValueSet) -> Span { let new_span = Attributes::new(meta, values); Self::make(meta, new_span) } @@ -214,7 +214,7 @@ impl<'a> Span<'a> { /// [field values]: ::field::ValueSet /// [`follows_from`]: ::span::Span::follows_from #[inline] - pub fn new_root(meta: &'a Metadata<'a>, values: &field::ValueSet) -> Span<'a> { + pub fn new_root(meta: &'static Metadata<'static>, values: &field::ValueSet) -> Span { Self::make(meta, Attributes::new_root(meta, values)) } @@ -227,7 +227,7 @@ impl<'a> Span<'a> { /// [metadata]: ::metadata::Metadata /// [field values]: ::field::ValueSet /// [`follows_from`]: ::span::Span::follows_from - pub fn child_of(parent: I, meta: &'a Metadata<'a>, values: &field::ValueSet) -> Span<'a> + pub fn child_of(parent: I, meta: &'static Metadata<'static>, values: &field::ValueSet) -> Span where I: Into>, { @@ -240,15 +240,14 @@ impl<'a> Span<'a> { /// Constructs a new disabled span. #[inline(always)] - pub fn new_disabled() -> Span<'a> { + pub fn new_disabled() -> Span { Span { inner: None, - is_closed: false, } } #[inline(always)] - fn make(meta: &'a Metadata<'a>, new_span: Attributes) -> Span<'a> { + fn make(meta: &'static Metadata<'static>, new_span: Attributes) -> Span { let inner = dispatcher::get_default(move |dispatch| { let id = dispatch.new_span(&new_span); Some(Inner::new(id, dispatch, meta)) @@ -361,12 +360,12 @@ impl<'a> Span<'a> { } /// Returns this span's `Metadata`, if it is enabled. - pub fn metadata(&self) -> Option<&'a Metadata<'a>> { + pub fn metadata(&self) -> Option<&'static Metadata<'static>> { self.inner.as_ref().map(Inner::metadata) } } -impl<'a> fmt::Debug for Span<'a> { +impl<'a> fmt::Debug for Span { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let mut span = f.debug_struct("Span"); if let Some(ref inner) = self.inner { From 738b2ab19c0c3d0f26fa6d57b2597fc7c00e61a3 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 18 Mar 2019 10:56:49 -0700 Subject: [PATCH 3/5] fixup tests --- tokio-trace/benches/subscriber.rs | 2 +- tokio-trace/src/span.rs | 4 ++-- tokio-trace/tests/span.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tokio-trace/benches/subscriber.rs b/tokio-trace/benches/subscriber.rs index 18ece466c3a..e4a14a74180 100644 --- a/tokio-trace/benches/subscriber.rs +++ b/tokio-trace/benches/subscriber.rs @@ -110,7 +110,7 @@ fn enter_span(b: &mut Bencher) { #[bench] fn span_repeatedly(b: &mut Bencher) { #[inline] - fn mk_span(i: u64) -> tokio_trace::Span<'static> { + fn mk_span(i: u64) -> tokio_trace::Span { span!("span", i = i) } diff --git a/tokio-trace/src/span.rs b/tokio-trace/src/span.rs index 596d50474d7..7922b068746 100644 --- a/tokio-trace/src/span.rs +++ b/tokio-trace/src/span.rs @@ -37,9 +37,9 @@ //! # extern crate tokio_trace; //! # extern crate futures; //! # use futures::{Future, Poll, Async}; -//! struct MyFuture<'a> { +//! struct MyFuture { //! // data -//! span: tokio_trace::Span<'a>, +//! span: tokio_trace::Span, //! } //! //! impl<'a> Future for MyFuture<'a> { diff --git a/tokio-trace/tests/span.rs b/tokio-trace/tests/span.rs index 3d5ad2de809..8e10d43bb75 100644 --- a/tokio-trace/tests/span.rs +++ b/tokio-trace/tests/span.rs @@ -40,7 +40,7 @@ fn handles_to_different_spans_are_not_equal() { fn handles_to_different_spans_with_the_same_metadata_are_not_equal() { // Every time time this function is called, it will return a _new // instance_ of a span with the same metadata, name, and fields. - fn make_span() -> Span<'static> { + fn make_span() -> Span { span!("foo", bar = 1u64, baz = false) } From 74734ce7acd0c6c3585890fb656498431ae923be Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 18 Mar 2019 11:06:43 -0700 Subject: [PATCH 4/5] rustfmt --- tokio-trace/src/span.rs | 14 +++++++------- tokio-trace/tests/span.rs | 1 - 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/tokio-trace/src/span.rs b/tokio-trace/src/span.rs index 7922b068746..37698b3eb8d 100644 --- a/tokio-trace/src/span.rs +++ b/tokio-trace/src/span.rs @@ -227,7 +227,11 @@ impl Span { /// [metadata]: ::metadata::Metadata /// [field values]: ::field::ValueSet /// [`follows_from`]: ::span::Span::follows_from - pub fn child_of(parent: I, meta: &'static Metadata<'static>, values: &field::ValueSet) -> Span + pub fn child_of( + parent: I, + meta: &'static Metadata<'static>, + values: &field::ValueSet, + ) -> Span where I: Into>, { @@ -241,9 +245,7 @@ impl Span { /// Constructs a new disabled span. #[inline(always)] pub fn new_disabled() -> Span { - Span { - inner: None, - } + Span { inner: None } } #[inline(always)] @@ -252,9 +254,7 @@ impl Span { let id = dispatch.new_span(&new_span); Some(Inner::new(id, dispatch, meta)) }); - Self { - inner, - } + Self { inner } } /// Executes the given function in the context of this span. diff --git a/tokio-trace/tests/span.rs b/tokio-trace/tests/span.rs index 8e10d43bb75..1c1bc003619 100644 --- a/tokio-trace/tests/span.rs +++ b/tokio-trace/tests/span.rs @@ -254,7 +254,6 @@ fn span_closes_when_exited() { handle.assert_finished(); } - #[test] fn moved_field() { let (subscriber, handle) = subscriber::mock() From c1aa3727a8ac268a22acfb4fc165b85365176941 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 18 Mar 2019 11:16:00 -0700 Subject: [PATCH 5/5] fix doctests too --- tokio-trace/src/span.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tokio-trace/src/span.rs b/tokio-trace/src/span.rs index 37698b3eb8d..bc1aa8e0d7c 100644 --- a/tokio-trace/src/span.rs +++ b/tokio-trace/src/span.rs @@ -42,7 +42,7 @@ //! span: tokio_trace::Span, //! } //! -//! impl<'a> Future for MyFuture<'a> { +//! impl Future for MyFuture { //! type Item = (); //! type Error = (); //!