-
-
Notifications
You must be signed in to change notification settings - Fork 455
Flush logs on crash #4684
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
base: main
Are you sure you want to change the base?
Flush logs on crash #4684
Conversation
// if event is backfillable or cached we don't need to flush the logs, because it's an event | ||
// from the past. Otherwise we need to flush the logs to ensure they are sent on crash | ||
if (event != null && !isBackfillable && !isCached && event.isCrashed()) { | ||
loggerBatchProcessor.flush(options.getFlushTimeoutMillis()); |
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.
I'm not sure on this.
In theory we could wait for flushTimeoutMillis
twice, once here and once to flush the crash.
In practice I expect this method to be instantaneous in Android, as it doesn't wait for them to be sent, and there shouldn't be many logs on Android anyway
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.
IMHO crashes have the highest priority, so I would avoid any extra work/wait which could cause them to be not sent. => Just flush the logs to disk with a minimal timeout.
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.
technically, the event is captured first, so it will be sent before the logs anyway
but yeah, let's put a minimal timeout
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.
I moved the capture of replay and logs after the crash event one.
But now we are waiting only for the crash to be flushed to disk, with the risk of losing replay and logs (except for those 500ms on logs)
Is it fine this way?
@romtsn wdyt?
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ee747ae | 382.73 ms | 435.41 ms | 52.68 ms |
ee747ae | 415.92 ms | 470.15 ms | 54.23 ms |
ee747ae | 400.46 ms | 423.61 ms | 23.15 ms |
ee747ae | 554.98 ms | 611.50 ms | 56.52 ms |
ee747ae | 358.21 ms | 389.41 ms | 31.20 ms |
3699cd5 | 423.60 ms | 495.52 ms | 71.92 ms |
85d7417 | 347.21 ms | 394.35 ms | 47.15 ms |
7314dbe | 437.83 ms | 505.64 ms | 67.81 ms |
ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
ee747ae | 374.71 ms | 455.18 ms | 80.47 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
3699cd5 | 1.58 MiB | 2.10 MiB | 533.45 KiB |
85d7417 | 1.58 MiB | 2.10 MiB | 533.44 KiB |
7314dbe | 1.58 MiB | 2.10 MiB | 533.45 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
moved replay capture after sending the crash
moved replay capture after sending the crash
📜 Description
When a crash occurs, logs are flushed with a 500 timeout millis
Moved replay capture after sending the crash
💡 Motivation and Context
We ensure to store logs to disk (or even send them) instead of losing them due to being only in memory when a crash occurs
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps