RUM-3470 feat: Head-based sampling for local and (automatic) distributed tracing by ncreated · Pull Request #1783 · DataDog/dd-sdk-ios

ncreated · 2024-04-17T10:02:17Z

What and why?

📦 This PR implements head-based sampling for the scenarios defined earlier in #1772. It also adds tests for one more class of scenarios: using distributed trace created manually with OTFormatWriter API.

See all scenarios for head-based sampling vs their support level:

✅ Local trace with explicit parent:

client-ios-app:     [---- custom.span.1 -----]
client-ios-app:       [-- custom.span.2 ---]
client-ios-app:         [- custom.span.3 -]

✅ Local trace with implicit parent:

client-ios-app:     [----- active.span ------]
client-ios-app:       [-- custom.span.1 ---]
client-ios-app:         [- custom.span.2 -]

✅ Distributed trace sent via URLSessionInstrumentation:

dd-sdk-ios:         [--- urlsession.request ---]
client backend:        [--- backend span ---]

✅ Distributed trace sent via URLSessionInstrumentation with implicit parent:

client-ios-app:     [-------- active.span ---------]
dd-sdk-ios:           [--- urlsession.request ---]
client backend:          [--- backend span ---]

❌ Distributed trace sent via Tracer API (*HTTPHeadersWriter):

client-ios-app:     [------ network.span -------]
client backend:         [--- backend span ---]

❌ Distributed trace sent via Tracer API (*HTTPHeadersWriter) with implicit parent:

client-ios-app:     [------ active.span -------]
client-ios-app:       [---- network.span ----]
client backend:         [-- backend span --]

The last 2 scenarios are defined as failing (and disabled) tests. They will be supported and enabled in next PR.

How?

I did few changes that enable this feature.

The sampling decision is now a part of `DDSpanContext`

In OT, creating child span requires passing the OTSpanContext of the parent. This makes a good place for propagating the sampling decision for head-based sampling. For that reason, sampleRate and isKept are added to DDSpanContext.

DDSpanContext (parent): {traceID, spanID, nil, isKept, sampleRate}

                                   ↓

DDSpanContext (child):  {parent.traceID, spanID, parent.spanID, parent.isKept, parent.sampleRate}

Note: While isKept is crucial for sampling the whole trace equally, the sampleRate is not. We need to propagate it, so it can be later sent in SpanEvent for computing BE-side metrics.

The `OTFormatReader` implementations can now extract the sampling decision

Because the OTTracer can encode and decode the OTSpanContext:

// OTTracer
func inject(spanContext: OTSpanContext, writer: OTFormatWriter)
func extract(reader: OTFormatReader) -> OTSpanContext?

the implementations of HTTP header readers (DD, W3C and B3) were extended to extract the isKept and sampleRate.

Note: The sampleRate is not encoded as header so it needs to be supplied by the tracer in extract(reader:).

`NetworkInstrumentationFeature` no longer decodes `TraceContext` from request headers

I enhanced the implementation of passing TraceContext between task interception methods. It now avoids coding it through request headers, which enables passing extra information not available in standard headers (the sampleRate).

Before:

// NetworkInstrumentationFeature
let request = self.intercept(request: currentRequest, ...) // <-- encode TraceContext in request headers
self.intercept(task: task, ...) // <-- decode TraceContext from task.currentRequest headers

Now:

let (request, traceContexts) = self.intercept(request: currentRequest, ...)
self.intercept(task: task, with: traceContexts, ...)

Note: This simplifies and enhances task interception in the main flow. However, it required additional changes in URLSessionInterceptor which is required for Alamofire Extension.

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference
Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

Run unit tests for Core, RUM, Trace, Logs, CR and WVT
Run unit tests for Session Replay
Run integration tests
Run smoke tests
Run tests for tools/

ncreated · 2024-04-17T10:08:03Z

-        let spanContext1 = DDSpanContext(traceID: .init(idHi: 10, idLo: 100), spanID: 200, parentSpanID: .mockAny(), baggageItems: .mockAny())
-        let spanContext2 = DDSpanContext(traceID: 3, spanID: 4, parentSpanID: .mockAny(), baggageItems: .mockAny())
+        let spanContext1 = DDSpanContext(traceID: .init(idHi: 10, idLo: 100), spanID: 200, parentSpanID: .mockAny(), baggageItems: .mockAny(), sampleRate: .mockRandom(), isKept: .mockRandom())
+        let spanContext2 = DDSpanContext(traceID: 3, spanID: 4, parentSpanID: .mockAny(), baggageItems: .mockAny(), sampleRate: .mockRandom(), isKept: .mockRandom())

-        let httpHeadersWriter = HTTPHeadersWriter(sampler: .mockKeepAll())
+        let httpHeadersWriter = HTTPHeadersWriter(sampler: Sampler.mockKeepAll())


This surfaces the problem of head-based sampling in manual distributed tracing. No matter of the isKept decision made by tracer for spanContext, distributed trace headers are sampled exclusively using the sampler configured in *HTTPHeadersWriter.

I will work on this in "part 2" of this PR.

datadog-datadog-prod-us1 · 2024-04-17T10:11:11Z

Datadog Report

Branch report: ncreated/RUM-3470/head-based-sampling-part1
Commit report: c0f09b2
Test service: dd-sdk-ios

✅ 0 Failed, 3004 Passed, 0 Skipped, 12m 19.28s Wall Time
🔻 Test Sessions change in coverage: 13 decreased, 1 no change

🔻 Code Coverage Decreases vs Default Branch (13)

This report shows up to 5 code coverage decreases.

test DatadogInternalTests iOS 79.64% (-0.52%) - Details
test DatadogInternalTests tvOS 79.67% (-0.47%) - Details
test DatadogTraceTests iOS 49.15% (-0.31%) - Details
test DatadogRUMTests tvOS 80.73% (-0.3%) - Details
test DatadogLogsTests tvOS 45.18% (-0.19%) - Details

ncreated · 2024-04-17T11:37:59Z

+            sampleRate: parentSpanContext?.sampleRate ?? sampler.samplingRate,
+            isKept: parentSpanContext?.isKept ?? sampler.sample()


💡 This is the main point for head-based sampling implementation in this PR:

use the sampling decision from the parent span (if available)

or determine it based on provided sampler (which can be local or distributed trace sampler).

…atic distributed tracing

ncreated · 2024-04-17T13:02:59Z

            <SkippedTests>
               <Test
-                  Identifier = "HeadBasedSamplingTests/testSamplingLocalTrace()">
+                  Identifier = "HeadBasedSamplingTests/testSendingDroppedDistributedTraceWithNoParent_throughTracerAPI()">


💡 Some tests were renamed, some "red" ones were enabled (now "green") and some new "red" tests were added (disabled) to be fixed in next PR.

ncreated · 2024-04-17T13:05:48Z

-    /// Returns the trace of the current execution context.
-    func traceContext() -> TraceContext?


💡 No longer needed. It was actually doubling the effort:

active span was obtained once in modify(request:) of Trace handler,

obtained second time in intercept(task:) in NetworkInstrumentationFeature,

it was no-op in RUM handler.

Now we read it only once, in Trace handler and pass the parentSpanID in TraceContext.

ncreated · 2024-04-17T13:06:52Z


 import Foundation

+// TODO: RUM-3470 Add tests to `URLSessionInterceptor`


💡 URLSessionInterception has no tests coverage. I will add tests in next PR. Here, we're merging against feature branch.

ncreated · 2024-04-17T13:07:52Z

-    func testGivenOpenTracing_whenInterceptingRequests_itInjectsTrace() throws {
-        // Given


💡 All these tests are not necessary because we removed traceContext() in DatadogURLSessionHandler.

ncreated · 2024-04-17T13:08:31Z

-    func testTraceContext_whenInterceptionStarts_withActiveSpan_itReturnCurrentSpan() {
-        // When


💡 Not necessary with the removal of traceContext() from DatadogURLSessionHandler.

maxep

Looks good overall 👍 but I left a blocking point that we can discuss first.

…ders

ganeshnj

Looks good nothing block.

ganeshnj · 2024-04-19T14:00:13Z

    ///
    /// - Parameter sampler: The sampler used for headers injection.
-    public init(sampler: Sampler) {
+    public init(sampler: Sampling) {


❗
This is a breaking change

Technically yes, but the Sampler type is only defined in DatadogInternal without being re-exported from user-facing modules. In practise, the only supported public initializer for HTTPHeadersWriter is init(sampleRate:), which is not altered by this change.

ganeshnj · 2024-04-19T14:10:20Z

 }
+
+/// A sampler that determines sampling decisions deterministically (the same each time).
+public struct DeterministicSampler: Sampling {


for inspiration, I think we can multiple samplers like here https://github.com/open-telemetry/opentelemetry-swift/blob/4eb75bc8f0d6449bc197637b5c6044b51ab8c4ed/Sources/OpenTelemetrySdk/Trace/Samplers/Samplers.swift#L10

I'm particularly interested in ParentBasedSampler

@ganeshnj I'm sure what is the actual ask here 🙂, could you clarify? The SDK doesn't expose any sampler implementation to the end-user (interface + impls exist only in DatadogInternal). Because end-users only work with sampleRate: Float API today, I don't see reason for introducing more samplers than we need for SDK internals.

apologies, it wasn't clear. I mean to say - for our internal working we can use some existing pattern. I wasn't expecting any action on the comment. Just for information if we have tricky cases around sampling decision.

ganeshnj · 2024-04-19T14:14:36Z

✅ Local trace with implicit parent:

client-ios-app:     [----- active.span ------]
client-ios-app:       [-- custom.span.1 ---]
client-ios-app:         [- custom.span.2 -]

This must be rather from parent-child point of view.

client-ios-app:     [----- active.span ------]
client-ios-app:       [-- custom.span.1 ---]
client-ios-app:       [-- custom.span.2 ---]

ncreated · 2024-04-22T07:51:48Z

✅ Local trace with implicit parent:

client-ios-app:     [----- active.span ------]
client-ios-app:       [-- custom.span.1 ---]
client-ios-app:         [- custom.span.2 -]

This must be rather from parent-child point of view.

client-ios-app:     [----- active.span ------]
client-ios-app:       [-- custom.span.1 ---]
client-ios-app:       [-- custom.span.2 ---]

It already is:

 client-ios-app:     [-------- active.span -----]   |
 client-ios-app:       [- child1 -][- child2 -]     | all 3: keep or drop

^ one parent with two childs

maxep

Well done!

ncreated self-assigned this Apr 17, 2024

ncreated commented Apr 17, 2024

View reviewed changes

ncreated force-pushed the ncreated/RUM-3470/head-based-sampling-part1 branch from b552be4 to 39341f0 Compare April 17, 2024 10:52

ncreated commented Apr 17, 2024

View reviewed changes

RUM-3470 Support head-based trace sampling for local traces and autom…

8a89706

…atic distributed tracing

ncreated force-pushed the ncreated/RUM-3470/head-based-sampling-part1 branch from 39341f0 to 8a89706 Compare April 17, 2024 11:39

ncreated marked this pull request as ready for review April 17, 2024 12:59

ncreated requested review from a team as code owners April 17, 2024 12:59

ncreated commented Apr 17, 2024

View reviewed changes

maxep requested changes Apr 18, 2024

View reviewed changes

Comment thread DatadogTrace/Sources/DatadogTracer.swift Outdated

RUM-3470 CR feedback - remove tracerSampleRate from HTTP header rea…

c0f09b2

…ders

ncreated requested a review from maxep April 19, 2024 09:21

ganeshnj reviewed Apr 19, 2024

View reviewed changes

ncreated requested a review from ganeshnj April 22, 2024 07:51

ganeshnj approved these changes Apr 22, 2024

View reviewed changes

maxep approved these changes Apr 22, 2024

View reviewed changes

ncreated merged commit c452a2d into ncreated/RUM-3470/head-based-sampling Apr 22, 2024

ncreated deleted the ncreated/RUM-3470/head-based-sampling-part1 branch April 22, 2024 14:59

This was referenced Apr 24, 2024

RUM-3470 feat: Head-based sampling for manual distributed tracing #1793

Merged

RUM-3470 feat: Head-based sampling #1794

Merged

maciejburda mentioned this pull request May 7, 2024

Dogfood recent changes #1814

Closed

8 tasks

0xnm mentioned this pull request Jan 13, 2025

RUM-8051: Implement Head-based sampling for network instrumentation DataDog/dd-sdk-android#2483

Merged

3 tasks

		sampleRate: parentSpanContext?.sampleRate ?? sampler.samplingRate,
		isKept: parentSpanContext?.isKept ?? sampler.sample()

		/// Returns the trace of the current execution context.
		func traceContext() -> TraceContext?


		import Foundation

		// TODO: RUM-3470 Add tests to `URLSessionInterceptor`

		func testGivenOpenTracing_whenInterceptingRequests_itInjectsTrace() throws {
		// Given

		func testTraceContext_whenInterceptionStarts_withActiveSpan_itReturnCurrentSpan() {
		// When

Uh oh!

Conversation

ncreated commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What and why?

How?

The sampling decision is now a part of DDSpanContext

The OTFormatReader implementations can now extract the sampling decision

NetworkInstrumentationFeature no longer decodes TraceContext from request headers

Review checklist

Custom CI job configuration (optional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

datadog-datadog-prod-us1 Bot commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Datadog Report

🔻 Code Coverage Decreases vs Default Branch (13)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxep left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ganeshnj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ganeshnj commented Apr 19, 2024

Uh oh!

ncreated commented Apr 22, 2024

Uh oh!

maxep left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ncreated commented Apr 17, 2024 •

edited

Loading

The sampling decision is now a part of `DDSpanContext`

The `OTFormatReader` implementations can now extract the sampling decision

`NetworkInstrumentationFeature` no longer decodes `TraceContext` from request headers

datadog-datadog-prod-us1 Bot commented Apr 17, 2024 •

edited

Loading