diff --git a/examples/aws-xray/src/server.rs b/examples/aws-xray/src/server.rs index 1631084534..8f27e89ba0 100644 --- a/examples/aws-xray/src/server.rs +++ b/examples/aws-xray/src/server.rs @@ -20,7 +20,7 @@ async fn handle(req: Request) -> Result, Infallible> { .to_str() .unwrap(); - let span = global::tracer("example/server").start_from_context("hello", &parent_context); + let span = global::tracer("example/server").start_with_context("hello", parent_context); span.add_event(format!("Handling - {}", x_amzn_trace_id), Vec::new()); Ok(Response::new( diff --git a/examples/grpc/src/server.rs b/examples/grpc/src/server.rs index d066bdd99d..426916e333 100644 --- a/examples/grpc/src/server.rs +++ b/examples/grpc/src/server.rs @@ -25,7 +25,7 @@ impl Greeter for MyGreeter { request: Request, // Accept request of type HelloRequest ) -> Result, Status> { let parent_cx = global::get_text_map_propagator(|prop| prop.extract(request.metadata())); - let span = global::tracer("greeter").start_from_context("Processing reply", &parent_cx); + let span = global::tracer("greeter").start_with_context("Processing reply", parent_cx); span.set_attribute(KeyValue::new("request", format!("{:?}", request))); // Return an instance of type HelloReply diff --git a/examples/http/src/server.rs b/examples/http/src/server.rs index 5e116824f7..1c9f74f273 100644 --- a/examples/http/src/server.rs +++ b/examples/http/src/server.rs @@ -13,7 +13,7 @@ use std::{convert::Infallible, net::SocketAddr}; async fn handle(req: Request) -> Result, Infallible> { let parent_cx = global::get_text_map_propagator(|propagator| propagator.extract(req.headers())); - let span = global::tracer("example/server").start_from_context("hello", &parent_cx); + let span = global::tracer("example/server").start_with_context("hello", parent_cx); span.add_event("handling this...".to_string(), Vec::new()); Ok(Response::new("Hello, World!".into())) diff --git a/opentelemetry/src/api/trace/noop.rs b/opentelemetry/src/api/trace/noop.rs index 66812c9c91..6b67f0855f 100644 --- a/opentelemetry/src/api/trace/noop.rs +++ b/opentelemetry/src/api/trace/noop.rs @@ -129,12 +129,13 @@ impl trace::Tracer for NoopTracer { trace::NoopSpan::new() } - /// Starts a new `NoopSpan` in a given context. + /// Starts a new `NoopSpan` with a given context. /// /// If the context contains a valid span context, it is propagated. - fn start_from_context(&self, name: &str, cx: &Context) -> Self::Span { - let builder = self.span_builder(name); - self.build_with_context(builder, cx) + fn start_with_context(&self, name: &str, cx: Context) -> Self::Span { + let mut builder = self.span_builder(name); + builder.parent_context = Some(cx); + self.build(builder) } /// Starts a `SpanBuilder`. @@ -145,18 +146,14 @@ impl trace::Tracer for NoopTracer { /// Builds a `NoopSpan` from a `SpanBuilder`. /// /// If the span builder or context contains a valid span context, it is propagated. - fn build_with_context(&self, mut builder: trace::SpanBuilder, cx: &Context) -> Self::Span { - let parent_span_context = builder - .parent_context - .take() - .or_else(|| { - if cx.has_active_span() { - Some(cx.span().span_context().clone()) - } else { - None - } - }) - .or_else(|| cx.remote_span_context().cloned()); + fn build(&self, mut builder: trace::SpanBuilder) -> Self::Span { + let parent_span_context = builder.parent_context.take().and_then(|parent_cx| { + if parent_cx.has_active_span() { + Some(parent_cx.span().span_context().clone()) + } else { + parent_cx.remote_span_context().cloned() + } + }); if let Some(span_context) = parent_span_context { trace::NoopSpan { span_context } } else { @@ -190,6 +187,7 @@ impl SpanExporter for NoopSpanExporter { #[cfg(test)] mod tests { use super::*; + use crate::testing::trace::TestSpan; use crate::trace::{self, Span, Tracer}; fn valid_span_context() -> trace::SpanContext { @@ -205,15 +203,17 @@ mod tests { #[test] fn noop_tracer_defaults_to_invalid_span() { let tracer = NoopTracer::new(); - let span = tracer.start_from_context("foo", &Context::new()); + let span = tracer.start_with_context("foo", Context::new()); assert!(!span.span_context().is_valid()); } #[test] fn noop_tracer_propagates_valid_span_context_from_builder() { let tracer = NoopTracer::new(); - let builder = tracer.span_builder("foo").with_parent(valid_span_context()); - let span = tracer.build_with_context(builder, &Context::new()); + let builder = tracer + .span_builder("foo") + .with_parent_context(Context::current_with_span(TestSpan(valid_span_context()))); + let span = tracer.build(builder); assert!(span.span_context().is_valid()); } @@ -223,7 +223,7 @@ mod tests { let cx = Context::new().with_span(NoopSpan { span_context: valid_span_context(), }); - let span = tracer.start_from_context("foo", &cx); + let span = tracer.start_with_context("foo", cx); assert!(span.span_context().is_valid()); } @@ -231,7 +231,7 @@ mod tests { fn noop_tracer_propagates_valid_span_context_from_remote_span_context() { let tracer = NoopTracer::new(); let cx = Context::new().with_remote_span_context(valid_span_context()); - let span = tracer.start_from_context("foo", &cx); + let span = tracer.start_with_context("foo", cx); assert!(span.span_context().is_valid()); } } diff --git a/opentelemetry/src/api/trace/tracer.rs b/opentelemetry/src/api/trace/tracer.rs index a52084d40b..720f420c27 100644 --- a/opentelemetry/src/api/trace/tracer.rs +++ b/opentelemetry/src/api/trace/tracer.rs @@ -1,8 +1,6 @@ use crate::sdk; use crate::{ - trace::{ - Event, Link, Span, SpanContext, SpanId, SpanKind, StatusCode, TraceContextExt, TraceId, - }, + trace::{Event, Link, Span, SpanId, SpanKind, StatusCode, TraceContextExt, TraceId}, Context, KeyValue, }; use std::fmt; @@ -35,19 +33,20 @@ use std::time::SystemTime; /// Spans can be created and nested manually: /// /// ``` -/// use opentelemetry::{global, trace::{Span, Tracer}}; +/// use opentelemetry::{global, trace::{Span, Tracer, TraceContextExt}, Context}; /// /// let tracer = global::tracer("my-component"); /// /// let parent = tracer.start("foo"); +/// let parent_cx = Context::current_with_span(parent); /// let child = tracer.span_builder("bar") -/// .with_parent(parent.span_context().clone()) +/// .with_parent_context(parent_cx.clone()) /// .start(&tracer); /// /// // ... /// /// child.end(); -/// parent.end(); +/// drop(parent_cx) // end parent /// ``` /// /// Spans can also use the current thread's [`Context`] to track which span is active: @@ -189,10 +188,10 @@ pub trait Tracer: fmt::Debug + 'static { /// `is_remote` to true on a parent `SpanContext` so `Span` creation knows if the /// parent is remote. fn start(&self, name: &str) -> Self::Span { - self.start_from_context(name, &Context::current()) + self.start_with_context(name, Context::current()) } - /// Starts a new `Span` in a given context + /// Starts a new `Span` with a given context /// /// By default the currently active `Span` is set as the new `Span`'s /// parent. The `Tracer` MAY provide other default options for newly @@ -215,7 +214,7 @@ pub trait Tracer: fmt::Debug + 'static { /// created in another process. Each propagators' deserialization must set /// `is_remote` to true on a parent `SpanContext` so `Span` creation knows if the /// parent is remote. - fn start_from_context(&self, name: &str, context: &Context) -> Self::Span; + fn start_with_context(&self, name: &str, context: Context) -> Self::Span; /// Creates a span builder /// @@ -223,12 +222,7 @@ pub trait Tracer: fmt::Debug + 'static { fn span_builder(&self, name: &str) -> SpanBuilder; /// Create a span from a `SpanBuilder` - fn build(&self, builder: SpanBuilder) -> Self::Span { - self.build_with_context(builder, &Context::current()) - } - - /// Create a span from a `SpanBuilder` - fn build_with_context(&self, builder: SpanBuilder, cx: &Context) -> Self::Span; + fn build(&self, builder: SpanBuilder) -> Self::Span; /// Start a new span and execute the given closure with reference to the span's /// context. @@ -335,8 +329,8 @@ pub trait Tracer: fmt::Debug + 'static { /// ``` #[derive(Clone, Debug, Default)] pub struct SpanBuilder { - /// Parent `SpanContext` - pub parent_context: Option, + /// Parent `Context` + pub parent_context: Option, /// Trace id, useful for integrations with external tracing systems. pub trace_id: Option, /// Span id, useful for integrations with external tracing systems. @@ -385,7 +379,7 @@ impl SpanBuilder { } /// Assign parent context - pub fn with_parent(self, parent_context: SpanContext) -> Self { + pub fn with_parent_context(self, parent_context: Context) -> Self { SpanBuilder { parent_context: Some(parent_context), ..self diff --git a/opentelemetry/src/global/trace.rs b/opentelemetry/src/global/trace.rs index 319ebaa1ea..8b48277621 100644 --- a/opentelemetry/src/global/trace.rs +++ b/opentelemetry/src/global/trace.rs @@ -92,7 +92,7 @@ impl trace::Tracer for BoxedTracer { /// trace. A span is said to be a _root span_ if it does not have a parent. Each /// trace includes a single root span, which is the shared ancestor of all other /// spans in the trace. - fn start_from_context(&self, name: &str, cx: &Context) -> Self::Span { + fn start_with_context(&self, name: &str, cx: Context) -> Self::Span { BoxedSpan(self.0.start_with_context_boxed(name, cx)) } @@ -104,8 +104,8 @@ impl trace::Tracer for BoxedTracer { } /// Create a span from a `SpanBuilder` - fn build_with_context(&self, builder: trace::SpanBuilder, cx: &Context) -> Self::Span { - BoxedSpan(self.0.build_with_context_boxed(builder, cx)) + fn build(&self, builder: trace::SpanBuilder) -> Self::Span { + BoxedSpan(self.0.build_boxed(builder)) } } @@ -120,11 +120,11 @@ pub trait GenericTracer: fmt::Debug + 'static { /// Returns a trait object so the underlying implementation can be swapped /// out at runtime. - fn start_with_context_boxed(&self, name: &str, cx: &Context) -> Box; + fn start_with_context_boxed(&self, name: &str, cx: Context) -> Box; /// Returns a trait object so the underlying implementation can be swapped /// out at runtime. - fn build_with_context_boxed(&self, builder: trace::SpanBuilder, cx: &Context) -> Box; + fn build_boxed(&self, builder: trace::SpanBuilder) -> Box; } impl GenericTracer for T @@ -139,14 +139,14 @@ where /// Returns a trait object so the underlying implementation can be swapped /// out at runtime. - fn start_with_context_boxed(&self, name: &str, cx: &Context) -> Box { - Box::new(self.start_from_context(name, cx)) + fn start_with_context_boxed(&self, name: &str, cx: Context) -> Box { + Box::new(self.start_with_context(name, cx)) } /// Returns a trait object so the underlying implementation can be swapped /// out at runtime. - fn build_with_context_boxed(&self, builder: trace::SpanBuilder, cx: &Context) -> Box { - Box::new(self.build_with_context(builder, cx)) + fn build_boxed(&self, builder: trace::SpanBuilder) -> Box { + Box::new(self.build(builder)) } } diff --git a/opentelemetry/src/sdk/trace/sampler.rs b/opentelemetry/src/sdk/trace/sampler.rs index 670145d082..1e40022520 100644 --- a/opentelemetry/src/sdk/trace/sampler.rs +++ b/opentelemetry/src/sdk/trace/sampler.rs @@ -113,19 +113,21 @@ impl ShouldSample for Sampler { // Never sample the trace Sampler::AlwaysOff => SamplingDecision::Drop, // The parent decision if sampled; otherwise the decision of delegate_sampler - Sampler::ParentBased(delegate_sampler) => parent_context.map_or( - delegate_sampler - .should_sample(parent_context, trace_id, name, span_kind, attributes, links) - .decision, - |ctx| { - let parent_span_context = ctx.span().span_context(); - if parent_span_context.is_sampled() { - SamplingDecision::RecordAndSample - } else { - SamplingDecision::Drop - } - }, - ), + Sampler::ParentBased(delegate_sampler) => { + parent_context.filter(|cx| cx.has_active_span()).map_or( + delegate_sampler + .should_sample(parent_context, trace_id, name, span_kind, attributes, links) + .decision, + |ctx| { + let parent_span_context = ctx.span().span_context(); + if parent_span_context.is_sampled() { + SamplingDecision::RecordAndSample + } else { + SamplingDecision::Drop + } + }, + ) + } // Probabilistically sample the trace. Sampler::TraceIdRatioBased(prob) => { if *prob >= 1.0 { @@ -270,4 +272,20 @@ mod tests { ); } } + + #[test] + fn filter_parent_sampler_for_active_spans() { + let sampler = Sampler::ParentBased(Box::new(Sampler::AlwaysOn)); + let cx = Context::current_with_value("some_value"); + let result = sampler.should_sample( + Some(&cx), + TraceId::from_u128(1), + "should sample", + &SpanKind::Internal, + &[], + &[], + ); + + assert_eq!(result.decision, SamplingDecision::RecordAndSample); + } } diff --git a/opentelemetry/src/sdk/trace/span_processor.rs b/opentelemetry/src/sdk/trace/span_processor.rs index c69b63b072..83ad2febb1 100644 --- a/opentelemetry/src/sdk/trace/span_processor.rs +++ b/opentelemetry/src/sdk/trace/span_processor.rs @@ -695,10 +695,8 @@ mod tests { D: Fn(time::Duration) -> DS + 'static + Send + Sync, DS: Future + Send + Sync + 'static, { - async fn export(&mut self, batch: Vec) -> ExportResult { - println!("Accepting {} spans", batch.len()); + async fn export(&mut self, _batch: Vec) -> ExportResult { (self.delay_fn)(self.delay_for).await; - println!("Finish exporting, return result from exporter"); Ok(()) } } diff --git a/opentelemetry/src/sdk/trace/tracer.rs b/opentelemetry/src/sdk/trace/tracer.rs index af100d4a6c..29d54cab20 100644 --- a/opentelemetry/src/sdk/trace/tracer.rs +++ b/opentelemetry/src/sdk/trace/tracer.rs @@ -67,32 +67,26 @@ impl Tracer { #[allow(clippy::too_many_arguments)] fn make_sampling_decision( &self, - parent_context: Option<&SpanContext>, + parent_cx: Option<&Context>, trace_id: TraceId, name: &str, span_kind: &SpanKind, attributes: &[KeyValue], links: &[Link], - ctx: &Context, ) -> Option<(u8, Vec, TraceState)> { let provider = self.provider()?; let sampler = &provider.config().default_sampler; - let ctx = parent_context.map(|span_context| { - let span = Span::new(span_context.clone(), None, self.clone()); - ctx.with_span(span) - }); - let sampling_result = - sampler.should_sample(ctx.as_ref(), trace_id, name, span_kind, attributes, links); + sampler.should_sample(parent_cx, trace_id, name, span_kind, attributes, links); - self.process_sampling_result(sampling_result, parent_context) + self.process_sampling_result(sampling_result, parent_cx) } fn process_sampling_result( &self, sampling_result: SamplingResult, - parent_context: Option<&SpanContext>, + parent_cx: Option<&Context>, ) -> Option<(u8, Vec, TraceState)> { match sampling_result { SamplingResult { @@ -104,7 +98,9 @@ impl Tracer { attributes, trace_state, } => { - let trace_flags = parent_context.map(|ctx| ctx.trace_flags()).unwrap_or(0); + let trace_flags = parent_cx + .map(|ctx| ctx.span().span_context().trace_flags()) + .unwrap_or(0); Some((trace_flags & !TRACE_FLAG_SAMPLED, attributes, trace_state)) } SamplingResult { @@ -112,7 +108,9 @@ impl Tracer { attributes, trace_state, } => { - let trace_flags = parent_context.map(|ctx| ctx.trace_flags()).unwrap_or(0); + let trace_flags = parent_cx + .map(|ctx| ctx.span().span_context().trace_flags()) + .unwrap_or(0); Some((trace_flags | TRACE_FLAG_SAMPLED, attributes, trace_state)) } } @@ -129,17 +127,18 @@ impl crate::trace::Tracer for Tracer { Span::new(SpanContext::empty_context(), None, self.clone()) } - /// Starts a new `Span` in a given context. + /// Starts a new `Span` with a given context. /// /// Each span has zero or one parent spans and zero or more child spans, which /// represent causally related operations. A tree of related spans comprises a /// trace. A span is said to be a _root span_ if it does not have a parent. Each /// trace includes a single root span, which is the shared ancestor of all other /// spans in the trace. - fn start_from_context(&self, name: &str, cx: &Context) -> Self::Span { - let builder = self.span_builder(name); + fn start_with_context(&self, name: &str, cx: Context) -> Self::Span { + let mut builder = self.span_builder(name); + builder.parent_context = Some(cx); - self.build_with_context(builder, cx) + self.build(builder) } /// Creates a span builder @@ -156,7 +155,7 @@ impl crate::trace::Tracer for Tracer { /// trace. A span is said to be a _root span_ if it does not have a parent. Each /// trace includes a single root span, which is the shared ancestor of all other /// spans in the trace. - fn build_with_context(&self, mut builder: SpanBuilder, cx: &Context) -> Self::Span { + fn build(&self, mut builder: SpanBuilder) -> Self::Span { let provider = self.provider(); if provider.is_none() { return Span::new(SpanContext::empty_context(), None, self.clone()); @@ -175,17 +174,24 @@ impl crate::trace::Tracer for Tracer { let mut flags = 0; let mut span_trace_state = Default::default(); - let parent_span_context = builder - .parent_context - .as_ref() - .or_else(|| { - if cx.has_active_span() { - Some(cx.span().span_context()) - } else { - None + let parent_cx = builder.parent_context.take().map(|cx| { + // Sampling expects to be able to access the parent span via `span` so wrap remote span + // context in a wrapper span if necessary. Remote span contexts will be passed to + // subsequent context's, so wrapping is only necessary if there is no active span. + match cx.remote_span_context() { + Some(remote_sc) if !cx.has_active_span() => { + cx.with_span(Span::new(remote_sc.clone(), None, self.clone())) } - }) - .or_else(|| cx.remote_span_context()); + _ => cx, + } + }); + let parent_span_context = parent_cx.as_ref().and_then(|parent_cx| { + if parent_cx.has_active_span() { + Some(parent_cx.span().span_context()) + } else { + None + } + }); // Build context for sampling decision let (no_parent, trace_id, parent_span_id, remote_parent, parent_trace_flags) = parent_span_context @@ -215,16 +221,15 @@ impl crate::trace::Tracer for Tracer { // * There is no parent or a remote parent, in which case make decision now // * There is a local parent, in which case defer to the parent's decision let sampling_decision = if let Some(sampling_result) = builder.sampling_result.take() { - self.process_sampling_result(sampling_result, parent_span_context) + self.process_sampling_result(sampling_result, parent_cx.as_ref()) } else if no_parent || remote_parent { self.make_sampling_decision( - parent_span_context, + parent_cx.as_ref(), trace_id, &builder.name, &span_kind, &attribute_options, link_options.as_deref().unwrap_or(&[]), - cx, ) } else { // has parent that is local: use parent if sampled, or don't record. @@ -283,7 +288,7 @@ impl crate::trace::Tracer for Tracer { // Call `on_start` for all processors for processor in provider.span_processors() { - processor.on_start(&span, cx) + processor.on_start(&span, parent_cx.as_ref().unwrap_or(&Context::new())) } span @@ -341,19 +346,18 @@ mod tests { .with_config(config) .build(); let tracer = tracer_provider.get_tracer("test", None); - let context = Context::default(); let trace_state = TraceState::from_key_value(vec![("foo", "bar")]).unwrap(); let mut span_builder = SpanBuilder::default(); - span_builder.parent_context = Some(SpanContext::new( + span_builder.parent_context = Some(Context::current_with_span(TestSpan(SpanContext::new( TraceId::from_u128(128), SpanId::from_u64(64), TRACE_FLAG_SAMPLED, true, trace_state, - )); + )))); // Test sampler should change trace state - let span = tracer.build_with_context(span_builder, &context); + let span = tracer.build(span_builder); let span_context = span.span_context(); let expected = span_context.trace_state(); assert_eq!(expected.get("foo"), Some("notbar")) @@ -369,7 +373,7 @@ mod tests { let context = Context::current_with_span(TestSpan(SpanContext::empty_context())); let tracer = tracer_provider.get_tracer("test", None); - let span = tracer.start_from_context("must_not_be_sampled", &context); + let span = tracer.start_with_context("must_not_be_sampled", context); assert!(!span.span_context().is_sampled()); }