feat: set Sentry user id#40491
Merged
Merged
Conversation
- Move event gating and rewrite into custom transport (getMetaMetricsState + rewriteReport) - Remove beforeSend, filterEvents integration, and sentry-filter-events.ts - Remove getMetaMetricsEnabled; use getMetaMetricsState with participateInMetaMetrics and metaMetricsId - Update tests for rewriteReport(report, state) and add makeTransport tests - Simplify global types: sentry is Sentry | undefined (drop SentryObject) Made-with: Cursor
Contributor
|
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. |
Contributor
Builds ready [5755578]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…PersistedState Made-with: Cursor
… tests - Export getMetaMetricsStateFromAppState, getMetaMetricsStateFromPersistedState, getMetaMetricsStateFromBackupState - Add unit tests for state getters and makeTransport paths (app state, backup, failures) - Use defaultMetaMetricsState with test-metrics-id for rewriteReport cases Made-with: Cursor
Contributor
Builds ready [6242c6e]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
Gudahtt
reviewed
Mar 18, 2026
Gudahtt
reviewed
Mar 18, 2026
|
|
||
| return await fetch(...args); | ||
| }); | ||
| forEachEnvelopeItem(envelope, (item, type) => { |
Member
There was a problem hiding this comment.
What was the purpose of this refactor? The PR description implies that we'd be checking the opt-in status less often, but I don't think that's true. We're still checking once-per-event here, except now we're using a custom transport rather than an integration. it seems like a more confusing way of doing the same thing.
Contributor
Author
There was a problem hiding this comment.
Before this PR we were checking the opt-in status twice per event: once in the filterEvents function and once in the makeTransport function.
This PR was about to introduce a third location where the opt-in status was needed: in beforeSend's rewriteReport function, in order to set userId in each event.
So what was a little problematic (twice per event) was about to get more problematic (3 times / event), which is why we've introduced the refactoring that merges the logic that lived in filterEvents, beforeSend, and makeTransport into a single place (the custom makeTransport).
Contributor
Author
There was a problem hiding this comment.
We could check the opt-in status even less by introducing caching mechanism, but what makes me feel uncomfortable with this idea is that we’d technically be able to send an event shortly after opt-in status’s been set to false, when the cached value is still true.
Member
There was a problem hiding this comment.
makeTransport is not called per-event, it's called only once when Sentry is initialized. So it was only checking once per event before now.
I am not fully understanding the context of this future second event in rewriteReport. If the user had opted out, I am pretty sure rewriteReport would never get called, so we should have no reason to ever check there.
Contributor
Author
There was a problem hiding this comment.
makeTransport is not called per-event, it's called only once when Sentry is initialized. So it was only checking once per event before now.
My bad, you're right, makeTransport is called only once when Sentry is initialized.
If the user had opted out, I am pretty sure rewriteReport would never get called, so we should have no reason to ever check there.
Yes, that's right. But if the user had opted in, rewriteReport would get called, and would fetch the state again to get metaMetricsId and set it here.
So to recap, here's what we'd have without the refactoring:
- filterEvents -> 1 state fetch per event to get
participateInMetaMetrics - rewriteReport -> 1 state fetch per event to get
metaMetricsId - makeTransport -> 1 state fetch at init (not per event) to get
participateInMetaMetrics
Here's what we'd have with the refactoring:
- makeTransport -> 1 state fetch per event to get both
participateInMetaMetricsandmetaMetricsId
The refactoring reduces the number of state fetches per event from 2 to 1, which makes me think it's worth it. However, if we think having a custom transport rather than an integration is more confusing, I'm fine to revert.
Please let me know what you think.
cc @davidmurdoch as well in case you have an opinion on this
Contributor
Author
There was a problem hiding this comment.
we could derive the id from the debug snapshot
The id set in rewriteReport function is obtained thanks to getMetaMetricsStateFromAppState and getState, which itself gets it from :
globalThis.stateHooks?.getSentryState?.()
That means it is not fetched from local storage or backup storage.
I just wanted to confirm if this is what you mean by "debug snapshot"?
Member
There was a problem hiding this comment.
Yes exactly, that is what I meant by "debug snapshot". That looks good to me.
Member
There was a problem hiding this comment.
Note that the current implementation does not show an ID for pre-initialization errors.
If we really want id for pre-init errors, I think we can get it from the filter integration without adding a new call to get state (maybe we'd want to rename the integration to reflect the change in purpose, since it would be filtering and tagging with ID).
This is a potential enhancement but I don't think it should be a blocker, the current implementation is a huge step forward already.
Contributor
Author
There was a problem hiding this comment.
I tried it out in this secondary PR: #41191
I replaced filterEvents by a "metaMetricsIntegration", which both checks the opt-in status and injects the metaMetricsId .
If the approach makes sense to you, I'll merge it in order to go straight to the target solution that covers all kind of errors, including pre-initialization errors.
…d/flush - Add describe 'makeTransport (construction)' with two unit tests - Assert fetch is not called when makeTransport() is invoked - Assert default transport send and flush are not called at construction - Proves Sentry.makeFetchTransport() does not perform network requests Made-with: Cursor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Merged
7 tasks
itsyoboieltr
previously approved these changes
Mar 26, 2026
…than rewriteReport (#41191) ## **Description** This PR improves #40491 by setting Sentry user id in dedicated metametrics integration, rather than rewriteReport. The benefit of this change, is that it would work for pre-initialization errors as well (cf. @Gudahtt 's comment [here](#40491 (comment))). [](https://codespaces.new/MetaMask/metamask-extension/pull/41191?quickstart=1) ## **Changelog** CHANGELOG entry: null ## **Related issues** See: #40491 ## **Manual testing steps** See: #40491 ## **Screenshots/Recordings** See: #40491 ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes Sentry event filtering and user identification by moving MetaMetrics opt-in checks and `user.id` attachment into a new integration, which could affect which events are sent and how they’re attributed (including early-init errors). > > **Overview** > **Moves MetaMetrics-aware Sentry behavior out of `rewriteReport` and into a dedicated integration.** The new `metaMetricsIntegration` drops events when MetaMetrics is disabled and, when opted-in, attaches `event.user.id` from the same async MetaMetrics state resolution used for gating. > > Removes the old `filterEvents` integration and stops `rewriteReport` from setting `report.user`, updating tests accordingly (new `sentry-metametrics` unit tests; removed `setupSentry` user-id assertions). > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 265ecad. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Contributor
Builds ready [89e3858]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
DDDDDanica
approved these changes
Mar 30, 2026
|
Contributor
Builds ready [8ac6e1e]
⚡ Performance Benchmarks (Total: 🟢 17 pass · 🟡 1 warn · 🔴 0 fail)
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem we're trying to solve:
Today, when we see 100 events in Sentry , we don't know if the issue occurred 100x to the same user VS to 100 different users.
Solution:
This PR sets a userId in Sentry events. Setting a userId in Sentry will allow us to know the number of users impacted by given issue.
We decided to use
metaMetricsIdfor it, as it's kind of an "installationId" that allows to identify a MM instance: it's unique for every MM instance.Additionally, we did some refactoring.
Changelog
CHANGELOG entry: null
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/1753
Manual testing steps
.metamaskrcfile with the DSN of your personal Sentryyarn startmetametricsId:Screenshots/Recordings
See Manual testing steps section
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Changes Sentry event processing and transport gating based on MetaMetrics opt-in state, which could affect whether errors/sessions are reported and how users are identified. Risk is mitigated by added unit tests, but regressions could reduce observability if state resolution fails.
Overview
Sentry events now include a stable user identifier by attaching
event.user.idfrommetaMetricsIdwhen MetaMetrics is opted in.Refactors MetaMetrics opt-in resolution into new
sentry-get-statehelpers (snapshot → persisted → backup fallback) and replaces the oldfilterEventsintegration with a newmetaMetricsIntegrationplus a dedicatedmakeTransportthat blocks all Sentry network requests when opted out.Updates
beforeBreadcrumbto use the new participation state, adjusts globals typing (stateHooks.getBackupState?,globalThis.sentryno longer extended), and adds comprehensive unit tests for state resolution, transport behavior, and the new integration.Written by Cursor Bugbot for commit 8ac6e1e. This will update automatically on new commits. Configure here.