Skip to content

Commit

Permalink
[API] Propagators: do not overwrite the active span with a default in…
Browse files Browse the repository at this point in the history 
…valid span (#2511)
  • Loading branch information
@ecourreges-orange
ecourreges-orange authored Jan 29, 2024
1 parent 2320636 commit 7cc781e
Show file tree
Hide file tree
Showing 8 changed files with 111 additions and 13 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 8 additions & 1 deletion api/include/opentelemetry/trace/propagation/b3_propagator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 8 additions & 1 deletion api/include/opentelemetry/trace/propagation/http_trace_context.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 8 additions & 1 deletion api/include/opentelemetry/trace/propagation/jaeger.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 17 additions & 0 deletions api/test/context/propagation/composite_propagator_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
25 changes: 21 additions & 4 deletions api/test/trace/propagation/b3_propagation_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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)
Expand All @@ -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)
Expand Down
29 changes: 24 additions & 5 deletions api/test/trace/propagation/http_text_format_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);

Expand All @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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);

Expand All @@ -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);

Expand Down
21 changes: 20 additions & 1 deletion api/test/trace/propagation/jaeger_propagation_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down

2 comments on commit 7cc781e

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'OpenTelemetry-cpp api Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 7cc781e Previous: 2320636 Ratio
BM_SpanIdToLowerBase16 15.975216232735464 ns/iter 6.507392141699439 ns/iter 2.45
BM_SpanIdIsValid 0.6521058266871776 ns/iter 0.31700122004263304 ns/iter 2.06

This comment was automatically generated by workflow using github-action-benchmark.

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'OpenTelemetry-cpp sdk Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 7cc781e Previous: 2320636 Ratio
BM_BaselineBuffer/2 15712635.517120361 ns/iter 7753588.857441923 ns/iter 2.03
BM_LockFreeBuffer/2 5279242.515563965 ns/iter 2269208.6772052916 ns/iter 2.33

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.