From 5b7fa3cc5db1c9b96f9a4f0af7fabea976a9e320 Mon Sep 17 00:00:00 2001 From: Emmanuel Courreges Date: Thu, 25 Jan 2024 15:46:59 +0100 Subject: [PATCH] [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" --- CHANGELOG.md | 5 ++++ .../trace/propagation/b3_propagator.h | 9 +++++- .../trace/propagation/http_trace_context.h | 9 +++++- .../opentelemetry/trace/propagation/jaeger.h | 9 +++++- .../propagation/composite_propagator_test.cc | 17 +++++++++++ .../trace/propagation/b3_propagation_test.cc | 25 +++++++++++++--- .../propagation/http_text_format_test.cc | 29 +++++++++++++++---- .../propagation/jaeger_propagation_test.cc | 21 +++++++++++++- 8 files changed, 111 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 478e24edb9..23b80ce5b8 100644 --- 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 index e52e42b747..a13d2f0767 100644 --- 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 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 index bba7592ea6..a6b7e3b219 100644 --- 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 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 index 11cd820472..1f0195a248 100644 --- 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 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 callback) const noexcept override diff --git a/api/test/context/propagation/composite_propagator_test.cc b/api/test/context/propagation/composite_propagator_test.cc index 918f8cedd4..5bf18710ed 100644 --- 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>(ctx2_span)); + + span = nostd::get>(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 index 2538b5be29..9791c54378 100644 --- 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(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>(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 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>(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>(ctx2_span); - EXPECT_EQ(span->GetContext().IsRemote(), false); + EXPECT_FALSE(nostd::holds_alternative>(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 index 8fa0e44ed2..e41e0fe65c 100644 --- 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(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(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(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 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>(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(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(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 index c337617383..add38eac6c 100644 --- 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 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>(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(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))}; format.Inject(carrier, ctx); EXPECT_TRUE(carrier.headers_.count("uber-trace-id") == 0);