trace: Span API polish (#988)

This branch makes the following changes to `tokio-trace`'s `Span` type:

* **Remove manual close API from spans**
  In practice, there wasn't really a use-case for this, and it 
  complicates the implementation a bit. We can always add it back later.

* **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 inconvenient. 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 <eliza@buoyant.io>

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)
     }
 

tokio-trace/src/span.rs

@@ -37,12 +37,12 @@
 //! # 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> {
+//! impl Future for MyFuture {
 //!     type Item = ();
 //!     type Error = ();
 //!
@@ -91,29 +91,17 @@
 //! # }
 //! ```
 //!
-//! A span may be explicitly closed before when the span handle is dropped by
+//! A span may be explicitly closed by dropping a handle to it, if it is the only
-//! calling the [`Span::close`] method. Doing so will drop that handle the next
+//! 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");
+//! let my_span = span!("my_span");
-//! // Signal to my_span that it should close when it exits
+//! // Drop the handle to the span.
-//! my_span.close();
+//! drop(my_span); // --> Subscriber::drop_span(my_span)
-//! 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
-//! });
 //! # }
 //! ```
 //! However, if multiple handles exist, the span can still be re-entered even if
@@ -154,18 +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<'a>>,
+    inner: Option<Inner<'static>>,
-
-    /// 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>,
 }
 
@@ -207,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].
     ///
@@ -222,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)
     }
@@ -237,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))
     }
 
@@ -250,7 +227,11 @@ impl<'a> Span<'a> {
     /// [metadata]: ::metadata::Metadata
     /// [field values]: ::field::ValueSet
     /// [`follows_from`]: ::span::Span::follows_from
-    pub fn child_of<I>(parent: I, meta: &'a Metadata<'a>, values: &field::ValueSet) -> Span<'a>
+    pub fn child_of<I>(
+        parent: I,
+        meta: &'static Metadata<'static>,
+        values: &field::ValueSet,
+    ) -> Span
     where
         I: Into<Option<Id>>,
     {
@@ -263,23 +244,17 @@ impl<'a> Span<'a> {
 
     /// Constructs a new disabled span.
     #[inline(always)]
-    pub fn new_disabled() -> Span<'a> {
+    pub fn new_disabled() -> Span {
-        Span {
+        Span { inner: None }
-            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))
         });
-        Self {
+        Self { inner }
-            inner,
-            is_closed: false,
-        }
     }
 
     /// Executes the given function in the context of this span.
@@ -295,7 +270,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 +326,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
@@ -403,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 {
@@ -420,7 +377,7 @@ impl<'a> fmt::Debug for Span<'a> {
     }
 }
 
-impl<'a> Into<Option<Id>> for &'a Span<'a> {
+impl<'a> Into<Option<Id>> for &'a Span {
     fn into(self) -> Option<Id> {
         self.id()
     }
@@ -429,14 +386,6 @@ impl<'a> Into<Option<Id>> 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 +436,6 @@ impl<'a> Inner<'a> {
         Inner {
             id,
             subscriber: subscriber.clone(),
-            closed: false,
             meta,
         }
     }
@@ -516,7 +464,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 +473,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
+    /// to re-enter the span.
-    /// exit.
+    fn exit(self) -> Inner<'a> {
-    fn exit(self) -> Option<Inner<'a>> {
         self.inner.subscriber.exit(&self.inner.id);
-        if self.inner.closed {
+        self.inner
-            // Dropping `inner` will allow it to perform the closure if
-            // able.
-            None
-        } else {
-            Some(self.inner)
-        }
     }
 }

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
@@ -67,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)
     }
 
@@ -272,36 +245,10 @@ 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();
+        drop(foo);
-        assert!(foo.is_closed());
-
-        // Now that `foo` has closed, entering it should do nothing.
-        foo.enter(|| {});
-        assert!(foo.is_closed());
-    });
-
-    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();