From 847fb59b17d8e9786abfdac474ad59e120a94254 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Thu, 11 Apr 2019 11:56:42 -0700 Subject: [PATCH] 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 --- tokio-trace/src/macros.rs | 16 +++++- tokio-trace/tests/macros.rs | 24 ++++++++ tokio-trace/tests/support/event.rs | 1 + tokio-trace/tests/support/field.rs | 3 +- tokio-trace/tests/support/span.rs | 3 +- tokio-trace/tokio-trace-core/src/field.rs | 4 +- tokio-trace/tokio-trace-core/src/lib.rs | 11 +++- tokio-trace/tokio-trace-core/src/metadata.rs | 59 ++++++++++++++++++++ tokio-trace/tokio-trace-core/tests/macros.rs | 49 ++++++++++++++++ 9 files changed, 162 insertions(+), 8 deletions(-) create mode 100644 tokio-trace/tokio-trace-core/tests/macros.rs diff --git a/tokio-trace/src/macros.rs b/tokio-trace/src/macros.rs index e3e631ea4..3f5018f95 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 7da1c3ec2..975920d1d 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 5dbe2b235..de51848be 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 48b862bfa..469fac1e1 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 92d9c2293..8d5ce4240 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 d6f61ff2d..85615d6b8 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 b6babb078..46ddc17ed 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 e7c656f34..1c546a7cc 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, 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 000000000..9fa43250e --- /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 + }; +}