Add profilesSampleRate and profileSampler for Android sdk #2184
Conversation
…to decide sampling) deprecated SentryOptions.setProfilingEnabled() updated internal code and unit tests to change from SentryOptions.setProfilingEnabled() to SentryOptions.setProfilesSampleRate()
updated tests to use setProfilesSampleRate instead of setProfilingEnabled TracesSamplingDecision now includes profiles sampling decision
Codecov Report
@@ Coverage Diff @@
## main #2184 +/- ##
=========================================
Coverage 80.90% 80.90%
- Complexity 3313 3343 +30
=========================================
Files 236 236
Lines 12155 12200 +45
Branches 1616 1624 +8
=========================================
+ Hits 9834 9871 +37
- Misses 1725 1729 +4
- Partials 596 600 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
updated tests to use setProfilesSampleRate instead of setProfilingEnabled TracesSamplingDecision now includes profiles sampling decision
| @@ -1500,16 +1511,65 @@ public void setTransactionProfiler(final @Nullable ITransactionProfiler transact | |||
| * @return if profiling is enabled for transactions. | |||
| */ | |||
| public boolean isProfilingEnabled() { | |||
| return profilingEnabled; | |||
| return (getProfilesSampleRate() != null && getProfilesSampleRate() > 0) | |||
There was a problem hiding this comment.
We don't check > 0 for isTracingEnabled at least.
Do we need it here?
There was a problem hiding this comment.
well, if the sample rate is 0 and there's no sampler set, we'd still initialize the profile, even if in reality it will never be used
There was a problem hiding this comment.
Yeah for profiling maybe it's better to keep with this check.
For transactions, we actually need it, because 0 is enabled and it propagates transactions that come from the backend and inherits the sampling decision from the parent.
If sampling should be the case, then removing the > 0 check makes sense.
There was a problem hiding this comment.
the logic of profiles sampling mimics the transaction sampling, including the parent decision. But we don't get profiles from the backend. Should we consider the case for consistency? Initializing the profiler is quick anyway, and the option already says that null should be used to disable it
There was a problem hiding this comment.
That's a business decision, so I can't tell you what makes more sense for profiling.
| @@ -15,7 +15,7 @@ public static boolean isValidSampleRate(@Nullable Double sampleRate, boolean all | |||
| return allowNull; | |||
| } | |||
|
|
|||
| return !(sampleRate.isNaN() || (sampleRate > 1.0 || sampleRate <= 0.0)); | |||
| return !(sampleRate.isNaN() || sampleRate > 1.0 || sampleRate <= 0.0); | |||
There was a problem hiding this comment.
Apparently a new private method that takes a Double and do this check would avoid code duplication, See this method and isValidTracesSampleRate.
There was a problem hiding this comment.
may i rename isValidTracesSampleRate to isValidTracesOrProfilesSampleRate and use just one function, as they do basically the same thing?
There was a problem hiding this comment.
isValidSampleRate should be enough IMO, as long as both functions do the same.
There was a problem hiding this comment.
yeah, but isValidSampleRate already exists and does the same check, expect the rate cannot be 0.0
added some final keyword
added profiling activity to the sample app, with possibility to set time and number of background threads to get an idea of size of: profiling trace file, envelope item and compressed envelope item enabled profiling in the sample app
📜 Description
I added the
profilesSampleRateandprofileSampleroptions for the Android sdk.The
profilingEnabledoptions has been deprecated, and its usage changed to "setProfilesSampleRate(1.0 or null)". Also, it is overridden by theprofilesSampleRateoption.💡 Motivation and Context
We are reaching the profiling open beta stage.
We want to provide users a way to reduce the number of profiles sent to Sentry, in the same way we do with transactions.
To be consistent with transactions, we will provide the
profilesSampleRateandprofileSampleroptions.💚 How did you test it?
I added some unit tests
📝 Checklist
🔮 Next steps