Skip to content
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

Add an API to update Crash Reports from the previous run of the app #7503

Merged
merged 1 commit into from
Feb 25, 2021

Conversation

samedson
Copy link
Contributor

@samedson samedson commented Feb 10, 2021

  • This adds an API to annotate and read data from the last Crash Report if there are any custom errors or a crash in it.

Changes

Background on Processed and Prepared Reports

  • "processed" reports are ones that have been cleared for upload and have started to undergo on-device symbolication.
  • "prepared" reports have finished on-device symbolication. In the past this location was where reports sat while they waited for upload. Now that GoogleDataTransport exists, reports exist in the "prepared" folder for only a split second before they are converted to GDT's format and handed off to the GDT SDK to manage.

Semantics Changes

The semantics should match the existing checkForUnsentReports, except this PR makes a change to how "processed" and "prepared" reports are handled with unsent reports.

Semantics Change:

  • Before, if a crash was in "processed" or "prepared", but wasn't uploaded, it was considered unsent, and calling checkForUnsentReports would return true.
  • Now, processed and prepared reports are not considered unsent, and reports in those directories won't cause the unsent bool to be true. With the new API, this will result in the CrashlyticsReport being nil (the same as there not being unsent reports).

This change should be ok because:

  • Reports will only become processed or prepared when the developer has enabled automatic collection or called sendUnsentReports. Therefore, reports in these states are already considered "sent".
  • Now that Crashlytics uses GoogleDataTransport, reports do not naturally sit in prepared or processed folders unless there is a problem. Before GDT, "prepared" reports were the ones Crashlytics was waiting to upload. Now GDT manages reports waiting for upload.

The only theoretical tradeoff is what happens when a report becomes prepared or processed, but the app crashes or closes before the processed report is handed off to GDT. I'm calling these reports "In limbo".

  • Before SDK will always report there are unsent reports if any reports are stuck in "limbo", but at least the developer knows. There isn't anything the developer can do to resolve this situation, and it could impact the end user if checkForUnsentReports is used to show a crash feedback dialog.
  • Now the report won't be reported in checkForUnsentReports, but will need data collection enabled / sendUnsentReports to be sent.

}

- (void)processExistingActiveReportPath:(NSString *)path
dataCollectionToken:(FIRCLSDataCollectionToken *)dataCollectionToken
asUrgent:(BOOL)urgent {
FIRCLSInternalReport *report = [FIRCLSInternalReport reportWithPath:path];

// TODO: needsToBeSubmitted should really be called on the background queue.
if (![report needsToBeSubmitted]) {
// TODO: hasAnyEvents should really be called on the background queue.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reasoning for this not being on a background queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We humaned on this - might move to a background thread if possible

@samedson samedson merged commit e769c91 into master Feb 25, 2021
@samedson samedson deleted the report-callback-api branch February 25, 2021 16:14
ziadtamim pushed a commit to ziadtamim/firebase-ios-sdk that referenced this pull request Mar 11, 2021
@firebase firebase locked and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants