RUM-16886: Mark sampled-out network spans as droppable while preserving stats by abrooksv · Pull Request #3596 · DataDog/dd-sdk-android

abrooksv · 2026-07-01T22:08:54Z

What does this PR do?

CoreTracer needs to see all spans to calculate APM client-side stats. Since network spans have their own sampling rate, sampled-out spans are now finished with the _dd.local.force_drop tag instead of being dropped. This tells downstream systems to discard the span while still letting CoreTracer process it for stats calculations.

This only applies when the active tracer is the SDK's own DatadogTracerAdapter, which routes through CoreTraceWriter — the only implementation that honours _dd.local.force_drop. When a custom or global DatadogTracer is in use, sampled-out spans are dropped via drop() as before, preserving backward compatibility.

Note: Some of this can be cleaned up if we drop OpenTrace and move the OKHttp feature to use the new ApmNetworkInstrumentation in V4

Motivation

Allows us to capture APM metrics for traces that are sampled out at the request level

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

abrooksv · 2026-07-01T22:19:00Z

@codex review this

chatgpt-codex-connector · 2026-07-01T22:25:52Z

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: f51c50edf5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

codecov-commenter · 2026-07-01T22:52:43Z

Codecov Report

❌ Patch coverage is 88.13559% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.34%. Comparing base (0795ece) to head (5410db4).

Files with missing lines	Patch %	Lines
...ndroid/trace/internal/ApmNetworkInstrumentation.kt	66.67%	0 Missing and 6 partials ⚠️
...datadog/android/okhttp/trace/TracingInterceptor.kt	95.45%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                      Coverage Diff                      @@
##           feature/client-side-stats    #3596      +/-   ##
=============================================================
+ Coverage                      73.31%   73.34%   +0.03%     
=============================================================
  Files                            986      986              
  Lines                          35981    36011      +30     
  Branches                        6068     6068              
=============================================================
+ Hits                           26376    26409      +33     
- Misses                          7921     7922       +1     
+ Partials                        1684     1680       -4

Files with missing lines	Coverage Δ
...adog/android/trace/internal/_TraceInternalProxy.kt	`37.50% <100.00%> (+4.17%)`	⬆️
...dog/android/trace/internal/data/CoreTraceWriter.kt	`91.18% <100.00%> (+0.55%)`	⬆️
...trace/internal/net/ApmNetworkInstrumentationExt.kt	`81.82% <100.00%> (+0.87%)`	⬆️
.../android/trace/internal/net/RequestTracingState.kt	`100.00% <100.00%> (ø)`
...g/android/cronet/internal/CronetRequestCallback.kt	`85.48% <100.00%> (+0.48%)`	⬆️
...n/com/datadog/android/okhttp/DatadogInterceptor.kt	`84.00% <100.00%> (+0.18%)`	⬆️
...oid/okhttp/internal/RequestTracingStateRegistry.kt	`95.24% <100.00%> (+0.12%)`	⬆️
...datadog/android/okhttp/trace/TracingInterceptor.kt	`83.20% <95.45%> (+<0.01%)`	⬆️
...ndroid/trace/internal/ApmNetworkInstrumentation.kt	`65.12% <66.67%> (+1.70%)`	⬆️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…ng stats CoreTracer needs to see all spans to calculate APM client-side stats. Since network spans have their own sampling rate, sampled-out spans are now finished with the `_dd.local.force_drop` tag instead of being dropped. This tells downstream systems to discard the span while still letting CoreTracer process it for stats calculations. This only applies when the active tracer is the SDK's own `DatadogTracerAdapter`, which routes through `CoreTraceWriter` — the only implementation that honors `_dd.local.force_drop`. When a custom or global `DatadogTracer` is in use, sampled-out spans are dropped via `drop()` as before, preserving backward compatibility.

satween

Left few comments.
isSDKBacked is a bit confusing if someone would look at it without the all context of the feture, so maybe change it into something like isDefaultTracer or isDropingSpanWithTags or something like this

satween · 2026-07-02T14:09:56Z

+ * Internal tag which indicates the span should be created locally but never sent to the backend.
+ */
+@InternalApi
+const val FORCE_DROP_SPAN: String = "_dd.local.force_drop"


could be const

Not sure I follow, it is a const?

satween · 2026-07-02T14:14:26Z

-internal fun DatadogSpan.finishRumAware(isSampled: Boolean, canSendSpan: Boolean) {
-    if (canSendSpan && isSampled) {
+internal fun DatadogSpan.finishRumAware(isSampled: Boolean, canSendSpan: Boolean, isSDKBacked: Boolean) {
+    if (canSendSpan && (isSDKBacked || isSampled)) {


it's hard to understand that complex boolean combination.

maybe could be simplified?

when { !canSendSpan -> drop() isSDKBacked && !sSampled -> { setTag(FORCE_DROP_SPAN, true) finish() } else -> dtop() }

Not sure if the code does the same, but flattening it a bit will make it more readable."

satween · 2026-07-02T14:15:18Z

    val span: DatadogSpan? = null,
-    val sampleRate: Float? = null
+    val sampleRate: Float? = null,
+    val isSDKBacked: Boolean


maybe isDefaultTracer bit better name for this var ?

Yep, that is much better :D

satween · 2026-07-02T14:18:51Z

+        // THEN
+        verifyNoInteractions(mockEventBatchWriter)
+
+        ddSpans.forEach { it.finish() }


this is wired line:

If this is an assertion and you've expected for example that there is no exception thrown - it's better to wrap into assertNotThrowing or something similar.

If this code tries to free up resources - this is the wrong way to do so as if test fails on the lines above - this line won't be executed

A few of the tests in this file have that and I am not sure why, they don't look to serve a purpose to me given they are Forge generated spans

dd-sdk-android/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/internal/data/CoreTraceWriterTest.kt

Lines 304 to 321 in 5410db4

@Test

fun `M not write spans with drop sampling priority W write()`(forge: Forge) {

// GIVEN

val ddSpans = createNonEmptyDdSpans(

forge = forge,

includeDropSamplingPriority = true

)

// WHEN

testedWriter.write(ddSpans)

// THEN

verifyNoInteractions(mockEventBatchWriter)

ddSpans.forEach {

it.finish()

}

}

satween · 2026-07-02T14:19:27Z

            // Then
-            if (fakeIsSampled) verify(mockSpan).finish() else verify(mockSpan).drop()
+            if (fakeIsSampled) {
+                verify(mockSpan, never()).setTag(FORCE_DROP_SPAN, true)


verify(mockSpan, never()).setTag(eq(FORCE_DROP_SPAN), any())

satween · 2026-07-02T14:19:47Z

            // Then
-            if (fakeIsSampled) verify(mockSpan).finish() else verify(mockSpan).drop()
+            if (fakeIsSampled) {
+                verify(mockSpan, never()).setTag(FORCE_DROP_SPAN, true)


satween · 2026-07-02T14:23:15Z

-    private fun DatadogSpan.finishRumAware(isSampled: Boolean) {
-        if (canSendSpan()) {
-            if (isSampled) finish() else drop()
+    private fun DatadogSpan.finishRumAware(isSampled: Boolean, isSDKBacked: Boolean) {


same here for the complex if-else logic. Maybe worth to move it somewhere into DatadogSpanExt extensions?

Ya, its the logic in ApmNetworkInstrumentationExt.kt but that is not visible to the feature/okhttp

Is it worth taking internal off that one and sharing it?

abrooksv force-pushed the abrooks/handle-network-dropped-spans branch 2 times, most recently from 3f00c72 to f51c50e Compare July 1, 2026 22:13

abrooksv force-pushed the abrooks/handle-network-dropped-spans branch from f51c50e to e2c6a81 Compare July 1, 2026 22:23

abrooksv force-pushed the abrooks/handle-network-dropped-spans branch from e2c6a81 to 5410db4 Compare July 2, 2026 00:16

abrooksv marked this pull request as ready for review July 2, 2026 14:04

abrooksv requested review from a team as code owners July 2, 2026 14:04

abrooksv requested a review from satween July 2, 2026 14:05

satween reviewed Jul 2, 2026

View reviewed changes

	@Test
	fun `M not write spans with drop sampling priority W write()`(forge: Forge) {
	// GIVEN
	val ddSpans = createNonEmptyDdSpans(
	forge = forge,
	includeDropSamplingPriority = true
	)

	// WHEN
	testedWriter.write(ddSpans)

	// THEN
	verifyNoInteractions(mockEventBatchWriter)

	ddSpans.forEach {
	it.finish()
	}
	}

Uh oh!

Conversation

abrooksv commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

abrooksv commented Jul 1, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 1, 2026

Uh oh!

codecov-commenter commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

abrooksv Jul 2, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

abrooksv Jul 2, 2026 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abrooksv commented Jul 1, 2026 •

edited

Loading

codecov-commenter commented Jul 1, 2026 •

edited

Loading

abrooksv Jul 2, 2026 •

edited

Loading

abrooksv Jul 2, 2026 •

edited

Loading