trace-core: Introduce callsite classification in metadata (#1046)

## Motivation

To ease the implementation of `Subscriber::register_callsite`, a field
should be added to `Metadata` to indicate if this callsite is an event or
a span.

## Solution

A new struct, `Kind`, is added to the `metadata` module in
`tokio-trace-core`, and a `Kind` field is added to the `Metadata`
struct. Macros which construct `metadata` now require a `Kind`.

`Kind` is represented as a struct with a private inner enum to allow new
`Kind`s to be added without breaking changes. However, the _addition_ of
the kind field _is_ a breaking change. While this could be done in a
backward-compatible way, it would permit the construction of metadata
with unknown kinds, and since the next `tokio-trace-core` release will
be a breaking change, I opted to make the breaking change instead.

New API tests for the `callsite!` and `metadata!` macros have been added
to guard against future API breakage.

Fixes: #986
Closes: #1008

Co-Authored-By: csmoe <csmoe@msn.com>

9 files changed, 162 insertions, 8 deletions

diff --git a/tokio-trace/src/macros.rs b/tokio-trace/src/macros.rs
index e3e631ea..3f5018f9 100644
--- a/tokio-trace/src/macros.rs
+++ b/tokio-trace/src/macros.rs
@@ -102,6 +102,7 @@ macro_rules! span {
             use $crate::callsite::Callsite;
             let callsite = callsite! {
                 name: $name,
+                kind: $crate::metadata::Kind::SPAN,
                 target: $target,
                 level: $lvl,
                 fields: $($($k).+),*
@@ -128,6 +129,7 @@ macro_rules! span {
             use $crate::callsite::Callsite;
             let callsite = callsite! {
                 name: $name,
+                kind: $crate::metadata::Kind::SPAN,
                 target: $target,
                 level: $lvl,
                 fields: $( $($k).+ ),*
@@ -742,6 +744,7 @@ macro_rules! event {
                         ":",
                         __tokio_trace_line!()
                     ),
+                    kind: $crate::metadata::Kind::EVENT,
                     target: $target,
                     level: $lvl,
                     fields: $( $($k).+ ),*
@@ -1223,17 +1226,24 @@ macro_rules! error {
 #[doc(hidden)]
 #[macro_export(local_inner_macros)]
 macro_rules! callsite {
-    (name: $name:expr, fields: $( $field_name:expr ),* $(,)*) => ({
+    (name: $name:expr, kind: $kind:expr, fields: $( $field_name:expr ),* $(,)*) => ({
         callsite! {
             name: $name,
+            kind: $kind,
             target: __tokio_trace_module_path!(),
             level: $crate::Level::TRACE,
             fields: $( $field_name ),*
         }
     });
-    (name: $name:expr, level: $lvl:expr, fields: $( $field_name:expr ),* $(,)*) => ({
+    (
+        name: $name:expr,
+        kind: $kind:expr,
+        level: $lvl:expr,
+        fields: $( $field_name:expr ),*  $(,)*
+    ) => ({
         callsite! {
             name: $name,
+            kind: $kind,
             target: __tokio_trace_module_path!(),
             level: $lvl,
             fields: $( $field_name ),*
@@ -1241,6 +1251,7 @@ macro_rules! callsite {
     });
     (
         name: $name:expr,
+        kind: $kind:expr,
         target: $target:expr,
         level: $lvl:expr,
         fields: $( $field_name:expr ),*
@@ -1256,6 +1267,7 @@ macro_rules! callsite {
                 level: $lvl,
                 fields: &[ $( __tokio_trace_stringify!($field_name) ),* ],
                 callsite: &MyCallsite,
+                kind: $kind,
             }
         };
         // FIXME: Rust 1.34 deprecated ATOMIC_USIZE_INIT. When Tokio's minimum
diff --git a/tokio-trace/tests/macros.rs b/tokio-trace/tests/macros.rs
index 7da1c3ec..975920d1 100644
--- a/tokio-trace/tests/macros.rs
+++ b/tokio-trace/tests/macros.rs
@@ -365,3 +365,27 @@ fn error() {
     error!(target: "foo_events", { foo = 2, bar.baz = 79 }, "quux {:?}, {quux}", true, quux = false);
     error!(target: "foo_events", { foo = 2, bar.baz = 78, }, "quux");
 }
+
+#[test]
+fn callsite_macro_api() {
+    // This test should catch any inadvertant breaking changes
+    // caused bu changes to the macro.
+    let _callsite = callsite! {
+        name: "test callsite",
+        kind: tokio_trace::metadata::Kind::EVENT,
+        target: "test target",
+        level: tokio_trace::Level::TRACE,
+        fields: foo, bar,
+    };
+    let _callsite = callsite! {
+        name: "test callsite",
+        kind: tokio_trace::metadata::Kind::SPAN,
+        level: tokio_trace::Level::TRACE,
+        fields: foo,
+    };
+    let _callsite = callsite! {
+        name: "test callsite",
+        kind: tokio_trace::metadata::Kind::SPAN,
+        fields: foo,
+    };
+}
diff --git a/tokio-trace/tests/support/event.rs b/tokio-trace/tests/support/event.rs
index 5dbe2b23..de51848b 100644
--- a/tokio-trace/tests/support/event.rs
+++ b/tokio-trace/tests/support/event.rs
@@ -70,6 +70,7 @@ impl MockEvent {
         let meta = event.metadata();
         let name = meta.name();
         self.metadata.check(meta, format_args!("event {}", name));
+        assert!(meta.is_event(), "expected an event but got {:?}", event);
         if let Some(mut expected_fields) = self.fields {
             let mut checker = expected_fields.checker(format!("{}", name));
             event.record(&mut checker);
diff --git a/tokio-trace/tests/support/field.rs b/tokio-trace/tests/support/field.rs
index 48b862bf..469fac1e 100644
--- a/tokio-trace/tests/support/field.rs
+++ b/tokio-trace/tests/support/field.rs
@@ -1,6 +1,7 @@
 use tokio_trace::{
     callsite::Callsite,
     field::{self, Field, Value, Visit},
+    metadata::Kind,
 };
 
 use std::{collections::HashMap, fmt};
@@ -199,7 +200,7 @@ impl<'a> From<&'a Value> for MockValue {
             }
         }
 
-        let fake_field = callsite!(name: "fake", fields: fake_field)
+        let fake_field = callsite!(name: "fake", kind: Kind::EVENT, fields: fake_field)
             .metadata()
             .fields()
             .field("fake_field")
diff --git a/tokio-trace/tests/support/span.rs b/tokio-trace/tests/support/span.rs
index 92d9c229..8d5ce424 100644
--- a/tokio-trace/tests/support/span.rs
+++ b/tokio-trace/tests/support/span.rs
@@ -109,7 +109,8 @@ impl MockSpan {
     }
 
     pub(in support) fn check_metadata(&self, actual: &tokio_trace::Metadata) {
-        self.metadata.check(actual, format_args!("span {}", self))
+        self.metadata.check(actual, format_args!("span {}", self));
+        assert!(actual.is_span(), "expected a span but got {:?}", actual);
     }
 }
 
diff --git a/tokio-trace/tokio-trace-core/src/field.rs b/tokio-trace/tokio-trace-core/src/field.rs
index d6f61ff2..85615d6b 100644
--- a/tokio-trace/tokio-trace-core/src/field.rs
+++ b/tokio-trace/tokio-trace-core/src/field.rs
@@ -673,7 +673,7 @@ impl_valid_len! {
 #[cfg(test)]
 mod test {
     use super::*;
-    use {Level, Metadata};
+    use metadata::{Kind, Level, Metadata};
 
     struct TestCallsite1;
     static TEST_CALLSITE_1: TestCallsite1 = TestCallsite1;
@@ -683,6 +683,7 @@ mod test {
         level: Level::INFO,
         fields: &["foo", "bar", "baz"],
         callsite: &TEST_CALLSITE_1,
+        kind: Kind::SPAN,
     };
 
     impl ::callsite::Callsite for TestCallsite1 {
@@ -703,6 +704,7 @@ mod test {
         level: Level::INFO,
         fields: &["foo", "bar", "baz"],
         callsite: &TEST_CALLSITE_2,
+        kind: Kind::SPAN,
     };
 
     impl ::callsite::Callsite for TestCallsite2 {
diff --git a/tokio-trace/tokio-trace-core/src/lib.rs b/tokio-trace/tokio-trace-core/src/lib.rs
index b6babb07..46ddc17e 100644
--- a/tokio-trace/tokio-trace-core/src/lib.rs
+++ b/tokio-trace/tokio-trace-core/src/lib.rs
@@ -98,7 +98,7 @@ macro_rules! identify_callsite {
 /// # #[macro_use]
 /// # extern crate tokio_trace_core;
 /// # use tokio_trace_core::{callsite::Callsite, subscriber::Interest};
-/// use tokio_trace_core::{Metadata, Level};
+/// use tokio_trace_core::metadata::{Kind, Level, Metadata};
 /// # fn main() {
 /// # pub struct MyCallsite { }
 /// # impl Callsite for MyCallsite {
@@ -116,6 +116,7 @@ macro_rules! identify_callsite {
 ///     level: Level::DEBUG,
 ///     fields: &["bar", "baz"],
 ///     callsite: &FOO_CALLSITE,
+///     kind: Kind::SPAN,
 /// };
 /// # }
 /// ```
@@ -129,7 +130,8 @@ macro_rules! metadata {
         target: $target:expr,
         level: $level:expr,
         fields: $fields:expr,
-        callsite: $callsite:expr
+        callsite: $callsite:expr,
+        kind: $kind:expr
     ) => {
         metadata! {
             name: $name,
@@ -137,6 +139,7 @@ macro_rules! metadata {
             level: $level,
             fields: $fields,
             callsite: $callsite,
+            kind: $kind,
         }
     };
     (
@@ -145,6 +148,7 @@ macro_rules! metadata {
         level: $level:expr,
         fields: $fields:expr,
         callsite: $callsite:expr,
+        kind: $kind:expr,
     ) => {
         $crate::metadata::Metadata {
             name: $name,
@@ -157,6 +161,7 @@ macro_rules! metadata {
                 names: $fields,
                 callsite: identify_callsite!($callsite),
             },
+            kind: $kind,
         }
     };
 }
@@ -198,7 +203,7 @@ pub use self::{
     dispatcher::Dispatch,
     event::Event,
     field::Field,
-    metadata::{Level, Metadata},
+    metadata::{Kind, Level, Metadata},
     subscriber::{Interest, Subscriber},
 };
 
diff --git a/tokio-trace/tokio-trace-core/src/metadata.rs b/tokio-trace/tokio-trace-core/src/metadata.rs
index e7c656f3..1c546a7c 100644
--- a/tokio-trace/tokio-trace-core/src/metadata.rs
+++ b/tokio-trace/tokio-trace-core/src/metadata.rs
@@ -122,8 +122,24 @@ pub struct Metadata<'a> {
     /// `Metadata::new` constructor instead!
     #[doc(hidden)]
     pub fields: field::FieldSet,
+
+    /// The kind of the callsite.
+    ///
+    /// **Warning**: The fields on this type are currently `pub` because it must
+    /// be able to be constructed statically by macros. However, when `const
+    /// fn`s are available on stable Rust, this will no longer be necessary.
+    /// Thus, these fields are *not* considered stable public API, and they may
+    /// change warning. Do not rely on any fields on `Metadata`. When
+    /// constructing new `Metadata`, use the `metadata!` macro or the
+    /// `Metadata::new` constructor instead!
+    #[doc(hidden)]
+    pub kind: Kind,
 }
 
+/// Indicate whether the callsite is a span or event.
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct Kind(KindInner);
+
 /// Describes the level of verbosity of a span or event.
 #[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd)]
 pub struct Level(LevelInner);
@@ -142,6 +158,7 @@ impl<'a> Metadata<'a> {
         line: Option<u32>,
         field_names: &'static [&'static str],
         callsite: &'static Callsite,
+        kind: Kind,
     ) -> Self {
         Metadata {
             name,
@@ -154,6 +171,7 @@ impl<'a> Metadata<'a> {
                 names: field_names,
                 callsite: callsite::Identifier(callsite),
             },
+            kind,
         }
     }
 
@@ -205,6 +223,16 @@ impl<'a> Metadata<'a> {
     pub fn callsite(&self) -> callsite::Identifier {
         self.fields.callsite()
     }
+
+    /// Returns true if the callsite kind is `Event`.
+    pub fn is_event(&self) -> bool {
+        self.kind.is_event()
+    }
+
+    /// Return true if the callsite kind is `Span`.
+    pub fn is_span(&self) -> bool {
+        self.kind.is_span()
+    }
 }
 
 impl<'a> fmt::Debug for Metadata<'a> {
@@ -235,10 +263,41 @@ impl<'a> fmt::Debug for Metadata<'a> {
 
         meta.field("fields", &format_args!("{}", self.fields))
             .field("callsite", &self.callsite())
+            .field("kind", &self.kind)
             .finish()
     }
 }
 
+#[derive(Clone, Debug, Eq, PartialEq)]
+enum KindInner {
+    Event,
+    Span,
+}
+
+impl Kind {
+    /// `Event` callsite
+    pub const EVENT: Kind = Kind(KindInner::Event);
+
+    /// `Span` callsite
+    pub const SPAN: Kind = Kind(KindInner::Span);
+
+    /// Return true if the callsite kind is `Span`
+    pub fn is_span(&self) -> bool {
+        match self {
+            Kind(KindInner::Span) => true,
+            _ => false,
+        }
+    }
+
+    /// Return true if the callsite kind is `Event`
+    pub fn is_event(&self) -> bool {
+        match self {
+            Kind(KindInner::Event) => true,
+            _ => false,
+        }
+    }
+}
+
 // ===== impl Level =====
 
 impl Level {
diff --git a/tokio-trace/tokio-trace-core/tests/macros.rs b/tokio-trace/tokio-trace-core/tests/macros.rs
new file mode 100644
index 00000000..9fa43250
--- /dev/null
+++ b/tokio-trace/tokio-trace-core/tests/macros.rs
@@ -0,0 +1,49 @@
+#[macro_use]
+extern crate tokio_trace_core;
+use tokio_trace_core::{
+    callsite::Callsite,
+    metadata::{Kind, Level, Metadata},
+    subscriber::Interest,
+};
+
+#[test]
+fn metadata_macro_api() {
+    // This test should catch any inadvertant breaking changes
+    // caused bu changes to the macro.
+    struct TestCallsite;
+
+    impl Callsite for TestCallsite {
+        fn set_interest(&self, _: Interest) {
+            unimplemented!("test")
+        }
+        fn metadata(&self) -> &Metadata {
+            unimplemented!("test")
+        }
+    }
+
+    static CALLSITE: TestCallsite = TestCallsite;
+    let _metadata = metadata! {
+        name: "test_metadata",
+        target: "test_target",
+        level: Level::DEBUG,
+        fields: &["foo", "bar", "baz"],
+        callsite: &CALLSITE,
+        kind: Kind::SPAN,
+    };
+    let _metadata = metadata! {
+        name: "test_metadata",
+        target: "test_target",
+        level: Level::TRACE,
+        fields: &[],
+        callsite: &CALLSITE,
+        kind: Kind::EVENT,
+    };
+    let _metadata = metadata! {
+        name: "test_metadata",
+        target: "test_target",
+        level: Level::INFO,
+        fields: &[],
+        callsite: &CALLSITE,
+        kind: Kind::EVENT
+    };
+}