-
Notifications
You must be signed in to change notification settings - Fork 160
Unify trace context injection behaviour #2473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,39 +78,38 @@ public class B3HTTPHeadersWriter: TracePropagationHeadersWriter { | |
| /// - Parameter spanID: The span ID. | ||
| /// - Parameter parentSpanID: The parent span ID, if applicable. | ||
| public func write(traceContext: TraceContext) { | ||
| let sampled = traceContext.isKept | ||
|
|
||
| typealias Constants = B3HTTPHeaders.Constants | ||
|
|
||
| switch (traceContextInjection, sampled) { | ||
| case (.all, _), (.sampled, true): | ||
| switch injectEncoding { | ||
| case .multiple: | ||
| traceHeaderFields = [ | ||
| B3HTTPHeaders.Multiple.sampledField: sampled ? Constants.sampledValue : Constants.unsampledValue | ||
| ] | ||
| let sampled = traceContext.isKept | ||
| let shouldInject: Bool = { | ||
| switch traceContextInjection { | ||
| case .all: return true | ||
| case .sampled: return sampled | ||
| } | ||
| }() | ||
| guard shouldInject else { | ||
| return | ||
| } | ||
|
|
||
| if sampled { | ||
| traceHeaderFields[B3HTTPHeaders.Multiple.traceIDField] = String(traceContext.traceID, representation: .hexadecimal32Chars) | ||
| traceHeaderFields[B3HTTPHeaders.Multiple.spanIDField] = String(traceContext.spanID, representation: .hexadecimal16Chars) | ||
| traceHeaderFields[B3HTTPHeaders.Multiple.parentSpanIDField] = traceContext.parentSpanID.map { String($0, representation: .hexadecimal16Chars) } | ||
| } | ||
| 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 | ||
| } | ||
|
Comment on lines
-98
to
-110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's the issue. Take a look at the dead code in line |
||
| switch injectEncoding { | ||
| case .multiple: | ||
| traceHeaderFields = [ | ||
| B3HTTPHeaders.Multiple.sampledField: sampled ? Constants.sampledValue : Constants.unsampledValue, | ||
| B3HTTPHeaders.Multiple.traceIDField: String(traceContext.traceID, representation: .hexadecimal32Chars), | ||
| B3HTTPHeaders.Multiple.spanIDField: String(traceContext.spanID, representation: .hexadecimal16Chars), | ||
| ] | ||
| if let parentSpanId = traceContext.parentSpanID.map({ String($0, representation: .hexadecimal16Chars) }) { | ||
| traceHeaderFields[B3HTTPHeaders.Multiple.parentSpanIDField] = parentSpanId | ||
| } | ||
| case (.sampled, false): | ||
| break | ||
| case .single: | ||
| 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) | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,29 +49,37 @@ public class HTTPHeadersWriter: TracePropagationHeadersWriter { | |
| /// - Parameter spanID: The span ID. | ||
| /// - Parameter parentSpanID: The parent span ID, if applicable. | ||
| public func write(traceContext: TraceContext) { | ||
| switch (traceContextInjection, traceContext.isKept) { | ||
| case (.all, _), (.sampled, true): | ||
|
Comment on lines
-52
to
-53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was actually correct - but decided to rework the code to match b3 implementation. I think it's slightly better to read now. |
||
| traceHeaderFields = [ | ||
| TracingHTTPHeaders.samplingPriorityField: traceContext.isKept ? "1" : "0" | ||
| ] | ||
| traceHeaderFields[TracingHTTPHeaders.traceIDField] = String(traceContext.traceID.idLo) | ||
| traceHeaderFields[TracingHTTPHeaders.parentSpanIDField] = String(traceContext.spanID, representation: .decimal) | ||
| traceHeaderFields[TracingHTTPHeaders.tagsField] = "_dd.p.tid=\(traceContext.traceID.idHiHex)" | ||
| var baggageItems: [String] = [] | ||
| if let sessionId = traceContext.rumSessionId { | ||
| baggageItems.append("\(W3CHTTPHeaders.Constants.rumSessionBaggageKey)=\(sessionId)") | ||
| } | ||
| if let userId = traceContext.userId { | ||
| baggageItems.append("\(W3CHTTPHeaders.Constants.userBaggageKey)=\(userId)") | ||
| } | ||
| if let accountId = traceContext.accountId { | ||
| baggageItems.append("\(W3CHTTPHeaders.Constants.accountBaggageKey)=\(accountId)") | ||
| } | ||
| if baggageItems.isEmpty == false { | ||
| traceHeaderFields[W3CHTTPHeaders.baggage] = baggageItems.joined(separator: ",") | ||
| typealias Constants = W3CHTTPHeaders.Constants | ||
|
|
||
| let sampled = traceContext.isKept | ||
| let shouldInject: Bool = { | ||
| switch traceContextInjection { | ||
| case .all: return true | ||
| case .sampled: return sampled | ||
| } | ||
| case (.sampled, false): | ||
| break | ||
| }() | ||
| guard shouldInject else { | ||
| return | ||
| } | ||
|
|
||
| traceHeaderFields = [ | ||
| TracingHTTPHeaders.samplingPriorityField: traceContext.isKept ? "1" : "0" | ||
| ] | ||
| traceHeaderFields[TracingHTTPHeaders.traceIDField] = String(traceContext.traceID.idLo) | ||
| traceHeaderFields[TracingHTTPHeaders.parentSpanIDField] = String(traceContext.spanID, representation: .decimal) | ||
| traceHeaderFields[TracingHTTPHeaders.tagsField] = "_dd.p.tid=\(traceContext.traceID.idHiHex)" | ||
| var baggageItems: [String] = [] | ||
| if let sessionId = traceContext.rumSessionId { | ||
| baggageItems.append("\(W3CHTTPHeaders.Constants.rumSessionBaggageKey)=\(sessionId)") | ||
| } | ||
| if let userId = traceContext.userId { | ||
| baggageItems.append("\(W3CHTTPHeaders.Constants.userBaggageKey)=\(userId)") | ||
| } | ||
| if let accountId = traceContext.accountId { | ||
| baggageItems.append("\(W3CHTTPHeaders.Constants.accountBaggageKey)=\(accountId)") | ||
| } | ||
| if baggageItems.isEmpty == false { | ||
| traceHeaderFields[W3CHTTPHeaders.baggage] = baggageItems.joined(separator: ",") | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,48 +61,52 @@ public class W3CHTTPHeadersWriter: TracePropagationHeadersWriter { | |
| typealias Constants = W3CHTTPHeaders.Constants | ||
|
|
||
| let sampled = traceContext.isKept | ||
| let shouldInject: Bool = { | ||
| switch traceContextInjection { | ||
| case .all: return true | ||
| case .sampled: return sampled | ||
| } | ||
| }() | ||
| guard shouldInject else { | ||
| return | ||
| } | ||
|
|
||
| switch (traceContextInjection, sampled) { | ||
| case (.all, _), (.sampled, true): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Behaviour was correct according to tests. I hope it's a bit less cryptic now. |
||
| traceHeaderFields[W3CHTTPHeaders.traceparent] = [ | ||
| Constants.version, | ||
| String(traceContext.traceID, representation: .hexadecimal32Chars), | ||
| String(traceContext.spanID, representation: .hexadecimal16Chars), | ||
| sampled ? Constants.sampledValue : Constants.unsampledValue | ||
| ] | ||
| .joined(separator: Constants.separator) | ||
| traceHeaderFields[W3CHTTPHeaders.traceparent] = [ | ||
| Constants.version, | ||
| String(traceContext.traceID, representation: .hexadecimal32Chars), | ||
| String(traceContext.spanID, representation: .hexadecimal16Chars), | ||
| sampled ? Constants.sampledValue : Constants.unsampledValue | ||
| ] | ||
| .joined(separator: Constants.separator) | ||
|
|
||
| // while merging, the tracestate values from the tracestate property take precedence | ||
| // over the ones from the trace context | ||
| let tracestate: [String: String] = [ | ||
| Constants.sampling: "\(sampled ? 1 : 0)", | ||
| Constants.parentId: String(traceContext.spanID, representation: .hexadecimal16Chars) | ||
| ].merging(tracestate) { old, new in | ||
| return new | ||
| } | ||
| // while merging, the tracestate values from the tracestate property take precedence | ||
| // over the ones from the trace context | ||
| let tracestate: [String: String] = [ | ||
| Constants.sampling: "\(sampled ? 1 : 0)", | ||
| Constants.parentId: String(traceContext.spanID, representation: .hexadecimal16Chars) | ||
| ].merging(tracestate) { old, new in | ||
| return new | ||
| } | ||
|
|
||
| let ddtracestate = tracestate | ||
| .map { "\($0.key)\(Constants.tracestateKeyValueSeparator)\($0.value)" } | ||
| .sorted() | ||
| .joined(separator: Constants.tracestatePairSeparator) | ||
| let ddtracestate = tracestate | ||
| .map { "\($0.key)\(Constants.tracestateKeyValueSeparator)\($0.value)" } | ||
| .sorted() | ||
| .joined(separator: Constants.tracestatePairSeparator) | ||
|
|
||
| traceHeaderFields[W3CHTTPHeaders.tracestate] = "\(Constants.dd)=\(ddtracestate)" | ||
| traceHeaderFields[W3CHTTPHeaders.tracestate] = "\(Constants.dd)=\(ddtracestate)" | ||
|
|
||
| var baggageItems: [String] = [] | ||
| if let sessionId = traceContext.rumSessionId { | ||
| baggageItems.append("\(Constants.rumSessionBaggageKey)=\(sessionId)") | ||
| } | ||
| if let userId = traceContext.userId { | ||
| baggageItems.append("\(Constants.userBaggageKey)=\(userId)") | ||
| } | ||
| if let accountId = traceContext.accountId { | ||
| baggageItems.append("\(Constants.accountBaggageKey)=\(accountId)") | ||
| } | ||
| if !baggageItems.isEmpty { | ||
| traceHeaderFields[W3CHTTPHeaders.baggage] = baggageItems.joined(separator: ",") | ||
| } | ||
| case (.sampled, false): | ||
| break | ||
| var baggageItems: [String] = [] | ||
| if let sessionId = traceContext.rumSessionId { | ||
| baggageItems.append("\(Constants.rumSessionBaggageKey)=\(sessionId)") | ||
| } | ||
| if let userId = traceContext.userId { | ||
| baggageItems.append("\(Constants.userBaggageKey)=\(userId)") | ||
| } | ||
| if let accountId = traceContext.accountId { | ||
| baggageItems.append("\(Constants.accountBaggageKey)=\(accountId)") | ||
| } | ||
| if !baggageItems.isEmpty { | ||
| traceHeaderFields[W3CHTTPHeaders.baggage] = baggageItems.joined(separator: ",") | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,13 +44,13 @@ class B3HTTPHeadersWriterTests: XCTestCase { | |
| ) | ||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the key difference in the behaviour in this PR. |
||
| } | ||
|
|
||
| func testWritingSampledTraceContext_withSingleEncoding_andCustomSamplingStrategy() { | ||
| let writer = B3HTTPHeadersWriter( | ||
| injectEncoding: .single, | ||
| traceContextInjection: .all | ||
| traceContextInjection: .sampled | ||
| ) | ||
|
|
||
| writer.write( | ||
|
|
@@ -69,7 +69,7 @@ class B3HTTPHeadersWriterTests: XCTestCase { | |
| func testWritingDroppedTraceContext_withSingleEncoding_andCustomSamplingStrategy() { | ||
| let writer = B3HTTPHeadersWriter( | ||
| injectEncoding: .single, | ||
| traceContextInjection: .all | ||
| traceContextInjection: .sampled | ||
| ) | ||
|
|
||
| writer.write( | ||
|
|
@@ -82,7 +82,7 @@ class B3HTTPHeadersWriterTests: XCTestCase { | |
| ) | ||
|
|
||
| let headers = writer.traceHeaderFields | ||
| XCTAssertEqual(headers[B3HTTPHeaders.Single.b3Field], "0") | ||
| XCTAssertNil(headers[B3HTTPHeaders.Single.b3Field]) | ||
| } | ||
|
|
||
| func testItWritesSingleHeaderWithoutOptionalValues() { | ||
|
|
@@ -141,10 +141,10 @@ class B3HTTPHeadersWriterTests: XCTestCase { | |
| ) | ||
|
|
||
| let headers = writer.traceHeaderFields | ||
| XCTAssertNil(headers[B3HTTPHeaders.Multiple.traceIDField]) | ||
| XCTAssertNil(headers[B3HTTPHeaders.Multiple.spanIDField]) | ||
| XCTAssertEqual(headers[B3HTTPHeaders.Multiple.traceIDField], "00000000000004d200000000000004d2") | ||
| XCTAssertEqual(headers[B3HTTPHeaders.Multiple.spanIDField], "0000000000000929") | ||
| XCTAssertEqual(headers[B3HTTPHeaders.Multiple.sampledField], "0") | ||
| XCTAssertNil(headers[B3HTTPHeaders.Multiple.parentSpanIDField]) | ||
| XCTAssertEqual(headers[B3HTTPHeaders.Multiple.parentSpanIDField], "000000000000162e") | ||
| } | ||
|
|
||
| func testWritingSampledTraceContext_withMultipleEncoding_andCustomSamplingStrategy() { | ||
|
|
@@ -185,10 +185,10 @@ class B3HTTPHeadersWriterTests: XCTestCase { | |
| ) | ||
|
|
||
| let headers = writer.traceHeaderFields | ||
| XCTAssertNil(headers[B3HTTPHeaders.Multiple.traceIDField]) | ||
| XCTAssertNil(headers[B3HTTPHeaders.Multiple.spanIDField]) | ||
| XCTAssertEqual(headers[B3HTTPHeaders.Multiple.traceIDField], "00000000000004d200000000000004d2") | ||
| XCTAssertEqual(headers[B3HTTPHeaders.Multiple.spanIDField], "0000000000000929") | ||
| XCTAssertEqual(headers[B3HTTPHeaders.Multiple.sampledField], "0") | ||
| XCTAssertNil(headers[B3HTTPHeaders.Multiple.parentSpanIDField]) | ||
| XCTAssertEqual(headers[B3HTTPHeaders.Multiple.parentSpanIDField], "000000000000162e") | ||
| } | ||
|
|
||
| func testItWritesMultipleHeaderWithoutOptionalValues() { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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=0andb3={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 = falseandTraceContextInjection = sampledwe should injectb3=0?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend SDK engineer here 👋🏼
I think we should be flexible on what we extract and accept both
b3=0andb3={TraceId}-{SpanId}-{SamplingState}-{ParentSpanId}styles, but only inject in theb3={TraceId}-{SpanId}-{SamplingState}-{ParentSpanId}styleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofb3=0so 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