[API] Fix b3, w3c and jaeger propagators: they will not overwrite the…

… active span with a default invalid span, which is especially useful when used with CompositePropagator (#2504) [TEST] Change wrong string "current-span" in unit tests to proper trace:kSpan which is "active_span"
open-telemetry · Jan 25, 2024 · 5b7fa3c · 5b7fa3c
1 parent bbd6325
commit 5b7fa3c
Show file tree

Hide file tree

Showing 8 changed files with 111 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,11 @@ Increment the:
 
 ## [Unreleased]
 
+* [API] Fix b3, w3c and jaeger propagators: they will not overwrite
+  the active span with a default invalid span, which is especially useful
+  when used with CompositePropagator
+  [TEST] fix string "current-span" to trace:kSpan which is "active_span"
+  [#2511](https://github.com/open-telemetry/opentelemetry-cpp/pull/2511)
 * [REMOVAL] Remove option WITH_OTLP_HTTP_SSL_PREVIEW
   [#2435](https://github.com/open-telemetry/opentelemetry-cpp/pull/2435)
 * [BUILD] Fix removing of NOMINMAX on Windows

diff --git a/api/include/opentelemetry/trace/propagation/b3_propagator.h b/api/include/opentelemetry/trace/propagation/b3_propagator.h
@@ -51,7 +51,14 @@ class B3PropagatorExtractor : public context::propagation::TextMapPropagator
   {
     SpanContext span_context = ExtractImpl(carrier);
     nostd::shared_ptr<Span> sp{new DefaultSpan(span_context)};
-    return trace::SetSpan(context, sp);
+    if (span_context.IsValid())
+    {
+      return trace::SetSpan(context, sp);
+    }
+    else
+    {
+      return context;
+    }
   }
 
   static TraceId TraceIdFromHex(nostd::string_view trace_id)

diff --git a/api/include/opentelemetry/trace/propagation/http_trace_context.h b/api/include/opentelemetry/trace/propagation/http_trace_context.h
@@ -54,7 +54,14 @@ class HttpTraceContext : public context::propagation::TextMapPropagator
   {
     SpanContext span_context = ExtractImpl(carrier);
     nostd::shared_ptr<Span> sp{new DefaultSpan(span_context)};
-    return trace::SetSpan(context, sp);
+    if (span_context.IsValid())
+    {
+      return trace::SetSpan(context, sp);
+    }
+    else
+    {
+      return context;
+    }
   }
 
   static TraceId TraceIdFromHex(nostd::string_view trace_id)

diff --git a/api/include/opentelemetry/trace/propagation/jaeger.h b/api/include/opentelemetry/trace/propagation/jaeger.h
@@ -57,7 +57,14 @@ class OPENTELEMETRY_DEPRECATED JaegerPropagator : public context::propagation::T
   {
     SpanContext span_context = ExtractImpl(carrier);
     nostd::shared_ptr<Span> sp{new DefaultSpan(span_context)};
-    return trace::SetSpan(context, sp);
+    if (span_context.IsValid())
+    {
+      return trace::SetSpan(context, sp);
+    }
+    else
+    {
+      return context;
+    }
   }
 
   bool Fields(nostd::function_ref<bool(nostd::string_view)> callback) const noexcept override

diff --git a/api/test/context/propagation/composite_propagator_test.cc b/api/test/context/propagation/composite_propagator_test.cc
@@ -93,6 +93,23 @@ TEST_F(CompositePropagatorTest, Extract)
   EXPECT_EQ(Hex(span->GetContext().span_id()), "e457b5a2e4d86bd1");
   EXPECT_EQ(span->GetContext().IsSampled(), true);
   EXPECT_EQ(span->GetContext().IsRemote(), true);
+
+  // Now check that last propagator does not win if there is no header for it
+  carrier.headers_ = {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-00"}};
+  ctx1             = context::Context{};
+
+  ctx2 = composite_propagator_->Extract(carrier, ctx1);
+
+  ctx2_span = ctx2.GetValue(trace::kSpanKey);
+  EXPECT_TRUE(nostd::holds_alternative<nostd::shared_ptr<trace::Span>>(ctx2_span));
+
+  span = nostd::get<nostd::shared_ptr<trace::Span>>(ctx2_span);
+
+  // Here the first propagator (W3C) wins
+  EXPECT_EQ(Hex(span->GetContext().trace_id()), "4bf92f3577b34da6a3ce929d0e0e4736");
+  EXPECT_EQ(Hex(span->GetContext().span_id()), "0102030405060708");
+  EXPECT_EQ(span->GetContext().IsSampled(), false);
+  EXPECT_EQ(span->GetContext().IsRemote(), true);
 }
 
 TEST_F(CompositePropagatorTest, Inject)

diff --git a/api/test/trace/propagation/b3_propagation_test.cc b/api/test/trace/propagation/b3_propagation_test.cc
@@ -51,7 +51,7 @@ TEST(B3PropagationTest, PropagateInvalidContext)
   // Do not propagate invalid trace context.
   TextMapCarrierTest carrier;
   context::Context ctx{
-      "current-span",
+      trace::kSpanKey,
       nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
   format.Inject(carrier, ctx);
   EXPECT_TRUE(carrier.headers_.count("b3") == 0);
@@ -64,8 +64,26 @@ TEST(B3PropagationTest, ExtractInvalidContext)
   context::Context ctx1 = context::Context{};
   context::Context ctx2 = format.Extract(carrier, ctx1);
   auto ctx2_span        = ctx2.GetValue(trace::kSpanKey);
+  EXPECT_FALSE(nostd::holds_alternative<nostd::shared_ptr<trace::Span>>(ctx2_span));
+}
+
+TEST(B3PropagationTest, DoNotOverwriteContextWithInvalidSpan)
+{
+  TextMapCarrierTest carrier;
+  constexpr uint8_t buf_span[]  = {1, 2, 3, 4, 5, 6, 7, 8};
+  constexpr uint8_t buf_trace[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
+  trace::SpanContext span_context{trace::TraceId{buf_trace}, trace::SpanId{buf_span},
+                                  trace::TraceFlags{true}, false};
+  nostd::shared_ptr<trace::Span> sp{new trace::DefaultSpan{span_context}};
+
+  // Make sure this invalid span does not overwrite the active span context
+  carrier.headers_ = {{"b3", "00000000000000000000000000000000-0000000000000000-0"}};
+  context::Context ctx1{trace::kSpanKey, sp};
+  context::Context ctx2 = format.Extract(carrier, ctx1);
+  auto ctx2_span        = ctx2.GetValue(trace::kSpanKey);
   auto span             = nostd::get<nostd::shared_ptr<trace::Span>>(ctx2_span);
-  EXPECT_EQ(span->GetContext().IsRemote(), false);
+
+  EXPECT_EQ(Hex(span->GetContext().trace_id()), "0102030405060708090a0b0c0d0e0f10");
 }
 
 TEST(B3PropagationTest, DoNotExtractWithInvalidHex)
@@ -75,8 +93,7 @@ TEST(B3PropagationTest, DoNotExtractWithInvalidHex)
   context::Context ctx1 = context::Context{};
   context::Context ctx2 = format.Extract(carrier, ctx1);
   auto ctx2_span        = ctx2.GetValue(trace::kSpanKey);
-  auto span             = nostd::get<nostd::shared_ptr<trace::Span>>(ctx2_span);
-  EXPECT_EQ(span->GetContext().IsRemote(), false);
+  EXPECT_FALSE(nostd::holds_alternative<nostd::shared_ptr<trace::Span>>(ctx2_span));
 }
 
 TEST(B3PropagationTest, SetRemoteSpan)

diff --git a/api/test/trace/propagation/http_text_format_test.cc b/api/test/trace/propagation/http_text_format_test.cc
@@ -50,7 +50,7 @@ TEST(TextMapPropagatorTest, NoSendEmptyTraceState)
   TextMapCarrierTest carrier;
   carrier.headers_ = {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"}};
   context::Context ctx1 = context::Context{
-      "current-span",
+      trace::kSpanKey,
       nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
   context::Context ctx2 = format.Extract(carrier, ctx1);
   TextMapCarrierTest carrier2;
@@ -65,7 +65,7 @@ TEST(TextMapPropagatorTest, PropogateTraceState)
   carrier.headers_ = {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"},
                       {"tracestate", "congo=t61rcWkgMzE"}};
   context::Context ctx1 = context::Context{
-      "current-span",
+      trace::kSpanKey,
       nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
   context::Context ctx2 = format.Extract(carrier, ctx1);
 
@@ -82,7 +82,7 @@ TEST(TextMapPropagatorTest, PropagateInvalidContext)
   // Do not propagate invalid trace context.
   TextMapCarrierTest carrier;
   context::Context ctx{
-      "current-span",
+      trace::kSpanKey,
       nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
   format.Inject(carrier, ctx);
   EXPECT_TRUE(carrier.headers_.count("traceparent") == 0);
@@ -107,6 +107,25 @@ TEST(TextMapPropagatorTest, SetRemoteSpan)
   EXPECT_EQ(span->GetContext().IsRemote(), true);
 }
 
+TEST(TextMapPropagatorTest, DoNotOverwriteContextWithInvalidSpan)
+{
+  TextMapCarrierTest carrier;
+  constexpr uint8_t buf_span[]  = {1, 2, 3, 4, 5, 6, 7, 8};
+  constexpr uint8_t buf_trace[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
+  trace::SpanContext span_context{trace::TraceId{buf_trace}, trace::SpanId{buf_span},
+                                  trace::TraceFlags{true}, false};
+  nostd::shared_ptr<trace::Span> sp{new trace::DefaultSpan{span_context}};
+
+  // Make sure this invalid span does not overwrite the active span context
+  carrier.headers_ = {{"traceparent", "00-FOO92f3577b34da6a3ce929d0e0e4736-010BAR0405060708-01"}};
+  context::Context ctx1{trace::kSpanKey, sp};
+  context::Context ctx2 = format.Extract(carrier, ctx1);
+  auto ctx2_span        = ctx2.GetValue(trace::kSpanKey);
+  auto span             = nostd::get<nostd::shared_ptr<trace::Span>>(ctx2_span);
+
+  EXPECT_EQ(Hex(span->GetContext().trace_id()), "0102030405060708090a0b0c0d0e0f10");
+}
+
 TEST(TextMapPropagatorTest, GetCurrentSpan)
 {
   TextMapCarrierTest carrier;
@@ -165,7 +184,7 @@ TEST(GlobalTextMapPropagator, NoOpPropagator)
   carrier.headers_ = {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"},
                       {"tracestate", "congo=t61rcWkgMzE"}};
   context::Context ctx1 = context::Context{
-      "current-span",
+      trace::kSpanKey,
       nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
   context::Context ctx2 = propagator->Extract(carrier, ctx1);
 
@@ -189,7 +208,7 @@ TEST(GlobalPropagator, SetAndGet)
   carrier.headers_ = {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"},
                       {"tracestate", trace_state_value}};
   context::Context ctx1 = context::Context{
-      "current-span",
+      trace::kSpanKey,
       nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
   context::Context ctx2 = propagator->Extract(carrier, ctx1);
 

diff --git a/api/test/trace/propagation/jaeger_propagation_test.cc b/api/test/trace/propagation/jaeger_propagation_test.cc
@@ -134,6 +134,25 @@ TEST(JaegerPropagatorTest, ExctractInvalidSpans)
   }
 }
 
+TEST(JaegerPropagatorTest, DoNotOverwriteContextWithInvalidSpan)
+{
+  TextMapCarrierTest carrier;
+  constexpr uint8_t buf_span[]  = {1, 2, 3, 4, 5, 6, 7, 8};
+  constexpr uint8_t buf_trace[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
+  trace::SpanContext span_context{trace::TraceId{buf_trace}, trace::SpanId{buf_span},
+                                  trace::TraceFlags{true}, false};
+  nostd::shared_ptr<trace::Span> sp{new trace::DefaultSpan{span_context}};
+
+  // Make sure this invalid span does not overwrite the active span context
+  carrier.headers_ = {{"uber-trace-id", "foo:bar:0:00"}};
+  context::Context ctx1{trace::kSpanKey, sp};
+  context::Context ctx2 = format.Extract(carrier, ctx1);
+  auto ctx2_span        = ctx2.GetValue(trace::kSpanKey);
+  auto span             = nostd::get<nostd::shared_ptr<trace::Span>>(ctx2_span);
+
+  EXPECT_EQ(Hex(span->GetContext().trace_id()), "0102030405060708090a0b0c0d0e0f10");
+}
+
 TEST(JaegerPropagatorTest, InjectsContext)
 {
   TextMapCarrierTest carrier;
@@ -161,7 +180,7 @@ TEST(JaegerPropagatorTest, DoNotInjectInvalidContext)
 {
   TextMapCarrierTest carrier;
   context::Context ctx{
-      "current-span",
+      trace::kSpanKey,
       nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
   format.Inject(carrier, ctx);
   EXPECT_TRUE(carrier.headers_.count("uber-trace-id") == 0);