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.

Sign up for GitHub

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

Jump to bottom

Apple client send and receive support for crash report cohort IDs (CRCIDs) #1116

Merged
merged 32 commits into from
Dec 19, 2024
Merged

Apple client send and receive support for crash report cohort IDs (CRCIDs) #1116

merged 32 commits into from
Dec 19, 2024

Conversation

jdjackson
Copy link
Contributor

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/1208592102886666/1208759541597499/f
iOS & macOS PRs: I’m looking for feedback on these BSK changes before posting app-level PRs
What kind of version bump will this require?: Minor (is this right? 😊).
CC: @LarryTurtis

Optional:
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. Before this is ready to merge:

  • I’ll have to coordinate iOS and macOS changes to match, as the API signatures for the CrashCollection class have changed
  • I will be waiting past the December 9 code freeze, so these changes are not included in the last public release of 2024, and instead have an extended internal testing period

Steps to test this PR:

  1. Relying on included automated tests is the primary testing function, but this can be tested live against @LarryTurtis’s dev instance. To do this, set CrashReportSender line 35 to:
    static let reportServiceUrl = URL(string: "https://9e3c-20-75-144-152.ngrok-free.app/crash.js")!

I have tested via the following means:

  • Unit/integration tests included in this PR.
  • Local changes to the iOS app to integrate with these BSK changes, seeing crashes submit to @LarryTurtis’s dev instance, which in turn routes crash through to our Sentry instance (development environment), and ensuring that the client sends, receives, and updates, its crcid when appropriate.

Note that I have not been able to test equivalent changes on macOS because I’m unable to add my computer to the DDG provisioning profile (see https://app.asana.com/0/1200194497630846/1208709596010744/f).

OS Testing:

  • iOS 14
  • iOS 15
  • iOS 16
  • macOS 10.15
  • macOS 11
  • macOS 12

—

Internal references:

Software Engineering Expectations
Technical Design Template

@jdjackson
Adopt dependency injection support for CrashCollection
88ae14a 
CrashCollection now supports dependency injection of both CrashReportSending and KeyValueStoring parameters.
@jdjackson
CrashCollectionTests: First test and supporting implementation nearly…
8583c2d 
… complete

• CrashCollectionTests has a new test for CRCID use (first of a few planned)
• CrashReportSender now returns data or an error
• CrashReportSender is now testable (previously it’s async design made it impossible to test results of sending)
@jdjackson
Support for CRCID send/receive
24cb701 
CrashReportSender and CrashCollection now support sending and recieving CRCIDs  with a not-yet-finalized HTTP header value.  One test present, and more planned out.
@jdjackson
CrashCollection supports CRCID store and retrieve
418c64c 
Tested against the dev server instance, and CRCIDs are sent and received as expected!
@jdjackson
Indentation fix
3b5d42a
@jdjackson
2 more tests implemented
1f6b78a
@jdjackson
Clarified remaining potential future tests
8440d72
@jdjackson
Cleanup pre-code-review
4dbf3f0
@jdjackson jdjackson mentioned this pull request Dec 6, 2024
Merged
16 tasks
@jdjackson
Refactor CRCID storage management into new CRCIDManager class
acc46b7 
This enables reuse in the macOS app non-appstore builds (which don’t use CrashCollection).
@jdjackson
Copy link
Contributor Author

Ok, I’ve updated BSK again so we can support crashes in the macOS app as well. To do this, I’ve created a new CRCIDManager class that is responsible for crcid storage and handling logic around updating, or not, the CRCID based on server response. This lets the macOS client (non-appstore) use the same logic without relying on CrashCollection, which is only used by macOS app store builds.

@samsymons samsymons self-requested a review December 14, 2024 04:43
@jdjackson
Fixing sending of empty CRCID header when no value is present
073fff9 
Empty CRCID header leads server to assign a CRCID.  No header = no cohorting.
@jdjackson jdjackson force-pushed the jjackson/crcid-send-receive-support branch from 3249ac6 to 4fd8049 Compare December 18, 2024 19:58
@jdjackson jdjackson marked this pull request as ready for review December 18, 2024 20:41
jdjackson and others added 7 commits December 19, 2024 09:10
@jdjackson
Code review cleanup
b6e731d 
No longer need to have MockKeyValueStore inherit NSObject.  And left explanatory comment about calls to crash.js needing to be sequential.
@jdjackson
Cleaner handling of CRCID as optional
07ca1c4 
CRCID header value needs to be an empty string when we don't have a value yet, but KeyValueStoring is more  appropriately used with optional values.  We now support an optional value in CRCIDManager, and send an empty string with the header if we don't have one yet.
@samsymons
Move CRCIDManager out into its own file.
15b0726
@samsymons
Replace print statement with Logger.
1c91259
@samsymons
Fix header comment formatting.
c42eb19
@samsymons
Use UserDefaults.standard.
c28ed68
@samsymons
Merge branch 'main' into jjackson/crcid-send-receive-support
88449bb 
* main: (24 commits)
  change api (#1133)
  Ensure authToken is present before calling refreshAuthTokenIfNeeded
  Add 'locale' to report broken site params
  Add 'locale' to report broken site params
  Ensure authToken is present before calling refreshAuthTokenIfNeeded
  Privacy Pro Free Trials - Models and API (#1120)
  remove MaliciousSiteProtectionSubfeature (#1131)
  Malware protection 6: Malware integration (#1099)
  Add underlying error to PrivacyStatsError (#1130)
  Initial changes for compilation time tracking. (#1111)
  iOS System level credential provider (#1127)
  Increase ratio of complete form saves (#1124)
  Add PrivacyStatsError.failedToClearPrivacyStats (#1128)
  Update autofill to 16.0.0 (#1122)
  Update local network routing (#1117)
  Update RMF version matching to ignore build number (#1118)
  Fix threading issue between using a Semaphore and async/await (#1115)
  add experiment test fake feature (#1119)
  Bump Tests/BrowserServicesKitTests/Resources/privacy-reference-tests from `a603ff9` to `6133e7d` (#1109)
  experiment default metrics pixels (#1107)
  ...
@samsymons
Merge branch 'main' into jjackson/crcid-send-receive-support
bcb85ec 
* main:
  Disable flaky tests (#1142)
  Add force old app delegate flag (#1141)
@samsymons samsymons merged commit 5704d77 into main Dec 19, 2024
7 checks passed
@samsymons samsymons deleted the jjackson/crcid-send-receive-support branch December 19, 2024 21:11
jdjackson added a commit to duckduckgo/iOS that referenced this pull request Dec 19, 2024
@jdjackson
Crash report cohort ID support for iOS (#3692)
8ce3815 
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](duckduckgo/BrowserServicesKit#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)
1. 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.
2. 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).
samsymons added a commit that referenced this pull request Dec 19, 2024
@samsymons
Merge branch 'main' into sam/upgrade-to-xcode-16-2
1fbca59 
* main:
  Apple client send and receive support for crash report cohort IDs (CRCIDs) (#1116)
  Disable flaky tests (#1142)
  Add force old app delegate flag (#1141)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LarryTurtis LarryTurtis LarryTurtis left review comments

@samsymons samsymons samsymons approved these changes

@ayoy ayoy Awaiting requested review from ayoy

@loremattei loremattei Awaiting requested review from loremattei

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jdjackson @samsymons @LarryTurtis