-
-
Notifications
You must be signed in to change notification settings - Fork 435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce flush timeout to 4s on Android to avoid ANRs #2858
Conversation
|
Performance metrics 🚀
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## feat/7.0.0 #2858 +/- ##
================================================
+ Coverage 81.26% 81.31% +0.04%
- Complexity 4560 4564 +4
================================================
Files 350 351 +1
Lines 16866 16867 +1
Branches 2272 2272
================================================
+ Hits 13706 13715 +9
+ Misses 2219 2211 -8
Partials 941 941
☔ View full report in Codecov by Sentry. |
@markushi there's one place where I'd like to keep the default 15s for flush timeout and it's this sentry-java/sentry/src/main/java/io/sentry/cache/EnvelopeCache.java Lines 430 to 438 in f60279b
when we're waiting for the previous unfinished session to be flushed to disk. Would it be possible to keep the 15s value there (we could just have a constant actually)? |
@@ -429,7 +431,9 @@ public void discard(final @NotNull SentryEnvelope envelope) { | |||
/** Awaits until the previous session (if any) is flushed to its own file. */ | |||
public boolean waitPreviousSessionFlush() { | |||
try { | |||
return previousSessionLatch.await(options.getFlushTimeoutMillis(), TimeUnit.MILLISECONDS); | |||
// use fixed timeout instead of configurable options.getFlushTimeoutMillis() to ensure there's | |||
// enough time to flush the session to disk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romtsn if you've better reasoning here, please adapt the comment 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it's good!
📜 Description
Reduce flush timeout to 4s on Android to avoid ANRs
💡 Motivation and Context
Fixes #2732
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps