RUM-5387: Override Metrics telemetry sample rate#3022
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3022 +/- ##
===========================================
+ Coverage 71.17% 71.40% +0.23%
===========================================
Files 859 859
Lines 31444 31455 +11
Branches 5302 5306 +4
===========================================
+ Hits 22378 22459 +81
+ Misses 7555 7517 -38
+ Partials 1511 1479 -32
🚀 New features to boost your workflow:
|
|
🎯 Code Coverage 🔗 Commit SHA: a412359 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
ambushwork
left a comment
There was a problem hiding this comment.
Well done, just a few comments in the unit test
| @FloatForgery(min = 0.0f, max = 100.0f) fakeSampleRate: Float | ||
| ) { | ||
| // Given | ||
| val bypassSampleRate = 100.0f |
There was a problem hiding this comment.
This can be faked as a random value by forge as well I think
There was a problem hiding this comment.
The idea of using 100.0f is to guarantee that the metric is always sent. Having a forged rate will make the tests flaky.
| forge: Forge | ||
| ) { | ||
| // Given | ||
| val hardcodedSampleRate = 100.0f |
There was a problem hiding this comment.
same as above this can be a random value
There was a problem hiding this comment.
Please read my comment above.
| } | ||
|
|
||
| @Test | ||
| fun `M not send metric W logMetric() {bypass rate is 0}`( |
There was a problem hiding this comment.
If I don't miss anything, there should be a test like M send metric W logMetric() {bypass rate is 100} ?
| fun setMetricTelemetrySampleRateBypass( | ||
| @FloatRange(from = 0.0, to = 100.0) sampleRate: Float | ||
| ) { | ||
| (sdkCore as? DatadogCore)?.coreFeature?.metricTelemetrySampleRateBypass = sampleRate |
There was a problem hiding this comment.
I'm not quite sure it is a safe thing to do, given that here we apply the same rate for all the metrics logged and right now the rate used for metrics call vary from 0.75f to 100f, but at worst case Shopist shouldn't bring a huge amount of data.
Also by setting it to non-100 we increase the rate for some metrics and decrease for some others.
Although I would question if we still need this change.
There was a problem hiding this comment.
It's true that some metrics will go from a sample rate of 0.75f to 100f, representing a 133x increase. I estimated that, in the Shopist Android app, there will be a 40x overall increase throughout all the metrics. Still, this will be a very small number when taking into account the intake from all the customers.
There was a problem hiding this comment.
Maybe a better API would be a sampling callback which can decide if particular metric can be sampled or not. And if decision is not made by this callback, then default sampling mechanism should be applied.
This would allow a granular control, because with the current change we apply the same rate for all metrics, meaning some will go down, some will go up.
But this API is not blocking for me.
| fun setMetricTelemetrySampleRateBypass( | ||
| @FloatRange(from = 0.0, to = 100.0) sampleRate: Float | ||
| ) { | ||
| (sdkCore as? DatadogCore)?.coreFeature?.metricTelemetrySampleRateBypass = sampleRate |
There was a problem hiding this comment.
Maybe a better API would be a sampling callback which can decide if particular metric can be sampled or not. And if decision is not made by this callback, then default sampling mechanism should be applied.
This would allow a granular control, because with the current change we apply the same rate for all metrics, meaning some will go down, some will go up.
But this API is not blocking for me.
What does this PR do?
Creates a
metricTelemetrySampleRateBypassfield inCoreFeatureand respective internal API setter, that will bypass any sample rates previously set throughout the codebase for metrics. Setting this andtelemetrySampleRateto100%will allow the Shopist app to send all metrics.Motivation
Shopist metric telemetry was sampled the same way as our customers (low sample rate). We want to be able to customize this in order to have more events for dogfooding.
Review checklist (to be filled by reviewers)