RUM-3516 Provide the correct sampling priority for our Span events based on APM new rules by mariusc83 · Pull Request #1913 · DataDog/dd-sdk-android

mariusc83 · 2024-03-13T12:49:11Z

What does this PR do?

Following the new rules in APM Java project we will provide the right sampling priority based on the APM tracer configuration.

Motivation

What inspired you to submit this pull request?

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)

xgouchet · 2024-03-14T07:28:05Z

+        val priority = ddSpanContext.samplingPriority
+        assertThat(priority).isIn(-1, 2)


Warning

This test is not testing whether we are using the sample rate passed in. We just test that when we set a sample rate, the span will have priority manual drop or manual keep.

A better test would be to create 100 spans with the tracer, and then count the number of manual keep, and make sure it's relatively close to the input sample rate.

I am not trying to test the sampling mechanism here, I am just trying to test that if you specify a sample rate you will either get -1 or 2 but the intention is not to test the sampling logic.

jonathanmos · 2024-03-14T09:04:57Z

            "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 = 100.0


Suggested change

internal const val KEEP_ALL_SAMPLE_RATE = 100.0

internal const val KEEP_ALL_SAMPLE_RATE_PERCENT = 100.0

jonathanmos · 2024-03-14T09:16:48Z

+        try {
+            pendingTraceBufferWorker.join(10000)
+        } catch (e: InterruptedException) {
+            // ignore


should we not log that this has happened? Alternatively if we keep it, I have some memory there is a convention to change the variable name to ignored in the catch block

yeah I am currently testing things here as I don't really know why my tests are failing in CI. This is currently WIP until I figure out what's the bottom of the issue.

codecov-commenter · 2024-03-14T13:38:15Z

Codecov Report

Merging #1913 (dcd9317) into feature/otel-support (f18d527) will decrease coverage by 25.95%.
Report is 2 commits behind head on feature/otel-support.
The diff coverage is 100.00%.

Additional details and impacted files

@@                    Coverage Diff                    @@
##           feature/otel-support    #1913       +/-   ##
=========================================================
- Coverage                 82.37%   56.43%   -25.95%     
=========================================================
  Files                       490      796      +306     
  Lines                     17411    29605    +12194     
  Branches                   2610     4854     +2244     
=========================================================
+ Hits                      14342    16705     +2363     
- Misses                     2348    11786     +9438     
- Partials                    721     1114      +393

Files	Coverage Δ
...main/java/com/datadog/trace/core/PendingTrace.java	`47.85% <ø> (ø)`
...in/com/datadog/android/trace/OtelTracerProvider.kt	`94.12% <100.00%> (ø)`
...ternal/domain/event/OtelDdSpanToSpanEventMapper.kt	`92.42% <100.00%> (+1.38%)`	⬆️

... and 335 files with indirect coverage changes

0xnm

lgtm, I've added some questions for the better understanding

0xnm · 2024-03-18T09:49:25Z

 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.NANOSECONDS;

+import android.util.Log;


unused import?

0xnm · 2024-03-18T09:56:31Z

+
+        // Then
+        val priority = delegateSpan.samplingPriority
+        assertThat(priority).isEqualTo(2)


It seems we can use here PrioritySampling.USER_KEEP, same for the tests below

Suggested change

assertThat(priority).isEqualTo(2)

assertThat(priority).isEqualTo(PrioritySampling.USER_KEEP)

0xnm · 2024-03-18T10:01:51Z

+    }
+
+    @Test
+    fun `M use keep priority W buildSpan { not provided sample rate }`() {


Suggested change

fun `M use keep priority W buildSpan { not provided sample rate }`() {

fun `M use auto-keep priority W buildSpan { not provided sample rate }`() {

0xnm · 2024-03-20T14:00:10Z

+
+        // Then
+        val priority = delegateSpan.samplingPriority
+        assertThat(priority).isEqualTo(PrioritySampling.USER_KEEP.toInt())


I believe it should be PrioritySampling.USER_DROP here by looking on the name of the test

0xnm · 2024-03-20T14:00:54Z

+    }
+
+    @Test
+    fun `M use auto - keep priority W buildSpan { not provided sample rate }`() {


nit

Suggested change

fun `M use auto - keep priority W buildSpan { not provided sample rate }`() {

fun `M use auto-keep priority W buildSpan { not provided sample rate }`() {

xgouchet · 2024-03-20T15:27:10Z

+        val priority = delegateSpan.samplingPriority
+        assertThat(priority).isIn(PrioritySampling.USER_DROP.toInt(), PrioritySampling.USER_KEEP.toInt())


Caution
This part still doesn't test that the appropriate sample rate is being used… the fact that a specific field is set on the core tracer (as tested line 445) doesn't mean it's the one that will be applied. Since we're testing the actual priority set on the spans, we should really try creating 100 spans and measure the effective sampling rate and compare that to the expected one (with some margin of error)

…sed on the new APM rules

xgouchet · 2024-03-21T11:00:20Z

+        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))


mariusc83 self-assigned this Mar 13, 2024

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch 2 times, most recently from 64cb324 to ef6b729 Compare March 13, 2024 12:54

Base automatically changed from mconstantin/rum-3294/bundle-dd-trace-core-into-dd-sdk-trace to feature/otel-support March 13, 2024 12:55

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch from ef6b729 to a9433a0 Compare March 13, 2024 12:57

mariusc83 marked this pull request as ready for review March 13, 2024 12:57

mariusc83 requested review from a team as code owners March 13, 2024 12:57

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch 8 times, most recently from 5992b52 to 8924351 Compare March 13, 2024 16:39

xgouchet suggested changes Mar 14, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch 2 times, most recently from 3669e9b to 8f02a06 Compare March 14, 2024 08:47

jonathanmos reviewed Mar 14, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch 2 times, most recently from ff10d25 to 9a14bf3 Compare March 14, 2024 13:16

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch 2 times, most recently from cfeaed3 to 166d20b Compare March 14, 2024 14:41

mariusc83 requested review from jonathanmos and xgouchet March 14, 2024 14:54

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch from 166d20b to e546eed Compare March 15, 2024 08:08

0xnm reviewed Mar 18, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch from e546eed to 8e26066 Compare March 20, 2024 13:48

0xnm reviewed Mar 20, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch from 8e26066 to 858200e Compare March 20, 2024 14:46

0xnm approved these changes Mar 20, 2024

View reviewed changes

mariusc83 requested a review from 0xnm March 20, 2024 15:10

0xnm previously approved these changes Mar 20, 2024

View reviewed changes

xgouchet suggested changes Mar 20, 2024

View reviewed changes

RUM-3516 Provide the correct sampling priority for our Span events ba…

dcd9317

…sed on the new APM rules

mariusc83 dismissed 0xnm’s stale review via dcd9317 March 21, 2024 09:59

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch from 858200e to dcd9317 Compare March 21, 2024 09:59

mariusc83 requested review from 0xnm and xgouchet March 21, 2024 10:01

0xnm approved these changes Mar 21, 2024

View reviewed changes

xgouchet approved these changes Mar 21, 2024

View reviewed changes

mariusc83 merged commit 50e49ba into feature/otel-support Mar 21, 2024

mariusc83 deleted the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch March 21, 2024 11:09

xgouchet added this to the 2.11.x milestone Jul 31, 2024

		val priority = ddSpanContext.samplingPriority
		assertThat(priority).isIn(-1, 2)

	internal const val KEEP_ALL_SAMPLE_RATE = 100.0
	internal const val KEEP_ALL_SAMPLE_RATE_PERCENT = 100.0

	assertThat(priority).isEqualTo(2)
	assertThat(priority).isEqualTo(PrioritySampling.USER_KEEP)

	fun `M use keep priority W buildSpan { not provided sample rate }`() {
	fun `M use auto-keep priority W buildSpan { not provided sample rate }`() {

	fun `M use auto - keep priority W buildSpan { not provided sample rate }`() {
	fun `M use auto-keep priority W buildSpan { not provided sample rate }`() {

		val priority = delegateSpan.samplingPriority
		assertThat(priority).isIn(PrioritySampling.USER_DROP.toInt(), PrioritySampling.USER_KEEP.toInt())

Uh oh!

Conversation

mariusc83 commented Mar 13, 2024 • 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!

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!

codecov-commenter commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

5 participants

mariusc83 commented Mar 13, 2024 •

edited

Loading

codecov-commenter commented Mar 14, 2024 •

edited

Loading