-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat: add/improve Sentry instrumentation for storage related error handling cp-13.15.0 #39113
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
feat: add/improve Sentry instrumentation for storage related error handling cp-13.15.0 #39113
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
… into database-corruption-workarounds-instrumentation
Builds ready [acd3adf]
UI Startup Metrics (1314 ± 106 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
The backup code was writing full controller state (including non-persistent properties like KeyringController.keyrings and encryptionKey) to storage, while the normal stateChange path correctly filters to only persist: true properties. This caused state corruption when frequently-updating controllers like MetaMetricsController triggered the backup, and is now fixed by applying the same deriveStateFromMetadata() filtering used by ComposableObservableStore.
Builds ready [598145d]
UI Startup Metrics (1299 ± 113 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [598145d]
UI Startup Metrics (1299 ± 113 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| ); | ||
| persistenceManager.update(key, state); | ||
| }); | ||
| } |
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.
This is a fix introduced in this commit: 598145d
Without this fix, should update from data state to split state E2E test was failing.
Adding MetaMetricsController to the list of backed up controllers (cf. discussion here) broke the should update from data state to split state E2E test.
It seems we were persisting the full state to storage (including non-persistent properties), instead of only persisting the persistent properties. This wasn’t visible before adding MetaMetricsController because the other backed up controllers (KeyringController and AppMetadataController) didn’t change very frequently, whereas MetaMetricsController changes more often. Persisting only persistent properties fixed the E2E test.
Builds ready [1ca4d4d]
UI Startup Metrics (1318 ± 114 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
LGTM ! |
Description
This PR is a follow-up PR for #39010
It adds Sentry instrumentation to monitor storage-related errors and enables error reporting even during database corruption scenarios.
Context
Firefox users occasionally experience database corruption that prevents MetaMask from reading or writing to
storage.local. This manifests as"Error: An unexpected error occurred". We have workarounds in place (vault recovery flow, storage error toast), but we lacked visibility into how often these occur in production.Changes
1. Added Sentry Tags and Fingerprints
persistence.errortags tocaptureExceptioncalls:get-failed- whenstorage.local.get()failsset-failed- whenstorage.local.set()failspersist-failed- when IndexedDB backup failsbackup-db-open-failed- when backup database can't be opened2. Enabled Sentry Reporting During Storage Corruption
MetaMetricsControllerto the list of backed-up controllers in IndexedDBgetBackupStatehook to allow Sentry to check MetaMetrics consent from backup when primary storage is unavailable3. Added Recovery Tracking for Temporary Failures
captureMessagecalls to track when storage operations recover after a temporary failure:set-recovered- whenstorage.local.set()succeeds after a previous failurepersist-recovered- when IndexedDB backup succeeds after a previous failurepersistence.eventtag (vspersistence.error) to distinguish recovery events from error events4. Fixed State Persistence in Backup Code Path
KeyringController.encryptionKey) to storagederiveStateFromMetadata()to filter to onlypersist: trueproperties, matching the normalstateChangebehaviorChangelog
CHANGELOG entry: Adds Sentry instrumentation to monitor storage-related errors and enables error reporting even during database corruption scenarios.
Related issues
None
Manual testing steps
Same as in this PR: #39010
Screenshots/Recordings
Before
Nothing visible in Sentry
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Sentry instrumentation & recovery
persistence.errortags and custom fingerprints tocaptureExceptionfor storage paths:get-failed,set-failed,set-backup-failed,persist-failed,persist-backup-failed, andbackup-db-open-failed.captureMessagerecovery signals withpersistence.eventtags:set-recoveredandpersist-recoveredwhen writes/backups succeed after a prior failure.storage.local.geterrors without immediately throwing; throwPersistenceErrorwith backup payload when vault is missing and a backup exists.Backup & consent fallback
MetaMetricsControllertobackedUpStateKeysandBackup; updategetBackup()accordingly.background.jssplit-statestateChange, usederiveStateFromMetadata(..., 'persist')to only back up persistent controller fields; throw if metadata missing.stateHooks.getBackupState()to read IndexedDB backup;setupSentry.getMetaMetricsEnabledfalls back to backup state when primary storage retrieval fails; addgetMetaMetricsEnabledFromBackupState.PersistenceManager robustness
setOnSetFailedwhen writes fail.InvalidStateErrorwith tagged Sentry capture and graceful degradation.Tests
persistence-manager.test.tsto verify new tags/fingerprints, recoverycaptureMessageevents, backup DB open error handling, and repeated failure/success behavior.Written by Cursor Bugbot for commit 1ca4d4d. This will update automatically on new commits. Configure here.