-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Fix Sentry breadcrumbs collection during initialization #20521
Conversation
The refactor of the Sentry state in #20491 accidentally broke our opt- in detection. The opt-in detection has been updated to look for both types of application state (during and after initialization).
6ebbff1
to
0ee1b5f
Compare
The check for whether the user had completed onboarding assumed that the application state was post-initialization UI state. It has been updated to handle background state and pre-initialization state as well.
Builds ready [0ee1b5f]
Page Load Metrics (1596 ± 42 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
OK, test instructions updated, I think it works now. I was thrown off at first because I wasn't seeing any breadcrumbs, but that was because I was only moving the state back to migration 91, which is far enough to produce console warnings but not far enough to guarantee they're picked up by Sentry in time to be included. |
Codecov Report
@@ Coverage Diff @@
## develop #20521 +/- ##
===========================================
- Coverage 68.68% 68.65% -0.03%
===========================================
Files 991 991
Lines 38451 38467 +16
Branches 10329 10333 +4
===========================================
Hits 26409 26409
- Misses 12042 12058 +16
|
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.
LGTM!
Builds ready [aabb66b]
Page Load Metrics (1551 ± 55 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [a745509]
Page Load Metrics (1914 ± 73 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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.
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.
LGTM! PR description is written very nicely 🤩
Explanation
The refactor of the Sentry state in #20491 accidentally broke our opt- in detection in the
beforeBreadcrumbs
hook for errors in the background process. It would have broken our error report opt-in detection as well, except that that function was broken in a way that made it work correctly but inefficiently (it was loading the persisted state each time to check, ignoring the application state).The opt-in detection has been updated to look for both types of application state (during and after initialization). The function was refactored to be used for the
beforeBreadcrumbs
hook as well.Manual Testing Steps
This was discovered by @danjm when investigating the e2e test failure in #20517. This can be tested by following a similar approach to the failing e2e test: modify the persisted state in a way that breaks a migration, and ensure the breadcrumbs are collected.
The steps I took are:
metamask-extension/app/scripts/lib/setupSentry.js
Line 140 in 88212a7
SENTRY_DSN_DEV
set in.metamaskrc
onboardingComplete
is set totrue
andparticipateInMetaMetrics
is set totrue
so that the request goes through and breadcrumbs aren't suppressedonboardingComplete
set tofalse
, and you should see the same error report but without the breadcrumbsparticipateInMetaMetrics
set tofalse
and there should be no error reportwindow.stateHooks.throwTestError()
in the UI (checking the network tab to see whether an error is sent, and whether it has breadcrumbs, which should depend on the two properties referenced earlier)window.stateHooks.throwTestBackgroundError()
in the UI (checking the network tab to see whether an error is sent, and whether it has breadcrumbs, which should depend on the two properties referenced earlier)Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Board
label.In this case, a QA Engineer approval will be be required.