From f7db0507de5941c3b3cb8dd805e9ad6729bb5fd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20K=C3=BChle?= Date: Tue, 6 Apr 2021 05:09:58 +0200 Subject: [PATCH] Use current span for SDK-less context propagation (#510) In the absence of an SDK the API should not record spans anymore but it should propagate the span context. In our case absence of an SDK means we're using the `NoopTracer`. There are different ways to create new spans and they provide different ways of specifying the parent context: - Tracer::start: uses Context::current() - Tracer::build: uses the context from the span builder or falls back to the current context. BUT it did not have the fallback in the NoopTracer. - Tracer::start_with_context: uses the specified context This changes the `SpanBuilder` to make the `parent_context` in it non-optional. That way the current context fallback is done on `SpanBuilder` creation. Tracer implementations just need to read the `parparent_context` from the span builder. --- opentelemetry/src/sdk/trace/tracer.rs | 51 ++++++++++++++------------- opentelemetry/src/trace/noop.rs | 27 +++++++------- opentelemetry/src/trace/tracer.rs | 14 ++++++-- 3 files changed, 52 insertions(+), 40 deletions(-) diff --git a/opentelemetry/src/sdk/trace/tracer.rs b/opentelemetry/src/sdk/trace/tracer.rs index a0df867e15..1ba8cb01d4 100644 --- a/opentelemetry/src/sdk/trace/tracer.rs +++ b/opentelemetry/src/sdk/trace/tracer.rs @@ -139,10 +139,7 @@ impl crate::trace::Tracer for Tracer { where T: Into>, { - let mut builder = self.span_builder(name); - builder.parent_context = Some(cx); - - self.build(builder) + self.build(SpanBuilder::from_name_with_context(name, cx)) } /// Creates a span builder @@ -181,12 +178,8 @@ impl crate::trace::Tracer for Tracer { let mut flags = 0; let mut span_trace_state = Default::default(); - let parent_cx = builder - .parent_context - .take() - .unwrap_or_else(Context::current); - let parent_span = if parent_cx.has_active_span() { - Some(parent_cx.span()) + let parent_span = if builder.parent_context.has_active_span() { + Some(builder.parent_context.span()) } else { None }; @@ -220,10 +213,10 @@ 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_cx) + self.process_sampling_result(sampling_result, &builder.parent_context) } else if no_parent || remote_parent { self.make_sampling_decision( - &parent_cx, + &builder.parent_context, trace_id, &builder.name, &span_kind, @@ -245,6 +238,16 @@ impl crate::trace::Tracer for Tracer { }; // Build optional inner context, `None` if not recording. + let SpanBuilder { + parent_context, + name, + start_time, + end_time, + message_events, + status_code, + status_message, + .. + } = builder; let inner = sampling_decision.map(|(trace_flags, mut extra_attrs, trace_state)| { flags = trace_flags; span_trace_state = trace_state; @@ -258,23 +261,23 @@ impl crate::trace::Tracer for Tracer { if let Some(link_options) = &mut link_options { links.append_vec(link_options); } - let start_time = builder.start_time.unwrap_or_else(crate::time::now); - let end_time = builder.end_time.unwrap_or(start_time); - let mut message_events = EvictedQueue::new(config.max_events_per_span); - if let Some(mut events) = builder.message_events { - message_events.append_vec(&mut events); + let start_time = start_time.unwrap_or_else(crate::time::now); + let end_time = end_time.unwrap_or(start_time); + let mut message_events_queue = EvictedQueue::new(config.max_events_per_span); + if let Some(mut events) = message_events { + message_events_queue.append_vec(&mut events); } - let status_code = builder.status_code.unwrap_or(StatusCode::Unset); - let status_message = builder.status_message.unwrap_or(Cow::Borrowed("")); + let status_code = status_code.unwrap_or(StatusCode::Unset); + let status_message = status_message.unwrap_or(Cow::Borrowed("")); SpanData { parent_span_id, span_kind, - name: builder.name, + name, start_time, end_time, attributes, - message_events, + message_events: message_events_queue, links, status_code, status_message, @@ -286,7 +289,7 @@ impl crate::trace::Tracer for Tracer { // Call `on_start` for all processors for processor in provider.span_processors() { - processor.on_start(&span, &parent_cx) + processor.on_start(&span, &parent_context) } span @@ -346,13 +349,13 @@ mod tests { let tracer = tracer_provider.get_tracer("test", None); let trace_state = TraceState::from_key_value(vec![("foo", "bar")]).unwrap(); let span_builder = SpanBuilder { - parent_context: Some(Context::current_with_span(TestSpan(SpanContext::new( + parent_context: Context::new().with_span(TestSpan(SpanContext::new( TraceId::from_u128(128), SpanId::from_u64(64), TRACE_FLAG_SAMPLED, true, trace_state, - )))), + ))), ..Default::default() }; diff --git a/opentelemetry/src/trace/noop.rs b/opentelemetry/src/trace/noop.rs index 19250744c3..560386c7bb 100644 --- a/opentelemetry/src/trace/noop.rs +++ b/opentelemetry/src/trace/noop.rs @@ -6,7 +6,7 @@ use crate::{ sdk::export::trace::{ExportResult, SpanData, SpanExporter}, trace, - trace::{TraceContextExt, TraceState}, + trace::{SpanBuilder, TraceContextExt, TraceState}, Context, KeyValue, }; use async_trait::async_trait; @@ -131,14 +131,12 @@ impl trace::Tracer for NoopTracer { /// Starts a new `NoopSpan` with a given context. /// - /// If the context contains a valid span context, it is propagated. + /// If the context contains a valid span, it's span context is propagated. fn start_with_context(&self, name: T, cx: Context) -> Self::Span where T: Into>, { - let mut builder = self.span_builder(name); - builder.parent_context = Some(cx); - self.build(builder) + self.build(SpanBuilder::from_name_with_context(name, cx)) } /// Starts a `SpanBuilder`. @@ -151,13 +149,16 @@ 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(&self, mut builder: trace::SpanBuilder) -> Self::Span { - match builder.parent_context.take() { - Some(cx) if cx.has_active_span() => trace::NoopSpan { + /// If the span builder or the context's current span contains a valid span context, it is + /// propagated. + fn build(&self, builder: trace::SpanBuilder) -> Self::Span { + let cx = builder.parent_context; + if cx.has_active_span() { + trace::NoopSpan { span_context: cx.span().span_context().clone(), - }, - _ => self.invalid(), + } + } else { + self.invalid() } } } @@ -212,13 +213,13 @@ mod tests { let tracer = NoopTracer::new(); let builder = tracer .span_builder("foo") - .with_parent_context(Context::current_with_span(TestSpan(valid_span_context()))); + .with_parent_context(Context::new().with_span(TestSpan(valid_span_context()))); let span = tracer.build(builder); assert!(span.span_context().is_valid()); } #[test] - fn noop_tracer_propagates_valid_span_context_from_span() { + fn noop_tracer_propagates_valid_span_context_from_explicitly_specified_context() { let tracer = NoopTracer::new(); let cx = Context::new().with_span(NoopSpan { span_context: valid_span_context(), diff --git a/opentelemetry/src/trace/tracer.rs b/opentelemetry/src/trace/tracer.rs index 5f7548d815..a6a6eb3cf8 100644 --- a/opentelemetry/src/trace/tracer.rs +++ b/opentelemetry/src/trace/tracer.rs @@ -337,7 +337,7 @@ pub trait Tracer: fmt::Debug + 'static { #[derive(Clone, Debug, Default)] pub struct SpanBuilder { /// Parent `Context` - pub parent_context: Option, + pub parent_context: Context, /// Trace id, useful for integrations with external tracing systems. pub trace_id: Option, /// Span id, useful for integrations with external tracing systems. @@ -368,8 +368,16 @@ pub struct SpanBuilder { impl SpanBuilder { /// Create a new span builder from a span name pub fn from_name>>(name: T) -> Self { + Self::from_name_with_context(name, Context::current()) + } + + /// Create a new span builder from a span name with the specified context + pub(crate) fn from_name_with_context>>( + name: T, + parent_context: Context, + ) -> Self { SpanBuilder { - parent_context: None, + parent_context, trace_id: None, span_id: None, span_kind: None, @@ -388,7 +396,7 @@ impl SpanBuilder { /// Assign parent context pub fn with_parent_context(self, parent_context: Context) -> Self { SpanBuilder { - parent_context: Some(parent_context), + parent_context, ..self } }