Skip to content

Commit 5feeb98

Browse files
hamorillohamorillo
committed
RUM-15138: Address PR review feedback — simplify sampler config, harden test constants
1 parent 1dfbfca commit 5feeb98

6 files changed

Lines changed: 57 additions & 43 deletions

File tree

‎features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/ApmNetworkInstrumentationConfiguration.kt‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,7 @@ class ApmNetworkInstrumentationConfiguration internal constructor(
4646
internal var localTracerFactory: (SdkCore, Set<TracingHeaderType>) -> DatadogTracer = DEFAULT_LOCAL_TRACER_FACTORY,
4747
internal var traceContextInjection: TraceContextInjection = TraceContextInjection.SAMPLED,
4848
internal var tracedRequestListener: NetworkTracedRequestListener = NoOpNetworkTracedRequestListener(),
49-
internal var traceSampleRate: Float = DEFAULT_TRACE_SAMPLE_RATE,
50-
internal var customTraceSampler: Sampler<DatadogSpan>? = null,
49+
internal var traceSampler: Sampler<DatadogSpan> = DeterministicTraceSampler(DEFAULT_TRACE_SAMPLE_RATE),
5150
internal var globalTracerProvider: () -> DatadogTracer? = { GlobalDatadogTracer.getOrNull() },
5251
internal var networkTracingScope: ApmNetworkTracingScope = ApmNetworkTracingScope.EXCLUDE_INTERNAL_REDIRECTS,
5352
internal var headerPropagationOnly: Boolean = false
@@ -122,7 +121,7 @@ class ApmNetworkInstrumentationConfiguration internal constructor(
122121
* @param sampleRate the sample rate to use (percentage between 0f and 100f, default is 100f).
123122
*/
124123
fun setTraceSampleRate(@FloatRange(from = 0.0, to = 100.0) sampleRate: Float) = apply {
125-
this.traceSampleRate = sampleRate
124+
this.traceSampler = DeterministicTraceSampler(sampleRate)
126125
}
127126

128127
/**
@@ -133,7 +132,7 @@ class ApmNetworkInstrumentationConfiguration internal constructor(
133132
* By default it is a sampler accepting 100% of the traces.
134133
*/
135134
fun setTraceSampler(traceSampler: Sampler<DatadogSpan>) = apply {
136-
this.customTraceSampler = traceSampler
135+
this.traceSampler = traceSampler
137136
}
138137

139138
/**
@@ -219,8 +218,7 @@ class ApmNetworkInstrumentationConfiguration internal constructor(
219218
localTracerFactory = localTracerFactory,
220219
traceContextInjection = traceContextInjection,
221220
tracedRequestListener = tracedRequestListener,
222-
traceSampleRate = traceSampleRate,
223-
customTraceSampler = customTraceSampler,
221+
traceSampler = traceSampler,
224222
globalTracerProvider = globalTracerProvider,
225223
networkTracingScope = networkTracingScope,
226224
headerPropagationOnly = headerPropagationOnly
@@ -249,13 +247,10 @@ class ApmNetworkInstrumentationConfiguration internal constructor(
249247

250248
val tracerProvider = TracerProvider(localTracerFactory, globalTracerProvider)
251249

252-
val effectiveSampler = customTraceSampler
253-
?: DeterministicTraceSampler(traceSampleRate)
254-
255250
return ApmNetworkInstrumentation(
256251
canSendSpan = !headerPropagationOnly,
257252
traceOrigin = traceOrigin,
258-
traceSampler = effectiveSampler,
253+
traceSampler = traceSampler,
259254
tracerProvider = tracerProvider,
260255
sdkInstanceName = sdkInstanceName,
261256
injectionType = traceContextInjection,

‎features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/internal/ApmNetworkInstrumentation.kt‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ class ApmNetworkInstrumentation internal constructor(
130130
}
131131

132132
val span = tracer.buildSpan(request, networkingLibraryName, traceOrigin)
133-
val isSampled = span.extractRumContext(rumContextPropagator, block = true).sample(request, traceSampler)
133+
val isSampled = span.isSampled(request)
134134
if (span.isRootSpan) {
135135
span.applyPriority(isSampled, traceSampler)
136136
}
@@ -220,6 +220,10 @@ class ApmNetworkInstrumentation internal constructor(
220220
)
221221
}
222222

223+
private fun DatadogSpan.isSampled(request: HttpRequestInfo): Boolean =
224+
extractRumContext(rumContextPropagator, block = true)
225+
.sample(request, traceSampler)
226+
223227
private fun RequestTracingState.onRequestIntercepted(
224228
response: HttpResponseInfo?,
225229
throwable: Throwable?

‎features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/internal/RumContextPropagatorTest.kt‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import com.datadog.android.trace.utils.RumContextTestsUtils.RUM_CONTEXT_ACTION_I
2525
import com.datadog.android.trace.utils.RumContextTestsUtils.RUM_CONTEXT_APPLICATION_ID
2626
import com.datadog.android.trace.utils.RumContextTestsUtils.RUM_CONTEXT_SESSION_ID
2727
import com.datadog.android.trace.utils.RumContextTestsUtils.RUM_CONTEXT_VIEW_ID
28-
import com.datadog.android.trace.utils.RumContextTestsUtils.RUM_SESSION_SAMPLE_RATE
2928
import com.datadog.android.trace.utils.RumContextTestsUtils.aDatadogContextWithRumContext
3029
import com.datadog.android.trace.utils.RumContextTestsUtils.aRumContext
3130
import com.datadog.android.utils.forge.Configurator
@@ -294,7 +293,7 @@ class RumContextPropagatorTest {
294293
verify(span).setTag(LogAttributes.RUM_SESSION_ID, fakeRumContext[RUM_CONTEXT_SESSION_ID])
295294
verify(span).setTag(LogAttributes.RUM_VIEW_ID, fakeRumContext[RUM_CONTEXT_VIEW_ID])
296295
verify(span).setTag(LogAttributes.RUM_ACTION_ID, fakeRumContext[RUM_CONTEXT_ACTION_ID])
297-
verify(span).setTag(LogAttributes.RUM_SESSION_SAMPLE_RATE, fakeRumContext[RUM_SESSION_SAMPLE_RATE])
296+
verify(span).setTag(LogAttributes.RUM_SESSION_SAMPLE_RATE, fakeRumContext[RUM_CONTEXT_SESSION_SAMPLE_RATE])
298297
verify(span).setTag(HttpCodec.RUM_KEY_ACCOUNT_ID, fakeDatadogContext.accountInfo?.id as? Any)
299298
verify(span).setTag(HttpCodec.RUM_KEY_USER_ID, fakeDatadogContext.userInfo.id as? Any)
300299
verify(span).setTag(DATADOG_INITIAL_CONTEXT, null as Any?)
@@ -321,7 +320,7 @@ class RumContextPropagatorTest {
321320
verify(span).setTag(LogAttributes.RUM_ACTION_ID, fakeRumContext[RUM_CONTEXT_ACTION_ID])
322321
verify(
323322
span
324-
).setTag(LogAttributes.RUM_SESSION_SAMPLE_RATE, fakeRumContext[RUM_SESSION_SAMPLE_RATE])
323+
).setTag(LogAttributes.RUM_SESSION_SAMPLE_RATE, fakeRumContext[RUM_CONTEXT_SESSION_SAMPLE_RATE])
325324
verify(span).setTag(HttpCodec.RUM_KEY_ACCOUNT_ID, fakeDatadogContext.accountInfo?.id as? Any)
326325
verify(span).setTag(HttpCodec.RUM_KEY_USER_ID, fakeDatadogContext.userInfo.id as? Any)
327326
verify(span).setTag(DATADOG_INITIAL_CONTEXT, null as Any?)

‎features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/internal/net/ApmInstrumentationConfigurationTest.kt‎

Lines changed: 3 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ internal class ApmInstrumentationConfigurationTest {
142142
}
143143

144144
@Test
145-
fun `M keep custom sampler W setTraceSampler() then setTraceSampleRate()`(
145+
fun `M use last writer W setTraceSampler() then setTraceSampleRate()`(
146146
@FloatForgery(min = 0f, max = 100f) fakeSampleRate: Float
147147
) {
148148
// When
@@ -152,7 +152,8 @@ internal class ApmInstrumentationConfigurationTest {
152152
.createInstrumentation(fakeNetworkLibraryName)
153153

154154
// Then
155-
assertThat(result.traceSampler).isSameAs(mockTraceSampler)
155+
assertThat(result.traceSampler).isInstanceOf(DeterministicTraceSampler::class.java)
156+
assertThat(result.traceSampler.getSampleRate()).isEqualTo(fakeSampleRate)
156157
}
157158

158159
@Test
@@ -254,28 +255,4 @@ internal class ApmInstrumentationConfigurationTest {
254255
// Then
255256
assertThat(result.networkTracingScope).isEqualTo(fakeScope)
256257
}
257-
258-
@Test
259-
fun `M use custom sampler W createInstrumentation() {custom sampler provided}`() {
260-
// When
261-
val result = testedBuilder
262-
.setTraceSampler(mockTraceSampler)
263-
.createInstrumentation(fakeNetworkLibraryName)
264-
265-
// Then
266-
assertThat(result.traceSampler).isSameAs(mockTraceSampler)
267-
}
268-
269-
@Test
270-
fun `M return raw trace rate W createInstrumentation() {default sampler}`(
271-
@FloatForgery(min = 0f, max = 100f) fakeSampleRate: Float
272-
) {
273-
// When
274-
val result = testedBuilder
275-
.setTraceSampleRate(fakeSampleRate)
276-
.createInstrumentation(fakeNetworkLibraryName)
277-
278-
// Then — getSampleRate() returns the raw configured rate (no rebasing without a span)
279-
assertThat(result.traceSampler.getSampleRate()).isEqualTo(fakeSampleRate)
280-
}
281258
}

‎features/dd-sdk-android-trace/src/testFixtures/kotlin/com/datadog/android/trace/utils/RumContextTestsUtils.kt‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,8 @@ object RumContextTestsUtils {
1919
const val RUM_CONTEXT_SESSION_ID = "session_id"
2020
const val RUM_CONTEXT_APPLICATION_ID = "application_id"
2121

22-
const val RUM_SESSION_SAMPLE_RATE = "session_sample_rate"
22+
const val RUM_CONTEXT_SESSION_SAMPLE_RATE = "session_sample_rate"
2323

24-
private const val SESSION_SAMPLE_RATE_KEY = "session_sample_rate"
2524
private const val HEX = 16
2625

2726
fun Forge.aRumContext(
@@ -31,7 +30,7 @@ object RumContextTestsUtils {
3130
put(RUM_CONTEXT_VIEW_ID, getForgery<UUID>().toString())
3231
put(RUM_CONTEXT_ACTION_ID, getForgery<UUID>().toString())
3332
put(RUM_CONTEXT_APPLICATION_ID, getForgery<UUID>().toString())
34-
put(SESSION_SAMPLE_RATE_KEY, sessionSampleRate ?: aFloat(min = 0f, max = 100f))
33+
put(RUM_CONTEXT_SESSION_SAMPLE_RATE, sessionSampleRate ?: aFloat(min = 0f, max = 100f))
3534
sessionId?.let {
3635
put(
3736
RUM_CONTEXT_SESSION_ID,

‎integrations/dd-sdk-android-okhttp/src/test/kotlin/com/datadog/android/okhttp/trace/TracingInterceptorTest.kt‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1725,6 +1725,46 @@ internal open class TracingInterceptorTest {
17251725
)
17261726
}
17271727

1728+
@Test
1729+
fun `M set raw agent_psr W intercept() {non-DeterministicTraceSampler}`(
1730+
@FloatForgery(min = 1f, max = 100f) fakeSampleRate: Float
1731+
) {
1732+
// Given
1733+
whenever(mockTraceSampler.getSampleRate()) doReturn fakeSampleRate
1734+
whenever(mockSpan.isRootSpan) doReturn true
1735+
whenever(mockSpanContext.setSamplingPriority(any())) doReturn true
1736+
whenever(mockResolver.isFirstPartyUrl(fakeUrl.toHttpUrl())).thenReturn(true)
1737+
stubChain(mockChain)
1738+
1739+
// When
1740+
testedInterceptor.intercept(mockChain)
1741+
1742+
// Then
1743+
verify(mockSpanContext).setMetric(
1744+
eq("_dd.agent_psr"),
1745+
eq(fakeSampleRate.toDouble() / 100.0)
1746+
)
1747+
}
1748+
1749+
@Test
1750+
fun `M set zero agent_psr W intercept() {non-DeterministicTraceSampler, null sample rate}`() {
1751+
// Given
1752+
whenever(mockTraceSampler.getSampleRate()) doReturn null
1753+
whenever(mockSpan.isRootSpan) doReturn true
1754+
whenever(mockSpanContext.setSamplingPriority(any())) doReturn true
1755+
whenever(mockResolver.isFirstPartyUrl(fakeUrl.toHttpUrl())).thenReturn(true)
1756+
stubChain(mockChain)
1757+
1758+
// When
1759+
testedInterceptor.intercept(mockChain)
1760+
1761+
// Then
1762+
verify(mockSpanContext).setMetric(
1763+
eq("_dd.agent_psr"),
1764+
eq(0.0)
1765+
)
1766+
}
1767+
17281768
// endregion
17291769

17301770
// region Internal

0 commit comments

Comments
 (0)