From dcd93177a432a86b837ee3fdab80dd6e48bd7d1f Mon Sep 17 00:00:00 2001 From: Marius Constantin Date: Wed, 13 Mar 2024 13:47:05 +0100 Subject: [PATCH] RUM-3516 Provide the correct sampling priority for our Span events based on the new APM rules --- .../com/datadog/trace/core/PendingTrace.java | 1 - .../android/trace/OtelTracerProvider.kt | 25 ++- .../event/OtelDdSpanToSpanEventMapper.kt | 4 +- .../trace/OtelTracerBuilderProviderTest.kt | 177 +++++++++++++++--- .../event/OtelDdSpanToSpanEventMapperTest.kt | 4 +- .../utils/forge/CoreDDSpanForgeryFactory.kt | 2 + 6 files changed, 172 insertions(+), 41 deletions(-) diff --git a/features/dd-sdk-android-trace/src/main/java/com/datadog/trace/core/PendingTrace.java b/features/dd-sdk-android-trace/src/main/java/com/datadog/trace/core/PendingTrace.java index 53e0ed9679..6071611c48 100644 --- a/features/dd-sdk-android-trace/src/main/java/com/datadog/trace/core/PendingTrace.java +++ b/features/dd-sdk-android-trace/src/main/java/com/datadog/trace/core/PendingTrace.java @@ -491,7 +491,6 @@ public void setSamplingPriorityIfNecessary() { if (traceConfig.sampler instanceof PrioritySampler && rootSpan != null && rootSpan.context().getSamplingPriority() == PrioritySampling.UNSET) { - ((PrioritySampler) traceConfig.sampler).setSamplingPriority(rootSpan); } } diff --git a/features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/OtelTracerProvider.kt b/features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/OtelTracerProvider.kt index 70f6b6a7e9..fce85e11e8 100644 --- a/features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/OtelTracerProvider.kt +++ b/features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/OtelTracerProvider.kt @@ -95,7 +95,7 @@ class OtelTracerProvider( ) { private var tracingHeaderTypes: Set = setOf(TracingHeaderType.DATADOG, TracingHeaderType.TRACECONTEXT) - private var sampleRate: Double = DEFAULT_SAMPLE_RATE + private var sampleRate: Double? = null private var serviceName: String = "" get() { return field.ifEmpty { @@ -201,11 +201,24 @@ class OtelTracerProvider( TracerConfig.SPAN_TAGS, globalTags.map { "${it.key}:${it.value}" }.joinToString(",") ) - properties.setProperty( - TracerConfig.TRACE_SAMPLE_RATE, - (sampleRate / DEFAULT_SAMPLE_RATE).toString() - ) + // In case the sample rate is not set we should not specify it. The agent code under the hood + // will provide different sampler based on this property and also different sampling priorities used + // in the metrics + // -1 MANUAL_DROP User indicated to drop the trace via configuration (sampling rate). + // 0 AUTO_DROP Sampler indicated to drop the trace using a sampling rate provided by the Agent through + // a remote configuration. The Agent API is not used in Android so this `sampling_priority:0` will never + // be used. + // 1 AUTO_KEEP Sampler indicated to keep the trace using a sampling rate from the default configuration + // which right now is 100.0 + // (Default sampling priority value. or in our case no specified sample rate will be considered as 100) + // 2 MANUAL_KEEP User indicated to keep the trace, either manually or via configuration (sampling rate) + sampleRate?.let { + properties.setProperty( + TracerConfig.TRACE_SAMPLE_RATE, + (it / KEEP_ALL_SAMPLE_RATE_PERCENT).toString() + ) + } val propagationStyles = tracingHeaderTypes.joinToString(",") properties.setProperty(TracerConfig.PROPAGATION_STYLE_EXTRACT, propagationStyles) properties.setProperty(TracerConfig.PROPAGATION_STYLE_INJECT, propagationStyles) @@ -224,7 +237,7 @@ class OtelTracerProvider( internal const val TRACER_ALREADY_EXISTS_WARNING_MESSAGE = "Tracer for %s already exists. Returning existing instance." internal const val DEFAULT_TRACER_NAME = "android" - internal const val DEFAULT_SAMPLE_RATE = 100.0 + internal const val KEEP_ALL_SAMPLE_RATE_PERCENT = 100.0 internal const val TRACING_NOT_ENABLED_ERROR_MESSAGE = "You're trying to create an OtelTracerProvider instance, " + diff --git a/features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/internal/domain/event/OtelDdSpanToSpanEventMapper.kt b/features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/internal/domain/event/OtelDdSpanToSpanEventMapper.kt index 615dd4e8b8..48c1b17a7f 100644 --- a/features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/internal/domain/event/OtelDdSpanToSpanEventMapper.kt +++ b/features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/internal/domain/event/OtelDdSpanToSpanEventMapper.kt @@ -12,6 +12,7 @@ import com.datadog.android.core.internal.utils.toHexString import com.datadog.android.log.LogAttributes import com.datadog.android.trace.model.SpanEvent import com.datadog.trace.core.DDSpan +import com.datadog.trace.core.DDSpanContext internal class OtelDdSpanToSpanEventMapper( internal val networkInfoEnabled: Boolean @@ -52,8 +53,7 @@ internal class OtelDdSpanToSpanEventMapper( private fun resolveMetrics(event: DDSpan): SpanEvent.Metrics { val metrics = resolveMetricsFromSpanContext(event).apply { - this["_dd.agent_psr"] = 1.0f - this["_sampling_priority_v1"] = 1 + this[DDSpanContext.PRIORITY_SAMPLING_KEY] = event.samplingPriority() } return SpanEvent.Metrics( topLevel = if (event.parentId == 0L) 1 else null, diff --git a/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/OtelTracerBuilderProviderTest.kt b/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/OtelTracerBuilderProviderTest.kt index 2bf8c83207..400cbbbc37 100644 --- a/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/OtelTracerBuilderProviderTest.kt +++ b/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/OtelTracerBuilderProviderTest.kt @@ -19,8 +19,10 @@ import com.datadog.opentelemetry.trace.OtelSpanContext import com.datadog.tools.unit.getFieldValue import com.datadog.trace.api.Config import com.datadog.trace.api.config.TracerConfig +import com.datadog.trace.api.sampling.PrioritySampling import com.datadog.trace.common.writer.Writer import com.datadog.trace.core.CoreTracer +import com.datadog.trace.core.DDSpan import com.datadog.trace.core.DDSpanContext import fr.xgouchet.elmyr.Forge import fr.xgouchet.elmyr.annotation.DoubleForgery @@ -31,6 +33,7 @@ import fr.xgouchet.elmyr.junit5.ForgeConfiguration import fr.xgouchet.elmyr.junit5.ForgeExtension import org.assertj.core.api.Assertions.assertThat import org.assertj.core.api.Assertions.offset +import org.assertj.core.data.Offset import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import org.junit.jupiter.api.extension.ExtendWith @@ -72,6 +75,9 @@ internal class OtelTracerBuilderProviderTest { @StringForgery lateinit var fakeInstrumentationName: String + @StringForgery + lateinit var fakeOperationName: String + @Mock lateinit var mockTraceWriter: Writer @@ -343,36 +349,6 @@ internal class OtelTracerBuilderProviderTest { assertThat(coreTracer.partialFlushMinSpans).isEqualTo(threshold) } - @Test - fun `M use the default sample rate W creating a tracer`() { - // Given - val expectedNormalizedSampleRate = OtelTracerProvider.DEFAULT_SAMPLE_RATE / 100.0 - val tracer = testedOtelTracerProviderBuilder.build() - .tracerBuilder(fakeInstrumentationName).build() - - // When - val coreTracer: CoreTracer = tracer.getFieldValue("tracer") - - // Then - val config: Config = coreTracer.getFieldValue("initialConfig") - assertThat(config.traceSampleRate).isCloseTo(expectedNormalizedSampleRate, offset(0.005)) - } - - @Test - fun `M use the sample rate W setSampleRate`(@DoubleForgery(min = 0.0, max = 100.0) sampleRate: Double) { - // Given - val expectedNormalizedSampleRate = sampleRate / 100.0 - val tracer = testedOtelTracerProviderBuilder.setSampleRate(sampleRate).build() - .tracerBuilder(fakeInstrumentationName).build() - - // When - val coreTracer: CoreTracer = tracer.getFieldValue("tracer") - - // Then - val config: Config = coreTracer.getFieldValue("initialConfig") - assertThat(config.traceSampleRate).isCloseTo(expectedNormalizedSampleRate, offset(0.005)) - } - @Test fun `M set correct propagating style W setting tracing header types`(forge: Forge) { // Given @@ -446,4 +422,145 @@ internal class OtelTracerBuilderProviderTest { } // endregion + + // region Sampling priority + + @Test + fun `M not add a sample rate by default W creating a tracer`() { + // Given + val tracer = testedOtelTracerProviderBuilder.build() + .tracerBuilder(fakeInstrumentationName).build() + + // When + val coreTracer: CoreTracer = tracer.getFieldValue("tracer") + + // Then + val config: Config = coreTracer.getFieldValue("initialConfig") + val traceSampleRate: Double? = config.traceSampleRate + assertThat(traceSampleRate).isNull() + } + + @Test + fun `M use the sample rate W setSampleRate`(@DoubleForgery(min = 0.0, max = 100.0) sampleRate: Double) { + // Given + val expectedNormalizedSampleRate = sampleRate / 100.0 + val tracer = testedOtelTracerProviderBuilder.setSampleRate(sampleRate).build() + .tracerBuilder(fakeInstrumentationName).build() + + // When + val coreTracer: CoreTracer = tracer.getFieldValue("tracer") + + // Then + val config: Config = coreTracer.getFieldValue("initialConfig") + assertThat(config.traceSampleRate).isCloseTo(expectedNormalizedSampleRate, offset(0.005)) + } + + @Test + fun `M use user-keep priority W buildSpan { provided keep sample rate }`() { + // Given + val tracer = testedOtelTracerProviderBuilder + .setPartialFlushThreshold(1) + .setSampleRate(100.0) + .build() + .tracerBuilder(fakeInstrumentationName) + .build() + + // When + val span = tracer + .spanBuilder(fakeOperationName) + .startSpan() + val delegateSpan: DDSpan = span.getFieldValue("delegate") + delegateSpan.forceSamplingDecision() + span.end() + + // Then + val priority = delegateSpan.samplingPriority + assertThat(priority).isEqualTo(PrioritySampling.USER_KEEP.toInt()) + } + + @Test + fun `M use user-drop priority W buildSpan { provide not keep sample rate }`() { + // Given + val tracer = testedOtelTracerProviderBuilder + .setPartialFlushThreshold(1) + .setSampleRate(0.0) + .build() + .tracerBuilder(fakeInstrumentationName) + .build() + + // When + val span = tracer + .spanBuilder(fakeOperationName) + .startSpan() + val delegateSpan: DDSpan = span.getFieldValue("delegate") + delegateSpan.forceSamplingDecision() + span.end() + + // Then + val priority = delegateSpan.samplingPriority + assertThat(priority).isEqualTo(PrioritySampling.USER_DROP.toInt()) + } + + @Test + fun `M use user-keep or user-not-keep priority W buildSpan { provided random sample rate }`( + @DoubleForgery(min = 0.0, max = 100.0) sampleRate: Double, + forge: Forge + ) { + // Given + val numberOfSpans = 100 + val tracer = testedOtelTracerProviderBuilder + .setPartialFlushThreshold(1) + .setSampleRate(sampleRate) + .build() + .tracerBuilder(fakeInstrumentationName) + .build() + val normalizedSampleRate = sampleRate / 100.0 + val expectedKeptSpans = (numberOfSpans * normalizedSampleRate).toInt() + val expectedDroppedSpans = numberOfSpans - expectedKeptSpans + + // When + val spans = (0 until numberOfSpans).map { + tracer.spanBuilder(forge.anAlphabeticalString()).startSpan() + } + val delegatedSpans = spans.map { + val delegatedSpan: DDSpan = it.getFieldValue("delegate") + delegatedSpan.forceSamplingDecision() + delegatedSpan + } + spans.forEach { it.end() } + val droppedSpans = delegatedSpans.filter { it.samplingPriority == PrioritySampling.USER_DROP.toInt() } + val keptSpans = delegatedSpans.filter { it.samplingPriority == PrioritySampling.USER_KEEP.toInt() } + + // Then + assertThat(droppedSpans.size + keptSpans.size).isEqualTo(numberOfSpans) + // The sampler does not guarantee the exact number of dropped/kept spans due to the random nature + // of the sampling so we use an offset to allow a small margin of error + val offset = 10 + assertThat(droppedSpans.size).isCloseTo(expectedDroppedSpans, Offset.offset(offset)) + assertThat(keptSpans.size).isCloseTo(expectedKeptSpans, Offset.offset(offset)) + } + + @Test + fun `M use auto - keep priority W buildSpan { not provided sample rate }`() { + // Given + val tracer = testedOtelTracerProviderBuilder + .setPartialFlushThreshold(1) + .build() + .tracerBuilder(fakeInstrumentationName) + .build() + + // When + val span = tracer + .spanBuilder(fakeOperationName) + .startSpan() + val delegateSpan: DDSpan = span.getFieldValue("delegate") + delegateSpan.forceSamplingDecision() + span.end() + + // Then + val priority = delegateSpan.samplingPriority + assertThat(priority).isEqualTo(PrioritySampling.SAMPLER_KEEP.toInt()) + } + + // endregion } diff --git a/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/internal/domain/event/OtelDdSpanToSpanEventMapperTest.kt b/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/internal/domain/event/OtelDdSpanToSpanEventMapperTest.kt index 4df50869ec..453898bb0f 100644 --- a/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/internal/domain/event/OtelDdSpanToSpanEventMapperTest.kt +++ b/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/internal/domain/event/OtelDdSpanToSpanEventMapperTest.kt @@ -13,6 +13,7 @@ import com.datadog.android.trace.assertj.SpanEventAssert.Companion.assertThat import com.datadog.android.utils.forge.Configurator import com.datadog.trace.api.DD128bTraceId import com.datadog.trace.core.DDSpan +import com.datadog.trace.core.DDSpanContext import fr.xgouchet.elmyr.Forge import fr.xgouchet.elmyr.annotation.BoolForgery import fr.xgouchet.elmyr.annotation.Forgery @@ -223,8 +224,7 @@ internal class OtelDdSpanToSpanEventMapperTest { private fun DDSpan.expectedMetrics(): Map { return tags.filterValues { it is Number }.mapValues { it.value as Number }.toMutableMap().apply { - this["_dd.agent_psr"] = 1.0f - this["_sampling_priority_v1"] = 1 + this[DDSpanContext.PRIORITY_SAMPLING_KEY] = samplingPriority() } } diff --git a/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/utils/forge/CoreDDSpanForgeryFactory.kt b/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/utils/forge/CoreDDSpanForgeryFactory.kt index cfed4fd31a..c59c8909a5 100644 --- a/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/utils/forge/CoreDDSpanForgeryFactory.kt +++ b/features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/utils/forge/CoreDDSpanForgeryFactory.kt @@ -29,6 +29,7 @@ internal class CoreDDSpanForgeryFactory : ForgeryFactory { val traceId = forge.aLong(min = 1) val spanId = forge.aLong(min = 1) val parentId = forge.aLong(min = 1) + val samplingPriority = forge.anInt() val tagsAndMetrics = tags + metrics val mockSpanContext: DDSpanContext = mock { whenever(it.baggageItems).thenReturn(baggageItems) @@ -48,6 +49,7 @@ internal class CoreDDSpanForgeryFactory : ForgeryFactory { whenever(it.parentId).thenReturn(parentId) whenever(it.baggage).thenReturn(baggageItems) whenever(it.tags).thenReturn(tagsAndMetrics) + whenever(it.samplingPriority()).thenReturn(samplingPriority) } return mockDDSpan }