From 601d2973e6602c00602be47443c7f8e4667f0fb8 Mon Sep 17 00:00:00 2001 From: Julian Tescher Date: Sun, 27 Dec 2020 16:37:08 -0800 Subject: [PATCH] Store parent context in span builder (#399) The span builder currently holds a parent `SpanContext`, and `Context` information is passed separately when starting a span directly or via a builder. This poses a few problems, first you can assign a context which contains an active span _as well as_ a builder with a parent span context, which can lead to confusion, and secondly we have to construct a wrapper context when sampling even in cases when the passed in context already contains an active span. This patch resolves this by changing the span builder's parent context to instead store a `Context` directly. This allows the builder methods to have a sigle context to look at when searchig for parent span contexts, and allows the wrapper context to onlly be created for sampling when it is facilitating a remote parent. --- examples/aws-xray/src/server.rs | 2 +- examples/grpc/src/server.rs | 2 +- examples/http/src/server.rs | 2 +- opentelemetry/src/api/trace/noop.rs | 42 +++++----- opentelemetry/src/api/trace/tracer.rs | 30 +++----- opentelemetry/src/global/trace.rs | 18 ++--- opentelemetry/src/sdk/trace/sampler.rs | 44 +++++++---- opentelemetry/src/sdk/trace/span_processor.rs | 4 +- opentelemetry/src/sdk/trace/tracer.rs | 76 ++++++++++--------- 9 files changed, 117 insertions(+), 103 deletions(-) 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()); }