-
Notifications
You must be signed in to change notification settings - Fork 86
RUM-15138: Rebase trace sample rate against RUM session sample rate for correlated cross-product sampling #3342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
eb5c382
eee9e76
6be5ce2
3597376
6f27b9a
3b4b36b
b7272e5
a985cb8
1dfbfca
6f012e4
eb2e1e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| /* | ||
| * Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0. | ||
| * This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
| * Copyright 2016-Present Datadog, Inc. | ||
| */ | ||
|
|
||
| package com.datadog.android.internal.sampling | ||
|
|
||
| private const val SAMPLE_ALL_RATE: Float = 100f | ||
| private const val SAMPLER_HASHER: ULong = 1111111111111111111u | ||
| private const val MAX_ID: ULong = 0xFFFFFFFFFFFFFFFFUL | ||
|
|
||
| /** | ||
| * Computes a deterministic sampling decision based on the given sample rate and identifier. | ||
| * | ||
| * @param sampleRate the sample rate in the range [0, 100]. | ||
| * @param id a stable numerical identifier derived from the item being sampled. | ||
| * @return true if the item should be sampled, false otherwise. | ||
| */ | ||
| fun computeSamplingDecision(sampleRate: Float, id: ULong): Boolean { | ||
| return when { | ||
| sampleRate >= SAMPLE_ALL_RATE -> true | ||
| sampleRate <= 0f -> false | ||
| else -> { | ||
| val hash = id * SAMPLER_HASHER | ||
| val threshold = (MAX_ID.toDouble() * sampleRate / SAMPLE_ALL_RATE).toULong() | ||
| hash < threshold | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /* | ||
| * Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0. | ||
| * This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
| * Copyright 2016-Present Datadog, Inc. | ||
| */ | ||
|
|
||
| package com.datadog.android.internal.sampling | ||
|
|
||
| import fr.xgouchet.elmyr.annotation.FloatForgery | ||
| import fr.xgouchet.elmyr.annotation.LongForgery | ||
| import fr.xgouchet.elmyr.junit5.ForgeExtension | ||
| import org.assertj.core.api.Assertions.assertThat | ||
| import org.junit.jupiter.api.RepeatedTest | ||
| import org.junit.jupiter.api.Test | ||
| import org.junit.jupiter.api.extension.ExtendWith | ||
| import org.junit.jupiter.api.extension.Extensions | ||
| import org.mockito.junit.jupiter.MockitoExtension | ||
| import org.mockito.junit.jupiter.MockitoSettings | ||
| import org.mockito.quality.Strictness | ||
|
|
||
| @Extensions( | ||
| ExtendWith(MockitoExtension::class), | ||
| ExtendWith(ForgeExtension::class) | ||
| ) | ||
| @MockitoSettings(strictness = Strictness.LENIENT) | ||
| internal class DeterministicSamplingTest { | ||
|
|
||
| @RepeatedTest(32) | ||
| fun `M always return true W computeSamplingDecision() {sampleRate is 100}`( | ||
| @LongForgery fakeId: Long | ||
| ) { | ||
| assertThat(computeSamplingDecision(100f, fakeId.toULong())).isTrue() | ||
| } | ||
|
|
||
| @RepeatedTest(32) | ||
| fun `M always return true W computeSamplingDecision() {sampleRate above 100}`( | ||
| @FloatForgery(min = 100.01f, max = 200f) fakeSampleRate: Float, | ||
| @LongForgery fakeId: Long | ||
| ) { | ||
| assertThat(computeSamplingDecision(fakeSampleRate, fakeId.toULong())).isTrue() | ||
| } | ||
|
|
||
| @RepeatedTest(32) | ||
| fun `M always return false W computeSamplingDecision() {sampleRate is 0}`( | ||
| @LongForgery fakeId: Long | ||
| ) { | ||
| assertThat(computeSamplingDecision(0f, fakeId.toULong())).isFalse() | ||
| } | ||
|
|
||
| @RepeatedTest(32) | ||
| fun `M always return false W computeSamplingDecision() {sampleRate below 0}`( | ||
| @FloatForgery(min = -100f, max = -0.01f) fakeSampleRate: Float, | ||
| @LongForgery fakeId: Long | ||
| ) { | ||
| assertThat(computeSamplingDecision(fakeSampleRate, fakeId.toULong())).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun `M return deterministic result W computeSamplingDecision() {same id same rate}`( | ||
| @FloatForgery(min = 1f, max = 99f) fakeSampleRate: Float, | ||
| @LongForgery fakeId: Long | ||
| ) { | ||
| val first = computeSamplingDecision(fakeSampleRate, fakeId.toULong()) | ||
| val second = computeSamplingDecision(fakeSampleRate, fakeId.toULong()) | ||
| assertThat(first).isEqualTo(second) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,8 @@ internal data class RumContext( | |
| val syntheticsResultId: String? = null, | ||
| val viewTimestamp: Long = 0L, | ||
| val viewTimestampOffset: Long = 0L, | ||
| val hasReplay: Boolean = false | ||
| val hasReplay: Boolean = false, | ||
| val sessionSampleRate: Float = FULL_SESSION_SAMPLE_RATE | ||
| ) { | ||
|
|
||
| fun toMap(): Map<String, Any?> { | ||
|
|
@@ -44,7 +45,8 @@ internal data class RumContext( | |
| SYNTHETICS_RESULT_ID to syntheticsResultId, | ||
| VIEW_TIMESTAMP to viewTimestamp, | ||
| HAS_REPLAY to hasReplay, | ||
| VIEW_TIMESTAMP_OFFSET to viewTimestampOffset | ||
| VIEW_TIMESTAMP_OFFSET to viewTimestampOffset, | ||
| SESSION_SAMPLE_RATE to sessionSampleRate | ||
| ) | ||
| } | ||
|
|
||
|
|
@@ -68,6 +70,8 @@ internal data class RumContext( | |
| const val HAS_REPLAY = "view_has_replay" | ||
| const val VIEW_TIMESTAMP = "view_timestamp" | ||
| const val VIEW_TIMESTAMP_OFFSET = "view_timestamp_offset" | ||
| const val SESSION_SAMPLE_RATE = "session_sample_rate" | ||
| const val FULL_SESSION_SAMPLE_RATE: Float = 100f | ||
|
|
||
| fun fromFeatureContext(featureContext: Map<String, Any?>): RumContext { | ||
| val applicationId = featureContext[APPLICATION_ID] as? String | ||
|
|
@@ -89,6 +93,8 @@ internal data class RumContext( | |
| val hasReplay = featureContext[HAS_REPLAY] as? Boolean ?: false | ||
| val viewTimestamp = featureContext[VIEW_TIMESTAMP] as? Long ?: 0L | ||
| val viewTimestampOffset = featureContext[VIEW_TIMESTAMP_OFFSET] as? Long ?: 0L | ||
| val sessionSampleRate = (featureContext[SESSION_SAMPLE_RATE] as? Number) | ||
| ?.toFloat() ?: FULL_SESSION_SAMPLE_RATE | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just curious: is there a motivation behind using 100% as a fallback, and not 0%?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main reason was that if we use 0% as a fallback, the rebased sampling would drop everything, so it seemed safer to me. However, now that you’ve mentioned it, I’m not sure if this is what we expect. |
||
|
|
||
| return RumContext( | ||
| applicationId = applicationId ?: NULL_UUID, | ||
|
|
@@ -105,7 +111,8 @@ internal data class RumContext( | |
| syntheticsResultId = syntheticsResultId, | ||
| viewTimestamp = viewTimestamp, | ||
| viewTimestampOffset = viewTimestampOffset, | ||
| hasReplay = hasReplay | ||
| hasReplay = hasReplay, | ||
| sessionSampleRate = sessionSampleRate | ||
| ) | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you moved this code out of the
DeterministicSampler? It's seems like only descendants of it are using it...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can see the discussion: #3342 (comment)