Skip to content

Commit

Permalink
fix: sentry sessions (#26192) [cherry-pick] (#26620)
Browse files Browse the repository at this point in the history
Cherry-pick of #26192 for v12.1.0. Original description:

## **Description**

Fix the automatic tracking of Sentry sessions, and refactor Sentry setup
to simplify logic and improve logging.

Specifically:

- Remove all dynamic `autoSessionTracking` enablement.
- Add a custom Sentry transport to prevent any requests reaching Sentry
if metrics are disabled.
- Only consider metrics enabled when preference is set and onboarding is
complete.
- Enable Sentry if `METAMASK_ENVIRONMENT` is not `production` and
`SENTRY_DSN_DEV` is specified.
- Remove the arguments to the `setupSentry` function.
- Since the file already references `process.env` and
`global.stateHooks` internally.
- Flatten all the functions within `setupSentry.js`.
  - Since they no longer require a dynamic `getState` callback.
- Log all Sentry messages via the `debug` package based on the `DEBUG`
environment variable.
  - Create separate loggers for UI and background to differentiate logs.
  - e.g. `DEBUG=metamask:sentry:*` in `.metamask.rc`
- Add additional log messages for easier debug.
- Including all sent events and completed sessions if `METAMASK_DEBUG`
is enabled.
- Add mock `SENTRY_DSN_DEV` to Flask and MMI test builds.
- Remove duplication in package scripts.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26007?quickstart=1)

## **Related issues**

Fixes:
[#2555](MetaMask/MetaMask-planning#2555)
#15691

## **Manual testing steps**

## **Screenshots/Recordings**

### **Before**

### **After**

<img width="713" alt="Updated Logs"
src="https://github.com/user-attachments/assets/8351a968-baea-4cbd-b7b6-0b8000cf1a84">

## **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/develop/.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/develop/.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.

Co-authored-by: Matthew Walsh <[email protected]>
  • Loading branch information
Gudahtt and matthewwalsh0 authored Aug 22, 2024
1 parent b0d1067 commit 04c5fc6
Show file tree
Hide file tree
Showing 13 changed files with 249 additions and 292 deletions.
7 changes: 6 additions & 1 deletion .metamaskrc.dist
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@ BLOCKAID_PUBLIC_KEY=
; Set this to true to enable the snap path for the Firefox WebDriver (Linux)
; FIREFOX_SNAP=

ENABLE_CONFIRMATION_REDESIGN=
; Enable the redesigned confirmations still in development, without needing to toggle the developer setting.
; ENABLE_CONFIRMATION_REDESIGN=

; Enables the Settings Page - Developer Options
; ENABLE_SETTINGS_PAGE_DEV_OPTIONS=true

; The endpoint used to submit errors and tracing data to Sentry.
; The below is the `test-metamask` project.
; SENTRY_DSN_DEV=https://[email protected]/273496
17 changes: 8 additions & 9 deletions app/scripts/controllers/metametrics.js
Original file line number Diff line number Diff line change
Expand Up @@ -454,16 +454,15 @@ export default class MetaMetricsController {
* if not set
*/
async setParticipateInMetaMetrics(participateInMetaMetrics) {
let { metaMetricsId } = this.state;
if (participateInMetaMetrics && !metaMetricsId) {
// We also need to start sentry automatic session tracking at this point
await globalThis.sentry?.startSession();
metaMetricsId = this.generateMetaMetricsId();
} else if (participateInMetaMetrics === false) {
// We also need to stop sentry automatic session tracking at this point
await globalThis.sentry?.endSession();
}
const { metaMetricsId: existingMetaMetricsId } = this.state;

const metaMetricsId =
participateInMetaMetrics && !existingMetaMetricsId
? this.generateMetaMetricsId()
: existingMetaMetricsId;

this.store.updateState({ participateInMetaMetrics, metaMetricsId });

if (participateInMetaMetrics) {
this.trackEventsAfterMetricsOptIn();
this.clearEventsAfterMetricsOptIn();
Expand Down
7 changes: 1 addition & 6 deletions app/scripts/controllers/metametrics.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,10 +143,7 @@ function getMetaMetricsController({
describe('MetaMetricsController', function () {
const now = new Date();
beforeEach(function () {
globalThis.sentry = {
startSession: jest.fn(),
endSession: jest.fn(),
};
globalThis.sentry = {};
jest.useFakeTimers().setSystemTime(now.getTime());
jest.spyOn(Utils, 'generateRandomId').mockReturnValue('DUMMY_RANDOM_ID');
});
Expand Down Expand Up @@ -316,7 +313,6 @@ describe('MetaMetricsController', function () {
metaMetricsController.state.participateInMetaMetrics,
).toStrictEqual(null);
await metaMetricsController.setParticipateInMetaMetrics(true);
expect(globalThis.sentry.startSession).toHaveBeenCalledTimes(1);
expect(
metaMetricsController.state.participateInMetaMetrics,
).toStrictEqual(true);
Expand All @@ -339,7 +335,6 @@ describe('MetaMetricsController', function () {
it('should not nullify the metaMetricsId when set to false', async function () {
const metaMetricsController = getMetaMetricsController();
await metaMetricsController.setParticipateInMetaMetrics(false);
expect(globalThis.sentry.endSession).toHaveBeenCalledTimes(1);
expect(metaMetricsController.state.metaMetricsId).toStrictEqual(
TEST_META_METRICS_ID,
);
Expand Down
15 changes: 12 additions & 3 deletions app/scripts/lib/sentry-filter-events.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { Event as SentryEvent, Integration } from '@sentry/types';
import { logger } from '@sentry/utils';

const NAME = 'FilterEvents';

Expand All @@ -8,17 +7,27 @@ const NAME = 'FilterEvents';
*
* @param options - Options bag.
* @param options.getMetaMetricsEnabled - Function that returns whether MetaMetrics is enabled.
* @param options.log - Function to log messages.
*/
export function filterEvents({
getMetaMetricsEnabled,
log,
}: {
getMetaMetricsEnabled: () => Promise<boolean>;
log: (message: string) => void;
}): Integration {
return {
name: NAME,
processEvent: async (event: SentryEvent) => {
if (!(await getMetaMetricsEnabled())) {
logger.warn(`Event dropped due to MetaMetrics setting.`);
// This integration is required in addition to the custom transport as it provides an
// asynchronous context which we may need in order to read the persisted state from the
// store, so it can later be added to the event via the `beforeSend` overload.
// It also provides a more native solution for discarding events, but any
// session requests will always be handled by the custom transport.
const metricsEnabled = await getMetaMetricsEnabled();

if (!metricsEnabled) {
log('Event dropped as metrics disabled');
return null;
}

Expand Down
Loading

0 comments on commit 04c5fc6

Please sign in to comment.