Recover the OT context header from the B3 headers if not propagated #1702
Conversation
Resolves envoyproxy#1364 Signed-off-by: Gary Brown <[email protected]>
|
Would be good if someone could independently test this using the zipkin-tracing example - simply remove the
|
There was a problem hiding this comment.
Thank you @objectiser. The approach looks good to me. @rshriram @fabolive @adriancole if you could comment.
| @@ -107,6 +107,20 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa | |||
|
|
|||
| new_zipkin_span = | |||
| tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time, context); | |||
|
|
|||
There was a problem hiding this comment.
nit: del newline here and at 123
| } 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); |
There was a problem hiding this comment.
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).
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. |
|
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 <[email protected]>
| uint64_t parentId(0); | ||
| if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), traceId, 16)) { | ||
| throw EnvoyException(fmt::format("invalid hex string for XB3TraceId '{}'", |
There was a problem hiding this comment.
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 <[email protected]>
| if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), traceId, 16) | ||
| || !StringUtil::atoul(request_headers.XB3SpanId()->value().c_str(), spanId, 16) | ||
| || (request_headers.XB3ParentSpanId() && !StringUtil::atoul(request_headers.XB3ParentSpanId()->value().c_str(), parentId, 16))) { | ||
| return nullptr; |
There was a problem hiding this comment.
i'd probably return nullspan here to be on a safe side.
There was a problem hiding this comment.
Sorry yes I meant NullSpan (not nullptr)
There was a problem hiding this comment.
No problem will sort it tomorrow.
|
@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? |
|
@fabolive I think it would be better to discuss in a separate issue. |
…cted Signed-off-by: Gary Brown <[email protected]>
| @@ -112,19 +112,11 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa | |||
| uint64_t traceId(0); | |||
There was a problem hiding this comment.
Sorry didn't catch this before. Please use snake_case for variables (trace_id, span_id, etc.).
Signed-off-by: Gary Brown <[email protected]>
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 <[email protected]> Signed-off-by: JP Simard <[email protected]>
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 <[email protected]> Signed-off-by: JP Simard <[email protected]>
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?