Skip to content

[Rum-6852] Add "Effective Sample Rate" to telemetry events#2453

Merged
satween merged 5 commits into
developfrom
tvaleev/rum-6852/effective_sample_rate
Dec 13, 2024
Merged

[Rum-6852] Add "Effective Sample Rate" to telemetry events#2453
satween merged 5 commits into
developfrom
tvaleev/rum-6852/effective_sample_rate

Conversation

@satween

@satween satween commented Dec 12, 2024

Copy link
Copy Markdown
Contributor

What does this PR do?

  • Adds support for effective sample rate field
  • Supporting unit tests for it
  • Supporting long-living events that could have two sample rates
  • Small improvements with annotations

Motivation

Main goal is to be able to see what exact value was applied through different sampling layer

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)

@codecov-commenter

codecov-commenter commented Dec 12, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 91.66667% with 6 lines in your changes missing coverage. Please review.

Project coverage is 70.38%. Comparing base (551b859) to head (6a57e0a).
Report is 6 commits behind head on develop.

Files with missing lines Patch % Lines
...ndroid/telemetry/internal/TelemetryEventHandler.kt 87.50% 0 Missing and 5 partials ⚠️
...n/kotlin/com/datadog/android/api/InternalLogger.kt 50.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #2453      +/-   ##
===========================================
+ Coverage    70.27%   70.38%   +0.11%     
===========================================
  Files          780      780              
  Lines        28823    28852      +29     
  Branches      4824     4833       +9     
===========================================
+ Hits         20255    20306      +51     
+ Misses        7226     7204      -22     
  Partials      1342     1342
Files with missing lines Coverage Δ
.../android/core/internal/logger/SdkInternalLogger.kt 92.86% <100.00%> (+6.69%) ⬆️
...oid/core/internal/metrics/MethodCalledTelemetry.kt 100.00% <100.00%> (ø)
...droid/internal/telemetry/InternalTelemetryEvent.kt 32.26% <100.00%> (+7.26%) ⬆️
...lin/com/datadog/android/rum/internal/RumFeature.kt 93.40% <ø> (+0.33%) ⬆️
...um/internal/metric/SessionEndedMetricDispatcher.kt 64.52% <100.00%> (-1.11%) ⬇️
...n/kotlin/com/datadog/android/api/InternalLogger.kt 94.12% <50.00%> (+0.37%) ⬆️
...ndroid/telemetry/internal/TelemetryEventHandler.kt 90.71% <87.50%> (+5.25%) ⬆️

... and 25 files with indirect coverage changes

@satween
satween marked this pull request as ready for review December 12, 2024 20:35
@satween
satween requested review from a team as code owners December 12, 2024 20:35
ambushwork
ambushwork reviewed Dec 13, 2024
View reviewed changes
Comment thread dd-sdk-android-core/src/main/kotlin/com/datadog/android/api/InternalLogger.kt Outdated
* @param messageBuilder the lambda building the metric message
* @param additionalProperties additional properties to add to the metric
* @param samplingRate value between 0-100 for sampling the event. Note that the sampling rate applied to this
* @param creationSampleRate value between 0-100 that was applied for long-lived metrics. This value will not be

@ambushwork ambushwork Dec 13, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand the documentation here, and the documentation doesn't explain where the word creation in the argument name comes from

@mariusc83 mariusc83 Dec 13, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I subscribe here, I am not sure why you need this and what this should be

@satween satween Dec 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, updated, hope it's more clear now

Comment thread ...k-android-core/src/main/kotlin/com/datadog/android/core/internal/logger/SdkInternalLogger.kt Outdated
Comment thread ...k-android-core/src/main/kotlin/com/datadog/android/core/internal/logger/SdkInternalLogger.kt Outdated
Comment thread ...droid-core/src/test/kotlin/com/datadog/android/core/internal/logger/SdkInternalLoggerTest.kt
Comment thread ...-core/src/test/kotlin/com/datadog/android/core/internal/metrics/MethodCalledTelemetryTest.kt Outdated
mariusc83
mariusc83 reviewed Dec 13, 2024
View reviewed changes
Comment thread dd-sdk-android-core/src/main/kotlin/com/datadog/android/api/InternalLogger.kt Outdated
Comment thread ...k-android-core/src/main/kotlin/com/datadog/android/core/internal/logger/SdkInternalLogger.kt Outdated
Comment thread ...k-android-core/src/main/kotlin/com/datadog/android/core/internal/logger/SdkInternalLogger.kt Outdated
Comment thread ...roid-internal/src/main/java/com/datadog/android/internal/telemetry/InternalTelemetryEvent.kt
mariusc83
mariusc83 requested changes Dec 13, 2024
View reviewed changes

@mariusc83 mariusc83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to remove the sessionSampleRate from effectiveSamplingRate equation.

mariusc83
mariusc83 approved these changes Dec 13, 2024
View reviewed changes

@mariusc83 mariusc83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am actually going to approve this as I do not want to block the PR. I will not be here to review it again.

@satween

satween commented Dec 13, 2024

Copy link
Copy Markdown
Contributor Author

We need to remove the sessionSampleRate from effectiveSamplingRate equation.

done

@satween
satween requested a review from ambushwork December 13, 2024 13:27
ambushwork
ambushwork approved these changes Dec 13, 2024
View reviewed changes
Comment thread dd-sdk-android-core/src/main/kotlin/com/datadog/android/api/InternalLogger.kt Outdated
* metric will be applied in addition to the global telemetry sampling rate.
* @param creationSampleRate value between 0-100. Some of the metrics like [PerformanceMetric] being sampled on the
* metric creation place and then reported with 100% probability. In such cases we need to use *creationSampleRate*
* to compute effectiveSampleRate correctly

@ambushwork ambushwork Dec 13, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you give a null default value here, it's better to explain it as well , for example what does null mean, does it mean 0 if we give null ?

@satween
satween force-pushed the tvaleev/rum-6852/effective_sample_rate branch from 35e11e3 to 492f178 Compare December 13, 2024 14:12
@satween
RUM-6852: post-review fixes: adjusting documentation,
6a57e0a 
removing sessionSample rate from effective sample rate computation,
getting rid from specific extension method
@satween
satween force-pushed the tvaleev/rum-6852/effective_sample_rate branch from 492f178 to 6a57e0a Compare December 13, 2024 14:21
@satween
satween merged commit 41f157b into develop Dec 13, 2024
@satween
satween deleted the tvaleev/rum-6852/effective_sample_rate branch December 13, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mariusc83 mariusc83 mariusc83 approved these changes
@ambushwork ambushwork ambushwork approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@satween @codecov-commenter @mariusc83 @ambushwork