-
Notifications
You must be signed in to change notification settings - Fork 205
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
Send minimal error report if cached file is corrupted/empty #500
Conversation
Sends a report to bugsnag whenever it's not possible to deliver an error, ensuring that the user receives at least some information. This new report will not be cached if delivery fails, to prevent any serialisation bugs from looping and creating multiple reports. To support this change, we now always attempt serialisation of the cached error. This removes the need for the Report class to have a file, which has an added benefit of allowing us to specify Report#error as non-null.
9964144
to
43dcccb
Compare
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.
Maze tests LGTM 👍
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.
The implementation of this seems a little different than I imagined from our discussions.
I'm confused about the choice to send an "empty" report, rather than sending a report that communicates exactly what happened. I think it will be really confusing for users, with no clue as to why it was there and why it doesn't have any info.
I was imagining something like this:
Attaching any data we have to "Stored Report" (or some other aptly named tab).
Are there any reasons not to pursue this kind of thing?
Note to self: we should probably truncate the error class beyond a certain size to prevent the filename from being too large |
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.
Implementation looks good to me, from reading the code. A few comments that I think will require answers rather than changes.
I'd like to understand how to edit the data on disk so I test out the functionality, unless this is going to UAT?
Thanks for the review! The best way to edit files on an emulator/device would be to set the device into airplane mode so that the crash is definitely stored, then use the Device File Explorer to edit the file before relaunching the app. The file will be stored under `data/data/com.bugsnag.android.example/cache/bugsnag-errors, and you should be able to edit it like a regular file. I'll have a read through the other comments and address them inline. |
I've addressed the remaining comments and also truncated the error class to a max of 40 chars - this is now ready for re-review. |
Goal
If a report cannot be delivered due to delivery failure, it is cached on disk. If this cached report cannot be deserialised, or is only partially written, e.g. due to lack of disk space, then currently no bugsnag report will be sent.
This changeset alters bugsnag-android to send a minimal error report for cached JVM exceptions only, by encoding basic error information in the filename.
Changeset
ErrorStore
to pass aDelegate
that is invoked whenever a cached file cannot be deserialisedDelegate
inClient
by sending a report withDeliveryStyle.NO_CACHE
(this prevents minimal errors from generating other minimal errors if delivery fails and there's a serialisation bug in the notifier)Error
to includeincomplete
field that is set when a report is incompletebeforeSend
callback was set; files are now always deserialised, which negates the need forReport#errorFile
ErrorStore
which encode and decode basic information about the error from the filenameTests