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

Yosemite: Log Fatal Errors to Sentry #2867

Merged
merged 4 commits into from
Sep 25, 2020

Conversation

shiki
Copy link
Member

@shiki shiki commented Sep 23, 2020

This is supposed to help with diagnosing #2858. We should also do the same for Storage (#2364) and Networking (#2372).

Changes

I added a new function, logErrorAndExit() which uses DDLogError and fatalError internally. The DDLogError will be sent to the Encrypted Logs. The fatalError() will be sent to Sentry with just “Fatal error” like before. But now, the underlying message will be accessible in the Encrypted Logs. Read more about Encrypted Logging at paaHJt-1ms-p2.

internal func logErrorAndExit(_ message: String, file: StaticString = #file, line: UInt = #line) -> Never {
DDLogError(message)
fatalError(message, file: file, line: line)
}

Testing

  1. Use the WooCommerce Alpha scheme.

  2. Turn the force-crash-logging launch argument on

  3. Add a breakpoint on this line of the Tracks EventLogging+Sentry.swift file.

  4. Change this line of the OrderStatsV4Internal+Date file to this so that it will crash later:

        guard let date = createDateFormatter(timeZone: timeZone).date(from: "abc") else {
  5. Run and install the app on the simulator.

  6. Log in. The app will crash.

  7. Stop debugging.

  8. Open the app on the simulator again. Not from Xcode. Open it a few times. This is to make sure that the crash will be queued.

  9. Revert the change in Step 4.

  10. Run the app from Xcode again.

  11. The app should stop in the breakpoint in Step 3.

  12. Capture and save the value of logFile.uuid.

  13. Let the app finish running so it will update the crash to Sentry.

  14. Head to the Encrypted Logs Console.

  15. Enter the UUID you captured in step 12 and click on View.

  16. Confirm that you see the error message like this:

Submitter Checklist

  • If it's feasible, I have added unit tests.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@shiki shiki added category: architecture Related to architecture such as the database, FluxC, Networking, Core Data, etc. feature: stats Related to stats, including Top Performers. labels Sep 23, 2020
@shiki shiki added this to the 5.2 milestone Sep 23, 2020
@shiki shiki marked this pull request as ready for review September 23, 2020 18:17
@shiki shiki requested a review from a team September 23, 2020 18:17
Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

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

Just an initial question: I think it'd be nice for the message to show in Sentry directly, without having to find it from Encrypted Logs Console for each crash event. Not sure if there's a way to log the crash on Sentry without having to call fatalError to exit the app which creates a duplicate crash on Sentry?

@shiki
Copy link
Member Author

shiki commented Sep 24, 2020

@jaclync My concern with that is we would end up with different error messages like the CoreDataManager Cannot Backup Store errors which causes Sentry to consider them as separate crashes.

But it looks like this fatalError issue is going to be fixed in Sentry soon: getsentry/sentry-cocoa#662.

This logErrorAndExit may just be a temporary fix that we'll delete later. What do you think about keeping this for now until it's resolved in Sentry? Or would you still prefer to have duplicate issues, one with no message, and the other with the message? Looks like that what this person did: getsentry/sentry-cocoa#662 (comment).

@jaclync
Copy link
Contributor

jaclync commented Sep 25, 2020

What do you think about keeping this for now until it's resolved in Sentry? Or would you still prefer to have duplicate issues, one with no message, and the other with the message? Looks like that what this person did: getsentry/sentry-cocoa#662 (comment).

Keeping this logErrorAndExit sounds great! It doesn't look like there's a timeline for the Sentry fix, and I didn't really like seeing two entries of the same crash on Sentry (it's confusing for anyone not familiar with the crash like for CTrO from the Android team). Testing now!

Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

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

Nice work! :shipit:

Code: ✅ (I was wondering where -force-crash-logging comes from, and found it by searching in the code base)
Testing: ✅ (I also generated an installable build on a testing branch and got to see the event on Sentry and log in Encrypted logs console as well!)

@shiki
Copy link
Member Author

shiki commented Sep 25, 2020

Thanks, Jaclyn!

@shiki shiki merged commit 879c7de into develop Sep 25, 2020
@shiki shiki deleted the issue/2858-add-logging-to-orderstats-crash branch September 25, 2020 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: architecture Related to architecture such as the database, FluxC, Networking, Core Data, etc. feature: stats Related to stats, including Top Performers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants