diff --git a/src/layer.rs b/src/layer.rs index 783f6eb..f8813eb 100644 --- a/src/layer.rs +++ b/src/layer.rs @@ -759,16 +759,30 @@ where /// [`span`]: tracing::Span /// [`Registry`]: tracing_subscriber::Registry fn parent_context(&self, attrs: &Attributes<'_>, ctx: &Context<'_, S>) -> OtelContext { - // If a span is specified, it _should_ exist in the underlying `Registry`. if let Some(parent) = attrs.parent() { - let span = ctx.span(parent).expect("Span not found, this is a bug"); - let mut extensions = span.extensions_mut(); - extensions - .get_mut::() - .map(|builder| self.tracer.sampled_context(builder)) - .unwrap_or_default() + // A span can have an _explicit_ parent that is NOT seen by this `Layer` (for which + // `Context::span` returns `None`. This happens if the parent span is filtered away + // from the layer by a per-layer filter. In that case, we fall-through to the `else` + // case, and consider this span a root span. + // + // This is likely rare, as most users who use explicit parents will configure their + // filters so that children and parents are both seen, but its not guaranteed. Also, + // if users configure their filter with a `reload` filter, its possible that a parent + // and child have different filters as they are created with a filter change + // in-between. + // + // In these case, we prefer to emit a smaller span tree instead of panicking. + if let Some(span) = ctx.span(parent) { + let mut extensions = span.extensions_mut(); + return extensions + .get_mut::() + .map(|builder| self.tracer.sampled_context(builder)) + .unwrap_or_default(); + } + } + // Else if the span is inferred from context, look up any available current span. - } else if attrs.is_contextual() { + if attrs.is_contextual() { ctx.lookup_current() .and_then(|span| { let mut extensions = span.extensions_mut(); @@ -948,25 +962,27 @@ where .get_mut::() .expect("Missing otel data span extensions"); - let follows_span = ctx - .span(follows) - .expect("Span to follow not found, this is a bug"); - let mut follows_extensions = follows_span.extensions_mut(); - let follows_data = follows_extensions - .get_mut::() - .expect("Missing otel data span extensions"); - - let follows_context = self - .tracer - .sampled_context(follows_data) - .span() - .span_context() - .clone(); - let follows_link = otel::Link::new(follows_context, Vec::new()); - if let Some(ref mut links) = data.builder.links { - links.push(follows_link); - } else { - data.builder.links = Some(vec![follows_link]); + // The follows span may be filtered away from this layer, in which case + // we just drop the data, as opposed to panicking. This uses the same + // reasoning as `parent_context` above. + if let Some(follows_span) = ctx.span(follows) { + let mut follows_extensions = follows_span.extensions_mut(); + let follows_data = follows_extensions + .get_mut::() + .expect("Missing otel data span extensions"); + + let follows_context = self + .tracer + .sampled_context(follows_data) + .span() + .span_context() + .clone(); + let follows_link = otel::Link::new(follows_context, Vec::new()); + if let Some(ref mut links) = data.builder.links { + links.push(follows_link); + } else { + data.builder.links = Some(vec![follows_link]); + } } } diff --git a/tests/filtered.rs b/tests/filtered.rs new file mode 100644 index 0000000..678d1a1 --- /dev/null +++ b/tests/filtered.rs @@ -0,0 +1,67 @@ +use futures_util::future::BoxFuture; +use opentelemetry::trace::TracerProvider as _; +use opentelemetry_sdk::{ + export::trace::{ExportResult, SpanData, SpanExporter}, + trace::{Tracer, TracerProvider}, +}; +use std::sync::{Arc, Mutex}; +use tracing::level_filters::LevelFilter; +use tracing::Subscriber; +use tracing_opentelemetry::layer; +use tracing_subscriber::prelude::*; + +#[derive(Clone, Default, Debug)] +struct TestExporter(Arc>>); + +impl SpanExporter for TestExporter { + fn export(&mut self, mut batch: Vec) -> BoxFuture<'static, ExportResult> { + let spans = self.0.clone(); + Box::pin(async move { + if let Ok(mut inner) = spans.lock() { + inner.append(&mut batch); + } + Ok(()) + }) + } +} + +fn test_tracer() -> (Tracer, TracerProvider, TestExporter, impl Subscriber) { + let exporter = TestExporter::default(); + let provider = TracerProvider::builder() + .with_simple_exporter(exporter.clone()) + .build(); + let tracer = provider.tracer("test"); + + let subscriber = tracing_subscriber::registry() + .with( + layer() + .with_tracer(tracer.clone()) + // DEBUG-level, so `trace_spans` are skipped. + .with_filter(LevelFilter::DEBUG), + ) + // This is REQUIRED so that the tracing fast path doesn't filter + // out trace spans at their callsite. + .with(tracing_subscriber::fmt::layer().with_filter(LevelFilter::TRACE)); + + (tracer, provider, exporter, subscriber) +} + +#[test] +fn trace_filtered() { + let (_tracer, provider, exporter, subscriber) = test_tracer(); + + tracing::subscriber::with_default(subscriber, || { + // Neither of these should panic + + let root = tracing::trace_span!("root"); + tracing::debug_span!(parent: &root, "child"); + + let root = tracing::trace_span!("root"); + root.in_scope(|| tracing::debug_span!("child")); + }); + + drop(provider); // flush all spans + let spans = exporter.0.lock().unwrap(); + // Only the child spans are reported. + assert_eq!(spans.len(), 2); +}