Recover the OT context header from the B3 headers if not propagated by objectiser · Pull Request #1702 · envoyproxy/envoy

objectiser · 2017-09-20T09:08:12Z

Resolves #1364

This fixes the issue by detecting that the OtSpanContext header has not been propagated, but the B3 headers have - and therefore will use those values instead.

This means that when an application is only concerned with propagating the span context, they only need to propagate the OtSpanContext header (not the B3 headers) - which is also supported prior to this PR. However if they wish to use a B3 compatible tracer to augment the tracing information within their application, then this tracer would only need to propagate the B3 headers to any outbound (Egress) connections.

So this will simplify the manual propagation case - only one header to propagate, while still allowing more complex tracing requirements to be addressed.

If this change is acceptable, then we may want to consider removing the B3 header propagation from the docs and examples, and instead have a section discussing the case where the application can enhance the default tracing information by using a suitable tracer directly?

Resolves envoyproxy#1364 Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2017-09-20T10:34:43Z

Would be good if someone could independently test this using the zipkin-tracing example - simply remove the 'X-Ot-Span-Context', line from the TRACE_HEADERS_TO_PROPAGATE list in https://github.com/envoyproxy/envoy/blob/master/examples/front-proxy/service.py and you should still see the same trace result in the zipkin UI with two spans, one associated with service1 and the child span associated with service2 .

mattklein123

Thank you @objectiser. The approach looks good to me. @rshriram @fabolive @adriancole if you could comment.

mattklein123 · 2017-09-20T17:10:14Z

source/common/tracing/zipkin/zipkin_tracer_impl.cc


    new_zipkin_span =
        tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time, context);
+


nit: del newline here and at 123

mattklein123 · 2017-09-20T17:11:20Z

source/common/tracing/zipkin/zipkin_tracer_impl.cc

+  } else if (request_headers.XB3TraceId() && request_headers.XB3SpanId()) {
+    uint64_t parentId(0);
+    if (request_headers.XB3ParentSpanId()) {
+      parentId = std::stoull(request_headers.XB3ParentSpanId()->value().c_str(), nullptr, 16);


Please use https://github.com/envoyproxy/envoy/blob/master/source/common/common/utility.h#L92 for string -> int conversions and handle error (also please add tests for that).

mattklein123 · 2017-09-20T17:13:03Z

If this change is acceptable, then we may want to consider removing the B3 header propagation from the docs and examples, and instead have a section discussing the case where the application can enhance the default tracing information by using a suitable tracer directly?

Agreed. In general, I think we should have better tracing docs. Would love any updates you want to do here: https://envoyproxy.github.io/envoy/intro/arch_overview/tracing.html and elsewhere.

codefromthecrypt · 2017-09-20T17:31:46Z

Not sure if I grok the relationship between fixing B3 then removing docs for it. It is a widely used format.

On the main topic, the change appears sound!

Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123 · 2017-09-20T19:37:41Z

source/common/tracing/zipkin/zipkin_tracer_impl.cc

+    uint64_t spanId(0);
    uint64_t parentId(0);
+    if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), traceId, 16)) {
+      throw EnvoyException(fmt::format("invalid hex string for XB3TraceId '{}'",


IIRC you can't safely throw here. It will crash the server. I think you probably want to return a null span or something like that.

Signed-off-by: Gary Brown <gary@brownuk.com>

RomanDzhabarov · 2017-09-20T20:36:07Z

source/common/tracing/zipkin/zipkin_tracer_impl.cc

+    uint64_t spanId(0);
+    uint64_t parentId(0);
+    if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), traceId, 16)
+        || !StringUtil::atoul(request_headers.XB3SpanId()->value().c_str(), spanId, 16)


i'd probably return nullspan here to be on a safe side.

Sorry yes I meant NullSpan (not nullptr)

No problem will sort it tomorrow.

RomanDzhabarov

LGTM, except that small comment.

fabolive · 2017-09-21T13:21:17Z

@objectiser At a high level, the approach makes sense. I agree with @adriancole that removing B3 from the documentation does not sound right. In fact, now that we have a clear way to distinguish ingress from egress we got to a point where the entire implementation of Zipkin support in Envoy could be reworked so that only B3 headers are used. Could this be part of the current PR, or do we want a separate PR for that?

objectiser · 2017-09-21T14:37:37Z

@fabolive I think it would be better to discuss in a separate issue.

…cted Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123

Looks good, small nit. Thanks.

mattklein123 · 2017-09-21T17:50:28Z

source/common/tracing/zipkin/zipkin_tracer_impl.cc

@@ -112,19 +112,11 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa
    uint64_t traceId(0);


Sorry didn't catch this before. Please use snake_case for variables (trace_id, span_id, etc.).

Signed-off-by: Gary Brown <gary@brownuk.com>

Description: Pickup up the following changes from Envoy repo a82962c...bbef2aa Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This reorganizes bench_test.go by removing duplicate code Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

Recover the OT context header from the B3 headers if not propagated

e6b82ff

Resolves envoyproxy#1364 Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser force-pushed the zipkin branch from 8e513f7 to e6b82ff Compare September 20, 2017 09:29

mattklein123 reviewed Sep 20, 2017

View reviewed changes

Change to use StringUtil and check for errors (with tests)

eb237f0

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser force-pushed the zipkin branch from 9c26053 to eb237f0 Compare September 20, 2017 18:56

mattklein123 reviewed Sep 20, 2017

View reviewed changes

Change from throwing exception to returning nullptr

540feff

Signed-off-by: Gary Brown <gary@brownuk.com>

RomanDzhabarov reviewed Sep 20, 2017

View reviewed changes

Change to use NullSpan instead of nullptr when invalid B3 header dete…

3e5c029

…cted Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser mentioned this pull request Sep 21, 2017

RFC: Update the tracing docs to better explain options available to service developers #1711

Closed

mattklein123 reviewed Sep 21, 2017

View reviewed changes

Change to snake_case for variable names

c1a859d

Signed-off-by: Gary Brown <gary@brownuk.com>

RomanDzhabarov approved these changes Sep 21, 2017

View reviewed changes

mattklein123 approved these changes Sep 21, 2017

View reviewed changes

mattklein123 merged commit 4574487 into envoyproxy:master Sep 21, 2017

mathetake added a commit that referenced this pull request Mar 3, 2026

test: reorganizes and runs the micro bench cases (#1702)

1737691

**Description** This reorganizes bench_test.go by removing duplicate code Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>


		new_zipkin_span =
		tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time, context);

		@@ -112,19 +112,11 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa
		uint64_t traceId(0);

Conversation

objectiser commented Sep 20, 2017

Uh oh!

objectiser commented Sep 20, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Sep 20, 2017

Uh oh!

codefromthecrypt commented Sep 20, 2017

Uh oh!

mattklein123 Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

RomanDzhabarov Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

objectiser Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

RomanDzhabarov left a comment

Choose a reason for hiding this comment

Uh oh!

fabolive commented Sep 21, 2017

Uh oh!

objectiser commented Sep 21, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants