Unify trace context injection behaviour #2473
Conversation
| public var sampled: Bool? { | ||
| if let single = httpHeaderFields[B3HTTPHeaders.Single.b3Field] { | ||
| return single != B3HTTPHeaders.Constants.unsampledValue | ||
| return single.components(separatedBy: B3HTTPHeaders.Constants.b3Separator)[safe: 2] != B3HTTPHeaders.Constants.unsampledValue |
There was a problem hiding this comment.
I wonder if we should read when B3 when it is just "0" as well.
I think that was the main source of confusion, because in public documentation:
https://github.com/openzipkin/b3-propagation
Both b3=0 and b3={TraceId}-{SpanId}-{SamplingState}-{ParentSpanId} are valid.
I think that in our case we always want to use the latter, or not inject header at all.
But maybe if sampled = false and TraceContextInjection = sampled we should inject b3=0?
There was a problem hiding this comment.
Backend SDK engineer here 👋🏼
I think we should be flexible on what we extract and accept both b3=0 and b3={TraceId}-{SpanId}-{SamplingState}-{ParentSpanId} styles, but only inject in the b3={TraceId}-{SpanId}-{SamplingState}-{ParentSpanId} style
There was a problem hiding this comment.
Actually, if the input is b3=0, then we don't have a trace-id or span-id at all. For reference in our C# library, we don't accept a value of b3=0 so if a new span is created then we get a brand new tracecontext. I wonder what the other libraries do...let me get back to you, but I think the injection behavior you have encoded is accurate so let's not block on this
| case .single: | ||
| if sampled { | ||
| traceHeaderFields[B3HTTPHeaders.Single.b3Field] = [ | ||
| String(traceContext.traceID, representation: .hexadecimal32Chars), | ||
| String(traceContext.spanID, representation: .hexadecimal16Chars), | ||
| sampled ? Constants.sampledValue : Constants.unsampledValue, | ||
| traceContext.parentSpanID.map { String($0, representation: .hexadecimal16Chars) } | ||
| ] | ||
| .compactMap { $0 } | ||
| .joined(separator: Constants.b3Separator) | ||
| } else { | ||
| traceHeaderFields[B3HTTPHeaders.Single.b3Field] = Constants.unsampledValue | ||
| } |
There was a problem hiding this comment.
Here's the issue. Take a look at the dead code in line 103 - dead code because of if statement before.
| switch (traceContextInjection, traceContext.isKept) { | ||
| case (.all, _), (.sampled, true): |
There was a problem hiding this comment.
This was actually correct - but decided to rework the code to match b3 implementation.
I think it's slightly better to read now.
| } | ||
|
|
||
| switch (traceContextInjection, sampled) { | ||
| case (.all, _), (.sampled, true): |
There was a problem hiding this comment.
Same here. Behaviour was correct according to tests.
I hope it's a bit less cryptic now.
|
|
||
| let headers = writer.traceHeaderFields | ||
| XCTAssertEqual(headers[B3HTTPHeaders.Single.b3Field], "0") | ||
| XCTAssertEqual(headers[B3HTTPHeaders.Single.b3Field], "00000000000004d200000000000004d2-0000000000000929-0-000000000000162e") |
There was a problem hiding this comment.
That's the key difference in the behaviour in this PR.
This comment has been minimized.
This comment has been minimized.
simaoseica-dd
force-pushed
the
maciey/fix-tracing-header-writers
branch
from
e7b26c1 to
2820e15
Compare
maciejburda
force-pushed
the
maciey/fix-tracing-header-writers
branch
from
2820e15 to
81e6bf7
Compare
What and why?
Unifies the behaviour of HTTP Header Writers to always inject headers with sampling decision information if
TracingContextInjectionis configured toall.Review checklist
make api-surface)