Continuous Profiling - stop when app goes in background#4311
Continuous Profiling - stop when app goes in background#4311stefanosiano merged 11 commits intomainfrom
Conversation
Added platform "android" in ProfileChunk envelope items
…dContinuousProfiler Added "delayed" stop of profiler, which stops the profiler after the current chunk is finished Added default span data (profiler id, thread name and thread id) to transaction root span
|
markushi
left a comment
There was a problem hiding this comment.
Looking good, I left two comments I'd like to clarified before merging.
| scopes.endSession(); | ||
| } | ||
| scopes.getOptions().getReplayController().stop(); | ||
| scopes.getOptions().getContinuousProfiler().stopAllProfiles(); |
There was a problem hiding this comment.
This means the profiler will continue to run for 30s in the background before stopping, is this what we want?
There was a problem hiding this comment.
yes, and actually stops after 30s and the last chunk is finished
There was a problem hiding this comment.
Alright, to be honest I still wouldn't do this. Once the app is in background, any executed code can easily produce a background ANR.
There was a problem hiding this comment.
true, but if we don't, the profiler could continue to run indefinitely, right?
sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java
Outdated
Show resolved
Hide resolved
markushi
left a comment
There was a problem hiding this comment.
LGTM, I left a few comments, but nothing major which should stop us from shipping this.
…-background # Conflicts: # CHANGELOG.md # sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java # sentry-android-core/src/test/java/io/sentry/android/core/AndroidContinuousProfilerTest.kt
added AndroidContinuousProfiler.close param isTerminating
Performance metrics 🚀
|
📜 Description
app going in the background now stops the continuous profiler
💡 Motivation and Context
Going to the background (after the session timer) should stop the profiler, as decided in the doc. Note that it shouldn't restart when it goes to the foreground again
💚 How did you test it?
Unit tests
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps