RUM-13627: Add sample rate for App launch profiling#3109
Conversation
a57c937 to
d7e1cc1
Compare
|
🎯 Code Coverage 🔗 Commit SHA: df55a77 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/perfetto-profiling #3109 +/- ##
==============================================================
- Coverage 71.52% 70.84% -0.68%
==============================================================
Files 894 906 +12
Lines 32838 33347 +509
Branches 5516 5586 +70
==============================================================
+ Hits 23486 23622 +136
- Misses 7800 8167 +367
- Partials 1552 1558 +6
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7e1cc1214
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| @RequiresApi(Build.VERSION_CODES.VANILLA_ICE_CREAM) | ||
| private fun sampleProfiling(context: Context, instanceNames: Set<String>) { | ||
| val sampleRate = ProfilingStorage.getSampleRate(context) |
There was a problem hiding this comment.
Storing sample rate disconnected from the SDK instance it set creates the following problem: suppose there is instance A with sample rate 10%, and instance B with sample rate 40%. Instance B is initialized after instance A, so the sample rate recorded will be 40%.
It means that instance A will have 40% sample rate, increasing the billing and exploding the profile volume for this org.
There was a problem hiding this comment.
I updated the logic of persisting the sample rate, now we set sample rate of configuration only if the sample rate is absent in sp or the old value is bigger than the new value.
There was a problem hiding this comment.
But this new logic also means if user wants to set sampling rate higher, nothing will be done, isn't it?
There was a problem hiding this comment.
Sample rate is also an one-time flag, once it's consumed, it will be removed, see here. then user can set another sample rate, lower or higher. one thing may miss here is that, if the session is sampled out, then set a lower sample rate will be ignored
There was a problem hiding this comment.
I updated the logic when the profiling is sampled out, the sample rate will be removed
|
|
||
| companion object { | ||
|
|
||
| private const val DEFAULT_SAMPLE_RATE = 100f |
There was a problem hiding this comment.
I don't think it should be 100% by default.
There was a problem hiding this comment.
For me 100f means let rate limiter handle the sampling, which is kind of OK, what value do you suggest?
b8494ea to
1b81f8d
Compare
1b81f8d to
e699abe
Compare
e699abe to
df55a77
Compare
3 participants
What does this PR do?
A brief description of the change being made with this pull request.
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)