RUM-3470 chore: Add tests for head-based trace sampling by ncreated · Pull Request #1772 · DataDog/dd-sdk-ios

ncreated · 2024-04-11T11:07:57Z

What and why?

📦 🧰 This PR prepares DatadogTrace for the implementation of head-based sampling. It only brings refactoring and fills gaps in tests coverage. There are no changes to existing behaviours.

Notably, it adds HeadBasedSamplingTests suite that covers main scenarios we want to achieve with head-based sampling. That includes:

Head-based sampling of local trace, where all spans must be either "kept" or "dropped":

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

Head-based sampling of distributed trace, where all spans must be either "kept" or "dropped":

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

Head-based sampling of distributed trace with local parent, where all spans must be either "kept" or "dropped":

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

The work has to be split between RUM-3470 (next PR) and RUM-3535 ("trace context injection"). To embrace common understanding of what we want to do, I propose to go red → green refactoring, hence I'm adding multiple tests that are yet disabled.

How?

Added HeadBasedSamplingTests to add missing coverage and set common goals for RUM-3470 and RUM-3535;
Renamed samplers in DatadogTrace for more clarity:
- sampler → localTraceSampler in DatadogTracer;
- tracingSampler → distributedTraceSampler in TracingURLSessionHandler;
To facilitate further work and better fit tests pyramid, moved first tracer tests from DatadogCoreTests (integration testing) to DatadogTraceTests (unit testing).

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/

instead of core. This is to remove core reference and migrate towards short-term goals from #1744

… names and fix comments

datadog-datadog-prod-us1 · 2024-04-11T11:14:56Z

Datadog Report

Branch report: ncreated/RUM-3470/add-head-based-sampling-tests
Commit report: e7ac165
Test service: dd-sdk-ios

✅ 0 Failed, 3000 Passed, 0 Skipped, 11m 13.69s Wall Time
🔻 Test Sessions change in coverage: 8 decreased, 6 increased

🔻 Code Coverage Decreases vs Default Branch (8)

This report shows up to 5 code coverage decreases.

test DatadogCrashReportingTests tvOS 27.91% (-0.69%) - Details
test DatadogCrashReportingTests iOS 27.87% (-0.69%) - Details
test DatadogWebViewTrackingTests iOS 17.13% (-0.68%) - Details
test DatadogCoreTests tvOS 79.64% (-0.62%) - Details
test DatadogCoreTests iOS 73.56% (-0.4%) - Details

ncreated · 2024-04-11T11:23:02Z

        XCTAssertEqual(try spanMatcher.meta.custom(keyPath: "meta.globaltag2"), "overwrittenValue")
    }

-    // MARK: - Tracer with sampling rate


💡 Moved to DatadogTracer+SamplingTests.swif - now part of DatadogTraceTests instead of DatadogCoreTests.

ncreated · 2024-04-11T11:23:21Z

    func interceptionDidComplete(interception: URLSessionTaskInterception)
 }

-internal struct NOPDatadogURLSessionInterceptor: DatadogURLSessionHandler {


💡 🧹 not used anywhere

ncreated · 2024-04-11T11:25:11Z

 @available(*, deprecated, message: "Use `URLSessionInstrumentation.enable(with:)` instead.")
 open class DatadogURLSessionDelegate: NSObject, URLSessionDataDelegate {
-    var interceptor: URLSessionInterceptor? {
+    var feature: NetworkInstrumentationFeature? {


💡 🧹 to further drop dependency on URLSessionInterceptor - now it is only required for DatadogAlamofireExtension (soon dropped in RUM-3060).

ncreated · 2024-04-11T11:27:33Z

-    internal weak var core: DatadogCoreProtocol?
+    /// Trace feature scope.
+    private let featureScope: FeatureScope


💡 to achieve mid-term goals ("no core dependency") from #1744. With this change we facilitate testing DatadogTracer in DatadogTraceTests (vs testing with core mock in DatadogCoreTests).

ncreated · 2024-04-11T11:28:40Z

    internal let origin: String?
-    /// The sampling rate for the span (between 0 and 100)
+    /// The sampling rate for the span (between 0 and 1)
    internal let samplingRate: Float


💡 🧹 this wasn't correct - when encoded, we normalize sample rate to 0...1 range

ncreated · 2024-04-11T11:43:58Z

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


💡 These tests are disabled on purpose. I propose we go red → green through RUM-3470 (next PR) and later RUM-3535 ("trace context injection control").

ganeshnj

Very neatly written test cases. a couple of question, everything looks great.

ganeshnj · 2024-04-12T08:01:19Z

+        let allKept = spans.filter({ $0.isKept }).count == 3
+        let allDropped = spans.filter({ !$0.isKept }).count == 3


❓
do we have inconsistent isKept flag for local spans in a trace?

Yes 😞, this test is red if enabled. Although the original issue touches only on head-based sampling for distributed traces I'll be fixing it for local traces as well. It might be one fix that works for both.

ganeshnj · 2024-04-12T08:02:39Z

+    // MARK: - Distributed Tracing
+
+    // TODO: RUM-3470 Enable this test when head-based sampling is supported
+    func testSendingSampledDistributedTraceWithNoParent() throws {


ganeshnj · 2024-04-12T08:11:17Z

+    }
+
+    // TODO: RUM-3470 Enable this test when head-based sampling is supported
+    func testSendingSampledDistributedTraceWithParent() throws {


ncreated added 5 commits April 11, 2024 13:02

RUM-3470 Refactor DatadogTracer to depend on FeatureScope

b31dccc

instead of core. This is to remove core reference and migrate towards short-term goals from #1744

RUM-3470 Add missing tests for sampling in header writers; Update API…

4965c61

… names and fix comments

RUM-3470 Add (yet disabled) tests for head-based sampling

796802d

RUM-3470 Renaming

37f3c30

RUM-3470 Fix after rebase

a87a82f

ncreated self-assigned this Apr 11, 2024

ncreated added 2 commits April 11, 2024 13:19

RUM-3470 Disable (yet) red test in DatadogCoreTests scheme

fedde50

RUM-3470 Rename test file

0484d17

ncreated commented Apr 11, 2024

View reviewed changes

RUM-3470 Disable (yet) red test for tvOS

e7ac165

ncreated commented Apr 11, 2024

View reviewed changes

ncreated marked this pull request as ready for review April 11, 2024 12:19

ncreated requested review from a team as code owners April 11, 2024 12:19

ganeshnj reviewed Apr 12, 2024

View reviewed changes

ganeshnj approved these changes Apr 12, 2024

View reviewed changes

ncreated merged commit 891e2d3 into develop Apr 12, 2024

ncreated deleted the ncreated/RUM-3470/add-head-based-sampling-tests branch April 12, 2024 15:22

This was referenced Apr 17, 2024

RUM-3470 feat: Head-based sampling for local and (automatic) distributed tracing #1783

Merged

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

Merged

ncreated mentioned this pull request Jun 25, 2025

RUM-9495: Remove deprecated APIs #2361

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RUM-3470 chore: Add tests for head-based trace sampling#1772

RUM-3470 chore: Add tests for head-based trace sampling#1772
ncreated merged 8 commits into
developfrom
ncreated/RUM-3470/add-head-based-sampling-tests

ncreated commented Apr 11, 2024 •

edited

Loading

Uh oh!

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

edited

Loading

Uh oh!

ncreated Apr 11, 2024

Uh oh!

ncreated Apr 11, 2024

Uh oh!

ncreated Apr 11, 2024

Uh oh!

ncreated Apr 11, 2024 •

edited

Loading

Uh oh!

ncreated Apr 11, 2024

Uh oh!

ncreated Apr 11, 2024

Uh oh!

ganeshnj left a comment

Uh oh!

ganeshnj Apr 12, 2024

Uh oh!

ncreated Apr 12, 2024

Uh oh!

ganeshnj Apr 12, 2024

Uh oh!

ganeshnj Apr 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let allKept = spans.filter({ $0.isKept }).count == 3
		let allDropped = spans.filter({ !$0.isKept }).count == 3

Uh oh!

Conversation

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

Uh oh!

What and why?

How?

Review checklist

Custom CI job configuration (optional)

Uh oh!

datadog-datadog-prod-us1 Bot commented Apr 11, 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 (8)

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!

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

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 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ncreated commented Apr 11, 2024 •

edited

Loading

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

edited

Loading

ncreated Apr 11, 2024 •

edited

Loading