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

Crash report cohort ID support for iOS #3692

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jdjackson
Copy link
Contributor

@jdjackson jdjackson commented Dec 6, 2024

Task/Issue URL: https://app.asana.com/0/1208592102886666/1208759541597499/f
Tech Design URL: https://app.asana.com/0/1208592102886666/1208660326715650/f

Description:
DO NOT MERGE - this is a draft for input, not ready to go live yet.

iOS client support for CRCID send/receive (primarily supported in BSK, with changes under review in BSK #1116). This is pretty straightforward, just conforming to CrashCollection’s new init signature, and clearing CRCIDs when the user opts out of crash reporting. BSK handles everything else.

Steps to test this PR:
Note: Must be tested on a physical device, as the simulator does not produce crash logs (and thus doesn’t find and upload them either).

To cause and report a crash:

  1. Launch the app and force a crash, which can be done from Settings → All Debug Options → Crash (fatal error) or similar. Note that Crash (CPU/Memory) does not appear to produce a crash log, and thus won’t trigger crash uploading.
  2. Launch the app again (easiest with a debugger)
  3. For the first crash of an app install: You will be prompted to opt in or out of crash reporting when the app is launched. Opt in and watch logs for “crcid” and you should see logs from CrashReportSender:56, and CrashCollection:95-109.
  4. On subsequent crashes, when opted in, you should see statements confirming the received crcid was sent, and that the server returned either the same matching one, or a new one (in which case the new one should be stored and used on subsequent crash reports)

To test clearing of the crcid when opting out:

  1. Navigate to Settings → About and switch “Send Crash Reports” off, then back on again (this step should clear the crcid)
  2. Follow steps from “To cause and report a crash” above, and confirm that the crash is submitted without an initial crcid, and that the server assigns one and it is stored (causing and uploading a second crash should confirm this new value is used on send).

Definition of Done (Internal Only):

Copy Testing: N/A

  • Use of correct apostrophes in new copy, ie rather than

Orientation Testing: N/A

  • Portrait
  • Landscape

Device Testing:

  • iPhone SE (1st Gen)
  • iPhone 8
  • iPhone X
  • iPhone 14 Pro
  • iPhone 15 Pro
  • iPad

OS Testing:

  • iOS 15
  • iOS 16
  • iOS 17
  • iOS 18

Theme Testing: N/A

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

Remove app-level CRCID user setting, and prepare for clearing CRCID owned by BSK (BSK changes still to come).
Oops, clearing CRCID needs to be done through CrashCollection, which now supports this properly.
Support for sending error pixels when crash report submission fails or a CRCID is not returned by the server when expected.
Comment on lines 508 to 509
case crashReportCRCIDMissing // crashreporting_crcid-missing
case crashReportingSubmissionFailed // crashreporting_submission-failed
Copy link
Contributor

Choose a reason for hiding this comment

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

We can omit the comments here for consistency, since the value below will contain the raw name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this, it some leftover note taking from when I was working it all out :) I’ve removed from both iOS and macOS clients and will push shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants