-
Notifications
You must be signed in to change notification settings - Fork 133
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
RUM-3470 feat: Head-based sampling for manual distributed tracing #1793
RUM-3470 feat: Head-based sampling for manual distributed tracing #1793
Conversation
Datadog ReportBranch report: β
0 Failed, 3016 Passed, 0 Skipped, 11m 35.89s Wall Time π» Code Coverage Decreases vs Default Branch (12)
|
f93f17e
to
d8c2f2b
Compare
|
||
func testItInjectsSpanContextWithHTTPHeadersWriter() { | ||
Trace.enable(with: config, in: core) | ||
let tracer = Tracer.shared(in: core) | ||
let spanContext1 = DDSpanContext(traceID: .init(idHi: 10, idLo: 100), spanID: 200, parentSpanID: .mockAny(), baggageItems: .mockAny(), sampleRate: .mockRandom(), isKept: .mockRandom()) |
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'm removing these tests from integration layer. Headers encoding / decoding was moved to DatadogTrace
unit tests. This includes adding missing UT coverage for HTTPHeadersWriter
.
DatadogInternal/Sources/NetworkInstrumentation/TracePropagationHeadersWriter.swift
Outdated
Show resolved
Hide resolved
blocking/ Note to myself:
|
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.
Neatly done @ncreated :kudos
DatadogInternal/Sources/NetworkInstrumentation/TracePropagationHeadersWriter.swift
Outdated
Show resolved
Hide resolved
@@ -162,33 +161,29 @@ extension DistributedTracing { | |||
let writer: TracePropagationHeadersWriter | |||
switch $0 { | |||
case .datadog: | |||
writer = HTTPHeadersWriter(sampler: deterministicSampler) | |||
writer = HTTPHeadersWriter(samplingStrategy: .auto) |
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.
Changing the default to auto/head is behavior breaking change. We must make sure this in our changelog.
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.
@ganeshnj There should be no breaking change here, at least in my best efforts π. Please note that .auto
here implies "derive sampling decision (0|1) from injected span context". The injectedSpanContext
is sampled with the same sampler
that was used before:
let injectedSpanContext = TraceContext(
// ...
sampleRate: sampler.samplingRate,
isKept: sampler.sample()
)
Let me know WDYT or if I'm missing something.
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.
here injected span context is basically span context of active span (Tracer) which we were not considering before while sampling the distributed trace (URLSessionTracking), which could mean more traces depending on the sampling rate defined at Trace level.
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 see. So this regards changes made to TracingURLSessionHandler
- RUM handler is backward compatible π. I will add a note on this in the consolidation PR (#1794).
@@ -32,23 +32,16 @@ public struct Sampler: Sampling { | |||
} | |||
|
|||
/// A sampler that determines sampling decisions deterministically (the same each time). | |||
public struct DeterministicSampler: Sampling { | |||
internal struct DeterministicSampler: Sampling { |
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.
πͺ
cdd181f
into
ncreated/RUM-3470/head-based-sampling
What and why?
π¦ This PR continues the efforts from #1783 and earlier #1772. It adds support to head-based sampling for distributed traces created manually with Tracer API:
*HTTPHeadersWriter
):*HTTPHeadersWriter
) with implicit parent:How?
In head-based sampling mode, headers must be sampled the same as the injected span context (
spanContext.isKept
). However, this contradicts with existinginit(sampleRate:...)
constructors. To accommodate this change, a new initializer (init(samplingStrategy: TraceSamplingStrategy)) was added to each headers writer. Additionally, existing constructors have been deprecated to encourage migration. Users should now consciously choose between arbitrary sampling or the new head-based approach:The
TraceSamplingStrategy
enum enables both the new behavior (.auto
) and the previous one (.custom(sampleRate:)
):π‘ Alternatives Considered
Another option was introducing new types for head-based sampling, but this approach would require solving the
RUM-3535: Trace Context injection control
issue first. It would simplify things, notably the uniformity of setting trace propagation headers across writers which varies:HTTPHeadersWriter
and inB3HTTPHeadersWriter
W3CHTTPHeadersWriter
Adopting head-based sampling in existing writers was chosen to avoid this complexity.
Review checklist
Custom CI job configuration (optional)
tools/