Skip to content

Commit 7cc781e

Browse files
ecourreges-orangeecourreges-orange
authored
[API] Propagators: do not overwrite the active span with a default invalid span (open-telemetry#2511)
1 parent 2320636 commit 7cc781e

File tree

8 files changed

+111
-13
lines changed

8 files changed

+111
-13
lines changed

‎CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@ Increment the:
1515

1616
## [Unreleased]
1717

18+
* [API] Fix b3, w3c and jaeger propagators: they will not overwrite
19+
the active span with a default invalid span, which is especially useful
20+
when used with CompositePropagator
21+
[TEST] fix string "current-span" to trace:kSpan which is "active_span"
22+
[#2511](https://github.com/open-telemetry/opentelemetry-cpp/pull/2511)
1823
* [REMOVAL] Remove option WITH_OTLP_HTTP_SSL_PREVIEW
1924
[#2435](https://github.com/open-telemetry/opentelemetry-cpp/pull/2435)
2025
* [BUILD] Fix removing of NOMINMAX on Windows

‎api/include/opentelemetry/trace/propagation/b3_propagator.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,14 @@ class B3PropagatorExtractor : public context::propagation::TextMapPropagator
5151
{
5252
SpanContext span_context = ExtractImpl(carrier);
5353
nostd::shared_ptr<Span> sp{new DefaultSpan(span_context)};
54-
return trace::SetSpan(context, sp);
54+
if (span_context.IsValid())
55+
{
56+
return trace::SetSpan(context, sp);
57+
}
58+
else
59+
{
60+
return context;
61+
}
5562
}
5663

5764
static TraceId TraceIdFromHex(nostd::string_view trace_id)

‎api/include/opentelemetry/trace/propagation/http_trace_context.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,14 @@ class HttpTraceContext : public context::propagation::TextMapPropagator
5454
{
5555
SpanContext span_context = ExtractImpl(carrier);
5656
nostd::shared_ptr<Span> sp{new DefaultSpan(span_context)};
57-
return trace::SetSpan(context, sp);
57+
if (span_context.IsValid())
58+
{
59+
return trace::SetSpan(context, sp);
60+
}
61+
else
62+
{
63+
return context;
64+
}
5865
}
5966

6067
static TraceId TraceIdFromHex(nostd::string_view trace_id)

‎api/include/opentelemetry/trace/propagation/jaeger.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,14 @@ class OPENTELEMETRY_DEPRECATED JaegerPropagator : public context::propagation::T
5757
{
5858
SpanContext span_context = ExtractImpl(carrier);
5959
nostd::shared_ptr<Span> sp{new DefaultSpan(span_context)};
60-
return trace::SetSpan(context, sp);
60+
if (span_context.IsValid())
61+
{
62+
return trace::SetSpan(context, sp);
63+
}
64+
else
65+
{
66+
return context;
67+
}
6168
}
6269

6370
bool Fields(nostd::function_ref<bool(nostd::string_view)> callback) const noexcept override

‎api/test/context/propagation/composite_propagator_test.cc‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,23 @@ TEST_F(CompositePropagatorTest, Extract)
9393
EXPECT_EQ(Hex(span->GetContext().span_id()), "e457b5a2e4d86bd1");
9494
EXPECT_EQ(span->GetContext().IsSampled(), true);
9595
EXPECT_EQ(span->GetContext().IsRemote(), true);
96+
97+
// Now check that last propagator does not win if there is no header for it
98+
carrier.headers_ = {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-00"}};
99+
ctx1 = context::Context{};
100+
101+
ctx2 = composite_propagator_->Extract(carrier, ctx1);
102+
103+
ctx2_span = ctx2.GetValue(trace::kSpanKey);
104+
EXPECT_TRUE(nostd::holds_alternative<nostd::shared_ptr<trace::Span>>(ctx2_span));
105+
106+
span = nostd::get<nostd::shared_ptr<trace::Span>>(ctx2_span);
107+
108+
// Here the first propagator (W3C) wins
109+
EXPECT_EQ(Hex(span->GetContext().trace_id()), "4bf92f3577b34da6a3ce929d0e0e4736");
110+
EXPECT_EQ(Hex(span->GetContext().span_id()), "0102030405060708");
111+
EXPECT_EQ(span->GetContext().IsSampled(), false);
112+
EXPECT_EQ(span->GetContext().IsRemote(), true);
96113
}
97114

98115
TEST_F(CompositePropagatorTest, Inject)

‎api/test/trace/propagation/b3_propagation_test.cc‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ TEST(B3PropagationTest, PropagateInvalidContext)
5151
// Do not propagate invalid trace context.
5252
TextMapCarrierTest carrier;
5353
context::Context ctx{
54-
"current-span",
54+
trace::kSpanKey,
5555
nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
5656
format.Inject(carrier, ctx);
5757
EXPECT_TRUE(carrier.headers_.count("b3") == 0);
@@ -64,8 +64,26 @@ TEST(B3PropagationTest, ExtractInvalidContext)
6464
context::Context ctx1 = context::Context{};
6565
context::Context ctx2 = format.Extract(carrier, ctx1);
6666
auto ctx2_span = ctx2.GetValue(trace::kSpanKey);
67+
EXPECT_FALSE(nostd::holds_alternative<nostd::shared_ptr<trace::Span>>(ctx2_span));
68+
}
69+
70+
TEST(B3PropagationTest, DoNotOverwriteContextWithInvalidSpan)
71+
{
72+
TextMapCarrierTest carrier;
73+
constexpr uint8_t buf_span[] = {1, 2, 3, 4, 5, 6, 7, 8};
74+
constexpr uint8_t buf_trace[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
75+
trace::SpanContext span_context{trace::TraceId{buf_trace}, trace::SpanId{buf_span},
76+
trace::TraceFlags{true}, false};
77+
nostd::shared_ptr<trace::Span> sp{new trace::DefaultSpan{span_context}};
78+
79+
// Make sure this invalid span does not overwrite the active span context
80+
carrier.headers_ = {{"b3", "00000000000000000000000000000000-0000000000000000-0"}};
81+
context::Context ctx1{trace::kSpanKey, sp};
82+
context::Context ctx2 = format.Extract(carrier, ctx1);
83+
auto ctx2_span = ctx2.GetValue(trace::kSpanKey);
6784
auto span = nostd::get<nostd::shared_ptr<trace::Span>>(ctx2_span);
68-
EXPECT_EQ(span->GetContext().IsRemote(), false);
85+
86+
EXPECT_EQ(Hex(span->GetContext().trace_id()), "0102030405060708090a0b0c0d0e0f10");
6987
}
7088

7189
TEST(B3PropagationTest, DoNotExtractWithInvalidHex)
@@ -75,8 +93,7 @@ TEST(B3PropagationTest, DoNotExtractWithInvalidHex)
7593
context::Context ctx1 = context::Context{};
7694
context::Context ctx2 = format.Extract(carrier, ctx1);
7795
auto ctx2_span = ctx2.GetValue(trace::kSpanKey);
78-
auto span = nostd::get<nostd::shared_ptr<trace::Span>>(ctx2_span);
79-
EXPECT_EQ(span->GetContext().IsRemote(), false);
96+
EXPECT_FALSE(nostd::holds_alternative<nostd::shared_ptr<trace::Span>>(ctx2_span));
8097
}
8198

8299
TEST(B3PropagationTest, SetRemoteSpan)

‎api/test/trace/propagation/http_text_format_test.cc‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ TEST(TextMapPropagatorTest, NoSendEmptyTraceState)
5050
TextMapCarrierTest carrier;
5151
carrier.headers_ = {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"}};
5252
context::Context ctx1 = context::Context{
53-
"current-span",
53+
trace::kSpanKey,
5454
nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
5555
context::Context ctx2 = format.Extract(carrier, ctx1);
5656
TextMapCarrierTest carrier2;
@@ -65,7 +65,7 @@ TEST(TextMapPropagatorTest, PropogateTraceState)
6565
carrier.headers_ = {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"},
6666
{"tracestate", "congo=t61rcWkgMzE"}};
6767
context::Context ctx1 = context::Context{
68-
"current-span",
68+
trace::kSpanKey,
6969
nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
7070
context::Context ctx2 = format.Extract(carrier, ctx1);
7171

@@ -82,7 +82,7 @@ TEST(TextMapPropagatorTest, PropagateInvalidContext)
8282
// Do not propagate invalid trace context.
8383
TextMapCarrierTest carrier;
8484
context::Context ctx{
85-
"current-span",
85+
trace::kSpanKey,
8686
nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
8787
format.Inject(carrier, ctx);
8888
EXPECT_TRUE(carrier.headers_.count("traceparent") == 0);
@@ -107,6 +107,25 @@ TEST(TextMapPropagatorTest, SetRemoteSpan)
107107
EXPECT_EQ(span->GetContext().IsRemote(), true);
108108
}
109109

110+
TEST(TextMapPropagatorTest, DoNotOverwriteContextWithInvalidSpan)
111+
{
112+
TextMapCarrierTest carrier;
113+
constexpr uint8_t buf_span[] = {1, 2, 3, 4, 5, 6, 7, 8};
114+
constexpr uint8_t buf_trace[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
115+
trace::SpanContext span_context{trace::TraceId{buf_trace}, trace::SpanId{buf_span},
116+
trace::TraceFlags{true}, false};
117+
nostd::shared_ptr<trace::Span> sp{new trace::DefaultSpan{span_context}};
118+
119+
// Make sure this invalid span does not overwrite the active span context
120+
carrier.headers_ = {{"traceparent", "00-FOO92f3577b34da6a3ce929d0e0e4736-010BAR0405060708-01"}};
121+
context::Context ctx1{trace::kSpanKey, sp};
122+
context::Context ctx2 = format.Extract(carrier, ctx1);
123+
auto ctx2_span = ctx2.GetValue(trace::kSpanKey);
124+
auto span = nostd::get<nostd::shared_ptr<trace::Span>>(ctx2_span);
125+
126+
EXPECT_EQ(Hex(span->GetContext().trace_id()), "0102030405060708090a0b0c0d0e0f10");
127+
}
128+
110129
TEST(TextMapPropagatorTest, GetCurrentSpan)
111130
{
112131
TextMapCarrierTest carrier;
@@ -165,7 +184,7 @@ TEST(GlobalTextMapPropagator, NoOpPropagator)
165184
carrier.headers_ = {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"},
166185
{"tracestate", "congo=t61rcWkgMzE"}};
167186
context::Context ctx1 = context::Context{
168-
"current-span",
187+
trace::kSpanKey,
169188
nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
170189
context::Context ctx2 = propagator->Extract(carrier, ctx1);
171190

@@ -189,7 +208,7 @@ TEST(GlobalPropagator, SetAndGet)
189208
carrier.headers_ = {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"},
190209
{"tracestate", trace_state_value}};
191210
context::Context ctx1 = context::Context{
192-
"current-span",
211+
trace::kSpanKey,
193212
nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
194213
context::Context ctx2 = propagator->Extract(carrier, ctx1);
195214

‎api/test/trace/propagation/jaeger_propagation_test.cc‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,25 @@ TEST(JaegerPropagatorTest, ExctractInvalidSpans)
134134
}
135135
}
136136

137+
TEST(JaegerPropagatorTest, DoNotOverwriteContextWithInvalidSpan)
138+
{
139+
TextMapCarrierTest carrier;
140+
constexpr uint8_t buf_span[] = {1, 2, 3, 4, 5, 6, 7, 8};
141+
constexpr uint8_t buf_trace[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
142+
trace::SpanContext span_context{trace::TraceId{buf_trace}, trace::SpanId{buf_span},
143+
trace::TraceFlags{true}, false};
144+
nostd::shared_ptr<trace::Span> sp{new trace::DefaultSpan{span_context}};
145+
146+
// Make sure this invalid span does not overwrite the active span context
147+
carrier.headers_ = {{"uber-trace-id", "foo:bar:0:00"}};
148+
context::Context ctx1{trace::kSpanKey, sp};
149+
context::Context ctx2 = format.Extract(carrier, ctx1);
150+
auto ctx2_span = ctx2.GetValue(trace::kSpanKey);
151+
auto span = nostd::get<nostd::shared_ptr<trace::Span>>(ctx2_span);
152+
153+
EXPECT_EQ(Hex(span->GetContext().trace_id()), "0102030405060708090a0b0c0d0e0f10");
154+
}
155+
137156
TEST(JaegerPropagatorTest, InjectsContext)
138157
{
139158
TextMapCarrierTest carrier;
@@ -161,7 +180,7 @@ TEST(JaegerPropagatorTest, DoNotInjectInvalidContext)
161180
{
162181
TextMapCarrierTest carrier;
163182
context::Context ctx{
164-
"current-span",
183+
trace::kSpanKey,
165184
nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))};
166185
format.Inject(carrier, ctx);
167186
EXPECT_TRUE(carrier.headers_.count("uber-trace-id") == 0);

0 commit comments

Comments
 (0)