Skip to content

Commit

Permalink
subscriber: remove LookupMetadata trait (#564)
Browse files Browse the repository at this point in the history
## Motivation

The `LookupMetadata` trait was essentially a prototype for `LookupSpan`,
and was intended to allow subscribers to implement more granular sets of
"registry" behavior. However, in practice, nothing ended up using it and
it was generally eclipsed by `LookupSpan`. The model of allowing
`Layer` implementations to opt in to more granular capabilities doesn't
make a whole lot of sense when there are only two such traits available
and one is a more powerful superset of the other. Before we release
`tracing-subscriber` 0.2.0, we should try to remove any APIs that aren't
necessary, since removing them later is a breaking change.

## Solution

Therefore, this commit removes `LookupMetadata`, and rewrites the
`Context::metadata` method to use `LookupSpan` to look up the metadata,
instead.

Signed-off-by: Eliza Weisman <[email protected]>
  • Loading branch information
hawkw authored Feb 4, 2020
1 parent 737eba2 commit a661155
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 86 deletions.
6 changes: 3 additions & 3 deletions tracing-subscriber/src/fmt/fmt_layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -573,10 +573,10 @@ mod test {
}

#[test]
fn is_lookup_meta() {
fn assert_lookup_meta<T: crate::registry::LookupMetadata>(_: T) {}
fn is_lookup_span() {
fn assert_lookup_span<T: for<'a> crate::registry::LookupSpan<'a>>(_: T) {}
let fmt = fmt::Layer::builder().finish();
let subscriber = fmt.with_subscriber(Registry::default());
assert_lookup_meta(subscriber)
assert_lookup_span(subscriber)
}
}
10 changes: 4 additions & 6 deletions tracing-subscriber/src/fmt/format/json.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
use super::{Format, FormatEvent, FormatFields, FormatTime};
use crate::{
field::MakeVisitor,
fmt::fmt_layer::FmtContext,
fmt::fmt_layer::FormattedFields,
registry::{LookupMetadata, LookupSpan},
field::MakeVisitor, fmt::fmt_layer::FmtContext, fmt::fmt_layer::FormattedFields,
registry::LookupSpan,
};
use serde::ser::{SerializeMap, Serializer as _};
use serde_json::Serializer;
Expand All @@ -29,7 +27,7 @@ pub struct Json;

impl<S, N, T> FormatEvent<S, N> for Format<Json, T>
where
S: Subscriber + for<'lookup> LookupSpan<'lookup> + LookupMetadata,
S: Subscriber + for<'lookup> LookupSpan<'lookup>,
N: for<'writer> FormatFields<'writer> + 'static,
T: FormatTime,
{
Expand All @@ -40,7 +38,7 @@ where
event: &Event<'_>,
) -> fmt::Result
where
S: Subscriber + for<'a> LookupSpan<'a> + LookupMetadata,
S: Subscriber + for<'a> LookupSpan<'a>,
{
use serde_json::{json, Value};
use tracing_serde::fields::AsMap;
Expand Down
25 changes: 14 additions & 11 deletions tracing-subscriber/src/fmt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,11 @@ pub mod writer;
pub use fmt_layer::{FmtContext, FormattedFields, Layer, LayerBuilder};

use crate::layer::Layer as _;
use crate::{filter::LevelFilter, layer, registry::Registry};
use crate::{
filter::LevelFilter,
layer,
registry::{LookupSpan, Registry},
};

#[doc(inline)]
pub use self::{
Expand Down Expand Up @@ -277,15 +281,14 @@ where
}
}

#[cfg(feature = "registry")]
#[cfg_attr(docsrs, doc(cfg(feature = "registry")))]
impl<N, E, F, W> crate::registry::LookupMetadata for Subscriber<N, E, F, W>
impl<'a, N, E, F, W> LookupSpan<'a> for Subscriber<N, E, F, W>
where
layer::Layered<F, Formatter<N, E, W>>: crate::registry::LookupMetadata,
layer::Layered<F, Formatter<N, E, W>>: LookupSpan<'a>,
{
#[inline]
fn metadata(&self, id: &span::Id) -> Option<&'static Metadata<'static>> {
self.inner.metadata(id)
type Data = <layer::Layered<F, Formatter<N, E, W>> as LookupSpan<'a>>::Data;

fn span_data(&'a self, id: &span::Id) -> Option<Self::Data> {
self.inner.span_data(id)
}
}

Expand Down Expand Up @@ -808,9 +811,9 @@ mod test {
}

#[test]
fn is_lookup_meta() {
fn assert_lookup_meta<T: crate::registry::LookupMetadata>(_: T) {}
fn is_lookup_span() {
fn assert_lookup_span<T: for<'a> crate::registry::LookupSpan<'a>>(_: T) {}
let subscriber = Subscriber::new();
assert_lookup_meta(subscriber)
assert_lookup_span(subscriber)
}
}
26 changes: 12 additions & 14 deletions tracing-subscriber/src/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use tracing_core::{
};

#[cfg(feature = "registry")]
use crate::registry::{self, LookupMetadata, LookupSpan, Registry, SpanRef};
use crate::registry::{self, LookupSpan, Registry, SpanRef};
use std::{any::TypeId, marker::PhantomData};

/// A composable handler for `tracing` events.
Expand Down Expand Up @@ -728,22 +728,23 @@ impl<'a, S: Subscriber> Context<'a, S> {
/// closed or the ID is invalid).
///
/// **Note**: This requires the wrapped subscriber to implement the
/// [`LookupMetadata`] trait. `Layer` implementations that wish to use this
/// [`LookupSpan`] trait. `Layer` implementations that wish to use this
/// function can bound their `Subscriber` type parameter with
/// ```rust,ignore
/// where S: Subscriber + LookupMetadata,
/// where S: Subscriber + for<'a> LookupSpan<'a>,
/// ```
/// or similar.
///
/// [`LookupMetadata`]: ../registry/trait.LookupMetadata.html
/// [`LookupSpan`]: ../registry/trait.LookupSpan.html
#[inline]
#[cfg(feature = "registry")]
#[cfg_attr(docsrs, doc(cfg(feature = "registry")))]
pub fn metadata(&self, id: &span::Id) -> Option<&'static Metadata<'static>>
where
S: LookupMetadata,
S: for<'lookup> LookupSpan<'lookup>,
{
self.subscriber.as_ref()?.metadata(id)
let span = self.subscriber.as_ref()?.span(id)?;
Some(span.metadata())
}

/// Returns [stored data] for the span with the given `id`, if it exists.
Expand Down Expand Up @@ -774,25 +775,22 @@ impl<'a, S: Subscriber> Context<'a, S> {
/// Returns `true` if an active span exists for the given `Id`.
///
/// **Note**: This requires the wrapped subscriber to implement the
/// [`LookupMetadata`] trait. `Layer` implementations that wish to use this
/// [`LookupSpan`] trait. `Layer` implementations that wish to use this
/// function can bound their `Subscriber` type parameter with
/// ```rust,ignore
/// where S: Subscriber + LookupMetadata,
/// where S: Subscriber + for<'a> LookupSpan<'a>,
/// ```
/// or similar.
///
/// [`LookupMetadata`]: ../registry/trait.LookupMetadata.html
/// [`LookupSpan`]: ../registry/trait.LookupSpan.html
#[inline]
#[cfg(feature = "registry")]
#[cfg_attr(docsrs, doc(cfg(feature = "registry")))]
pub fn exists(&self, id: &span::Id) -> bool
where
S: LookupMetadata,
S: for<'lookup> LookupSpan<'lookup>,
{
self.subscriber
.as_ref()
.map(|s| s.exists(id))
.unwrap_or(false)
self.subscriber.as_ref().and_then(|s| s.span(id)).is_some()
}

/// Returns [stored data] for the span that the wrapped subscriber considers
Expand Down
47 changes: 2 additions & 45 deletions tracing-subscriber/src/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@
//! [`layer::Context`][ctx] type will provide methods allowing `Layer`s to
//! [look up span data][lookup] stored in the registry. While [`Registry`] is a
//! reasonable default for storing spans and events, other stores that implement
//! [`LookupSpan`], [`LookupMetadata`], and [`Subscriber`] on themselves (with
//! [`SpanData`] implemented on the data _inside_ the store) can be used as a
//! drop-in replacement.
//! [`LookupSpan`] and [`Subscriber`] themselves (with [`SpanData`] implemented
//! by the per-span data they store) can be used as a drop-in replacement.
//!
//! For example, we might create a `Registry` and add multiple `Layer`s like so:
//! ```rust
Expand Down Expand Up @@ -62,7 +61,6 @@
//! [ctx]: ../layer/struct.Context.html
//! [lookup]: ../layer/struct.Context.html#method.span
//! [`LookupSpan`]: trait.LookupSpan.html
//! [`LookupMetadata`]: trait.LookupMetadata.html
//! [`SpanData`]: trait.SpanData.html
use tracing_core::{field::FieldSet, span::Id, Metadata};

Expand All @@ -81,38 +79,6 @@ pub use sharded::Data;
#[cfg_attr(docsrs, doc(cfg(feature = "registry")))]
pub use sharded::Registry;

/// Provides access to stored span metadata.
///
/// Subscribers which store span metadata and associate it with span IDs should
/// implement this trait; if they do, any [`Layer`]s wrapping them can look up
/// metadata via the [`Context`] type's [`metadata()`] method.
///
/// [`Layer`]: ../layer/trait.Layer.html
/// [`Context`]: ../layer/struct.Context.html
/// [`metadata()`]: ../layer/struct.Context.html#method.metadata
pub trait LookupMetadata {
/// Returns metadata for tne span with the given `id`, if it exists.
///
/// If no span exists for the provided ID (e.g. the span has closed and been
/// removed from the registry, or the ID is invalid), this should return `None`.
fn metadata(&self, id: &Id) -> Option<&'static Metadata<'static>>;

/// Returns `true` if a span with the given `id` exists, false otherwise.
///
/// **Note**: The default implementation of this method is simply:
///```rust,ignore
/// fn exists(&self, id: &span::Id) -> bool {
/// self.metadata(id).is_some()
/// }
///```
/// If the subscriber has a faster way of determining whether a span exists
/// for a given ID (e.g., if the ID is greater than the current value of an
/// increasing ID counter, etc), this method may be overridden as an optimization.
fn exists(&self, id: &Id) -> bool {
self.metadata(id).is_some()
}
}

/// Provides access to stored span data.
///
/// Subscribers which store span data and associate it with span IDs should
Expand Down Expand Up @@ -333,15 +299,6 @@ where
}
}

impl<L> LookupMetadata for L
where
L: for<'a> LookupSpan<'a>,
{
fn metadata(&self, id: &Id) -> Option<&'static Metadata<'static>> {
self.span_data(id).map(|data| data.metadata())
}
}

// === impl FromRoot ===

impl<'span, R> Iterator for FromRoot<'span, R>
Expand Down
13 changes: 6 additions & 7 deletions tracing-subscriber/src/registry/sharded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ use tracing_core::{
/// implementing various behaviors may be [added]. Unlike other types
/// implementing `Subscriber` `Registry` does not actually record traces itself:
/// instead, it collects and stores span data that is exposed to any `Layer`s
/// wrapping it through implementations of the [`LookupSpan`] and
/// [`LookupMetadata`] traits. The `Registry` is responsible for storing span
/// metadata, recording relationships between spans, and tracking which spans
/// are active and whicb are closed. In addition, it provides a mechanism
/// `Layer`s to store user-defined per-span data, called [extensions], in the
/// registry. This allows `Layer`-specific data to benefit from the `Registry`'s
/// wrapping it through implementations of the [`LookupSpan`] trait.
/// The `Registry` is responsible for storing span metadata, recording
/// relationships between spans, and tracking which spans are active and whicb
/// are closed. In addition, it provides a mechanism for `Layer`s to store
/// user-defined per-span data, called [extensions], in the registry. This
/// allows `Layer`-specific data to benefit from the `Registry`'s
/// high-performance concurrent storage.
///
/// This registry is implemented using a [lock-free sharded slab][slab], and is
Expand All @@ -41,7 +41,6 @@ use tracing_core::{
/// [`Layer`]: ../trait.Layer.html
/// [added]: ../trait.Layer.html#method.with_subscriber
/// [`LookupSpan`]: trait.LookupSpan.html
/// [`LookupMetadata`]: trait.LookupMetadata.html
/// [extensions]: extensions/index.html
#[cfg(feature = "registry")]
#[cfg_attr(docsrs, doc(cfg(feature = "registry")))]
Expand Down

0 comments on commit a661155

Please sign in to comment.