Propagate tracestate header in the OpenTelemetry tracer #22693

AlexanderEllis · 2022-08-13T14:49:45Z

Commit Message: Propagate tracestate header in the OpenTelemetry tracer
Additional Description: OTLP Spans include the tracestate from the tracestate header. We can propagate that information from the headers to the Span. For an initial cut, we can just store whatever incoming tracestate header is in the span. In the future, it may make sense to add more handling to update the tracestate, but I don't believe we currently do any tracestate updates in Envoy.
Risk Level: Low
Testing: Basic tests

Part of #9958

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

repokitteh-read-only · 2022-08-13T14:49:48Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #22693 was opened by AlexanderEllis.

see: more, trace.

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

moderation · 2022-08-14T00:09:45Z

test/extensions/tracers/opentelemetry/opentelemetry_tracer_impl_test.cc

+      key: "service.name"
+      value:
+        string_value: "unknown_service:envoy"
+  instrumentation_library_spans:


Should this be resource_spans: ?

I think as written it's OK, since instrumentation_library_spans is the child field of resource_spans, but now that I think about it, this should really be scope_spans with that other PR. This is the classic dueling PRs though, where with name change, this test would only pass once the other is submitted...

Happy to fix once the other PR is in, or if this one is merged first I can update this code in the other PR.

Speaking of that PR :) Updated this with scope_spans to match the new behavior. Thanks!

👍 Not really related to this PR but interested in how to leverage the config at https://github.com/envoyproxy/envoy/blob/main/test/extensions/tracers/opentelemetry/opentelemetry_tracer_impl_test.cc#L337-L357. Not clear from the test code where this is configured.

I definitely think it could use a little refactoring to be clearer (especially the bytes setting). I was currently using it to verify the final span that the Tracer exports, but so far I've only added the exact proto matching with this config for a few of the tests (you'll see sendMessageRaw_(_, _) in others instead of sendMessageRaw_(Grpc::ProtoBufferEqIgnoreRepeatedFieldOrdering(request_proto), _)). For legibility, I think it would also be helpful to only test the specific fields for each test to help reduce the busyness of each expectation.

jmarantz · 2022-08-15T12:35:50Z

/assign-from @envoyproxy/first-pass-reviewers

repokitteh-read-only · 2022-08-15T12:35:54Z

@envoyproxy/first-pass-reviewers assignee is @tonya11en

🐱

Caused by: a #22693 (comment) was created by @jmarantz.

see: more, trace.

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

htuch

Looks good, thanks, just a few minor feedback items.
/wait

htuch · 2022-08-17T02:46:10Z

source/extensions/tracers/opentelemetry/span_context_extractor.cc

+  // See https://www.w3.org/TR/trace-context/#processing-model-for-working-with-trace-context
+  absl::string_view tracestate = "";
+  // TODO: what if tracestate is in multiple headers? Spec says we should combine, but Envoy
+  // currently only surfaces the first seen.


See get, which returns a GetResult with all header values.

Thanks for that pointer! Did a little digging, and it looks like ConnectionManagerImpl::ActiveStream::traceRequest calls startSpan with its RequestHeaderMapPtr request_headers_ (the RequestHeaderMap containing the get). Driver::startSpan is defined with just Tracing::TraceContext though, which only has getByKey and forEach.

With that in mind, I added some handling for this (and a test) with iteration over all the headers. Doesn't seem very efficient to me, but I think this works for the multiple headers case. Happy to improve if you have any suggestions 👍

Maybe fix TraceContext to deal with the appropriate header-specific aggregation behavior. I think any tracer interacting with tracestate will need this behavior. I think in most places that we see header get-by-key and the value is singular and not aggregated, there is some code smell that we haven't fully thought through the valid wire combinations.

htuch · 2022-08-17T02:47:14Z

test/extensions/tracers/opentelemetry/span_context_extractor_test.cc

+TEST(SpanContextExtractorTest, ExtractSpanContextWithTracestate) {
+  Http::TestRequestHeaderMapImpl request_headers{
+      {"traceparent", fmt::format("{}-{}-{}-{}", version, trace_id, parent_id, trace_flags)},
+      {"tracestate", "sample-tracestate"}};


Is there one for missing traceparent?

Ah there isn't, good call! Technically it was already covered via the missing traceparent test above, but I think it's worthwhile to explicitly add one for it for legibility. I also fleshed out the comment in the extractor, mentioning that we are already checking for the existence of the traceparent header.

htuch · 2022-08-17T02:48:02Z

source/extensions/tracers/opentelemetry/tracer.cc

@@ -174,6 +176,9 @@ Tracing::SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::str
  new_span.setId(Hex::uint64ToHex(span_id));
  // Respect the previous span's sampled flag.
  new_span.setSampled(previous_span_context.sampled());
+  if (!previous_span_context.tracestate().empty()) {
+    new_span.setTracestate(std::string{previous_span_context.tracestate()});


Is this tested?

Yup, via ParseSpanContextFromHeadersTest.

In Driver::startSpan in opentelemetry_tracer_impl.cc, if there is a propagation header present, it'll call this fn, Tracer::startSpan with the previous span context. Since we're manually adding the traceparent propagation header (and tracestate) in the the test and then calling driver_->startSpan, the tracestate in the header is passed through in the previous_span_context here.

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

moderation · 2022-08-29T15:58:46Z

This ready to merge @htuch @AlexanderEllis ?

mattklein123

Thanks will merge as it looks like comments have been addressed.

comments addressed

AlexanderEllis added 3 commits July 16, 2022 21:20

Parse and propagate tracestate header in OTLP spans

02ed102

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

Merge branch 'main' into otlp-tracer-tracestate

dd4a15e

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

Fix formatting

44ec001

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

Include service.name in test expectation

9669154

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

AlexanderEllis marked this pull request as ready for review August 13, 2022 18:39

AlexanderEllis requested a review from htuch as a code owner August 13, 2022 18:39

moderation reviewed Aug 14, 2022

View reviewed changes

repokitteh-read-only bot assigned tonya11en Aug 15, 2022

AlexanderEllis added 3 commits August 15, 2022 11:22

Merge branch 'main' into otlp-tracer-tracestate

b87ae40

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

Update instrumentation_library_spans -> scope_spans

6cb6a36

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

Fix formatting

13ae997

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

jmarantz assigned phlax and htuch and unassigned tonya11en and phlax Aug 16, 2022

htuch previously requested changes Aug 17, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 17, 2022

AlexanderEllis added 2 commits August 17, 2022 21:30

Add explicit test for tracestate without traceparent and better comment

fc730be

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

Handle multiple tracestate headers

75f265d

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

repokitteh-read-only bot removed the waiting label Aug 18, 2022

AlexanderEllis requested a review from htuch August 18, 2022 02:58

Merge branch 'main' into otlp-tracer-tracestate

9200289

Signed-off-by: Alex Ellis <ellisonjtk@gmail.com>

mattklein123 approved these changes Aug 29, 2022

View reviewed changes

mattklein123 merged commit 11c4d3a into envoyproxy:main Aug 29, 2022

AlexanderEllis mentioned this pull request Sep 29, 2022

tracing: Transition to OpenTelemetry from OpenTracing and OpenCensus #9958

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate tracestate header in the OpenTelemetry tracer #22693

Propagate tracestate header in the OpenTelemetry tracer #22693

AlexanderEllis commented Aug 13, 2022

repokitteh-read-only bot commented Aug 13, 2022

moderation Aug 14, 2022

AlexanderEllis Aug 14, 2022

AlexanderEllis Aug 15, 2022

moderation Aug 15, 2022

AlexanderEllis Aug 15, 2022

jmarantz commented Aug 15, 2022

repokitteh-read-only bot commented Aug 15, 2022

htuch left a comment

htuch Aug 17, 2022

AlexanderEllis Aug 18, 2022

htuch Aug 18, 2022

htuch Aug 17, 2022

AlexanderEllis Aug 18, 2022

htuch Aug 17, 2022

AlexanderEllis Aug 18, 2022

moderation commented Aug 29, 2022

mattklein123 left a comment

Propagate tracestate header in the OpenTelemetry tracer #22693

Propagate tracestate header in the OpenTelemetry tracer #22693

Conversation

AlexanderEllis commented Aug 13, 2022

repokitteh-read-only bot commented Aug 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Aug 15, 2022

repokitteh-read-only bot commented Aug 15, 2022

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moderation commented Aug 29, 2022

mattklein123 left a comment

Choose a reason for hiding this comment