From e28db07a1b1c172ec6201a4e6fce09dca4757ca4 Mon Sep 17 00:00:00 2001 From: Brad Decker Date: Wed, 26 Jul 2023 07:13:28 -0500 Subject: [PATCH] Set sentry autoTrackSessions default (#20132) * Set sentry autoTrackSessions default * endSession.... * fixup * updated comment * prevent breaking devmode * remove changes to beforeSend * remove additional usage of sinon --- app/scripts/controllers/metametrics.js | 4 + app/scripts/controllers/metametrics.test.js | 10 +++ app/scripts/lib/setupSentry.js | 96 ++++++++++++++++++++- app/scripts/sentry-install.js | 19 ++++ test/e2e/tests/errors.spec.js | 30 +++++++ types/global.d.ts | 16 ++++ ui/index.js | 6 +- ui/store/actions.test.js | 26 ++++++ ui/store/actions.ts | 7 ++ 9 files changed, 211 insertions(+), 3 deletions(-) diff --git a/app/scripts/controllers/metametrics.js b/app/scripts/controllers/metametrics.js index 6de45393f..b5ef191fc 100644 --- a/app/scripts/controllers/metametrics.js +++ b/app/scripts/controllers/metametrics.js @@ -425,8 +425,12 @@ export default class MetaMetricsController { setParticipateInMetaMetrics(participateInMetaMetrics) { let { metaMetricsId } = this.state; if (participateInMetaMetrics && !metaMetricsId) { + // We also need to start sentry automatic session tracking at this point + globalThis.sentry?.startSession(); metaMetricsId = this.generateMetaMetricsId(); } else if (participateInMetaMetrics === false) { + // We also need to stop sentry automatic session tracking at this point + globalThis.sentry?.endSession(); metaMetricsId = null; } this.store.updateState({ participateInMetaMetrics, metaMetricsId }); diff --git a/app/scripts/controllers/metametrics.test.js b/app/scripts/controllers/metametrics.test.js index e881f4772..ceab26a5a 100644 --- a/app/scripts/controllers/metametrics.test.js +++ b/app/scripts/controllers/metametrics.test.js @@ -146,6 +146,14 @@ describe('MetaMetricsController', function () { const now = new Date(); let clock; beforeEach(function () { + globalThis.sentry = { + startSession: sinon.fake(() => { + /** NOOP */ + }), + endSession: sinon.fake(() => { + /** NOOP */ + }), + }; clock = sinon.useFakeTimers(now.getTime()); sinon.stub(Utils, 'generateRandomId').returns('DUMMY_RANDOM_ID'); }); @@ -312,6 +320,7 @@ describe('MetaMetricsController', function () { }); assert.equal(metaMetricsController.state.participateInMetaMetrics, null); metaMetricsController.setParticipateInMetaMetrics(true); + assert.ok(globalThis.sentry.startSession.calledOnce); assert.equal(metaMetricsController.state.participateInMetaMetrics, true); metaMetricsController.setParticipateInMetaMetrics(false); assert.equal(metaMetricsController.state.participateInMetaMetrics, false); @@ -328,6 +337,7 @@ describe('MetaMetricsController', function () { it('should nullify the metaMetricsId when set to false', function () { const metaMetricsController = getMetaMetricsController(); metaMetricsController.setParticipateInMetaMetrics(false); + assert.ok(globalThis.sentry.endSession.calledOnce); assert.equal(metaMetricsController.state.metaMetricsId, null); }); }); diff --git a/app/scripts/lib/setupSentry.js b/app/scripts/lib/setupSentry.js index bac3c79ce..7df0cea78 100644 --- a/app/scripts/lib/setupSentry.js +++ b/app/scripts/lib/setupSentry.js @@ -133,8 +133,45 @@ export default function setupSentry({ release, getState }) { Sentry.init({ dsn: sentryTarget, debug: METAMASK_DEBUG, + /** + * autoSessionTracking defaults to true and operates by sending a session + * packet to sentry. This session packet does not appear to be filtered out + * via our beforeSend or FilterEvents integration. To avoid sending a + * request before we have the state tree and can validate the users + * preferences, we initiate this to false. Later, in startSession and + * endSession we modify this option and start the session or end the + * session manually. + * + * In sentry-install we call toggleSession after the page loads and state + * is available, this handles initiating the session for a user who has + * opted into MetaMetrics. This script is ran in both the background and UI + * so it should be effective at starting the session in both places. + * + * In the MetaMetricsController the session is manually started or stopped + * when the user opts in or out of MetaMetrics. This occurs in the + * setParticipateInMetaMetrics function which is exposed to the UI via the + * MetaMaskController. + * + * In actions.ts, after sending the updated participateInMetaMetrics flag + * to the background, we call toggleSession to ensure sentry is kept in + * sync with the user's preference. + * + * Types for the global Sentry object, and the new methods added as part of + * this effort were added to global.d.ts in the types folder. + */ + autoSessionTracking: false, environment, integrations: [ + /** + * Filtering of events must happen in this FilterEvents custom + * integration instead of in the beforeSend handler because the Dedupe + * integration is unaware of the beforeSend functionality. If an event is + * queued in the sentry context, additional events of the same name will + * be filtered out by Dedupe even if the original event was not sent due + * to the beforeSend method returning null. + * + * @see https://github.com/MetaMask/metamask-extension/pull/15677 + */ new FilterEvents({ getMetaMetricsEnabled }), new Dedupe(), new ExtraErrorData(), @@ -144,7 +181,64 @@ export default function setupSentry({ release, getState }) { beforeBreadcrumb: beforeBreadcrumb(getState), }); - return Sentry; + /** + * As long as a reference to the Sentry Hub can be found, and the user has + * opted into MetaMetrics, change the autoSessionTracking option and start + * a new sentry session. + */ + const startSession = () => { + const hub = Sentry.getCurrentHub?.(); + const options = hub.getClient?.().getOptions?.() ?? {}; + if (hub && getMetaMetricsEnabled() === true) { + options.autoSessionTracking = true; + hub.startSession(); + } + }; + + /** + * As long as a reference to the Sentry Hub can be found, and the user has + * opted out of MetaMetrics, change the autoSessionTracking option and end + * the current sentry session. + */ + const endSession = () => { + const hub = Sentry.getCurrentHub?.(); + const options = hub.getClient?.().getOptions?.() ?? {}; + if (hub && getMetaMetricsEnabled() === false) { + options.autoSessionTracking = false; + hub.endSession(); + } + }; + + /** + * Call the appropriate method (either startSession or endSession) depending + * on the state of metaMetrics optin and the state of autoSessionTracking on + * the Sentry client. + */ + const toggleSession = () => { + const hub = Sentry.getCurrentHub?.(); + const options = hub.getClient?.().getOptions?.() ?? { + autoSessionTracking: false, + }; + const isMetaMetricsEnabled = getMetaMetricsEnabled(); + if ( + isMetaMetricsEnabled === true && + options.autoSessionTracking === false + ) { + startSession(); + } else if ( + isMetaMetricsEnabled === false && + options.autoSessionTracking === true + ) { + endSession(); + } + }; + + return { + ...Sentry, + startSession, + endSession, + toggleSession, + }; } /** diff --git a/app/scripts/sentry-install.js b/app/scripts/sentry-install.js index fc654371b..aa1264c04 100644 --- a/app/scripts/sentry-install.js +++ b/app/scripts/sentry-install.js @@ -8,3 +8,22 @@ global.sentry = setupSentry({ release: process.env.METAMASK_VERSION, getState: () => global.stateHooks?.getSentryState?.() || {}, }); + +/** + * As soon as state is available via getSentryState we can call the + * toggleSession method added to sentry in setupSentry to start automatic + * session tracking. + */ +function waitForStateHooks() { + if (global.stateHooks.getSentryState) { + // sentry is not defined in dev mode, so if sentry doesn't exist at this + // point it means that we are in dev mode and do not need to toggle the + // session. Using optional chaining is sufficient to prevent the error in + // development. + global.sentry?.toggleSession(); + } else { + setTimeout(waitForStateHooks, 100); + } +} + +waitForStateHooks(); diff --git a/test/e2e/tests/errors.spec.js b/test/e2e/tests/errors.spec.js index 194578558..6285ac3b6 100644 --- a/test/e2e/tests/errors.spec.js +++ b/test/e2e/tests/errors.spec.js @@ -23,6 +23,36 @@ describe('Sentry errors', function () { }, ], }; + it('should NOT send error events when participateInMetaMetrics is false', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: null, + participateInMetaMetrics: false, + }) + .build(), + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentry, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + await driver.fill('#password', 'correct horse battery staple'); + await driver.press('#password', driver.Key.ENTER); + // Trigger error + driver.executeScript('window.stateHooks.throwTestError()'); + driver.delay(3000); + // Wait for Sentry request + const isPending = await mockedEndpoint.isPending(); + assert.ok( + isPending, + 'A request to sentry was sent when it should not have been', + ); + }, + ); + }); it('should send error events', async function () { await withFixtures( { diff --git a/types/global.d.ts b/types/global.d.ts index f1e8c1b18..2c9c1d126 100644 --- a/types/global.d.ts +++ b/types/global.d.ts @@ -1,6 +1,7 @@ // In order for variables to be considered on the global scope they must be // declared using var and not const or let, which is why this rule is disabled /* eslint-disable no-var */ +import * as Sentry from '@sentry/browser'; declare class Platform { openTab: (opts: { url: string }) => void; @@ -8,8 +9,23 @@ declare class Platform { closeCurrentWindow: () => void; } +declare class SentryObject extends Sentry { + // Verifies that the user has opted into metrics and then updates the sentry + // instance to track sessions and begins the session. + startSession: () => void; + + // Verifies that the user has opted out of metrics and then updates the + // sentry instance to NOT track sessions and ends the current session. + endSession: () => void; + + // Calls either startSession or endSession based on optin status + toggleSession: () => void; +} + export declare global { var platform: Platform; + // Sentry is undefined in dev, so use optional chaining + var sentry: SentryObject | undefined; namespace jest { interface Matchers { diff --git a/ui/index.js b/ui/index.js index 1f1aeb281..21b883115 100644 --- a/ui/index.js +++ b/ui/index.js @@ -195,9 +195,11 @@ function setupDebuggingHelpers(store) { /** * The following stateHook is a method intended to throw an error, used in * our E2E test to ensure that errors are attempted to be sent to sentry. + * + * @param {string} [msg] - The error message to throw, defaults to 'Test Error' */ - window.stateHooks.throwTestError = async function () { - const error = new Error('Test Error'); + window.stateHooks.throwTestError = async function (msg = 'Test Error') { + const error = new Error(msg); error.name = 'TestError'; throw error; }; diff --git a/ui/store/actions.test.js b/ui/store/actions.test.js index ebf77ff77..91e2c2ef7 100644 --- a/ui/store/actions.test.js +++ b/ui/store/actions.test.js @@ -1619,6 +1619,32 @@ describe('Actions', () => { }); }); + describe('#setParticipateInMetaMetrics', () => { + beforeAll(() => { + window.sentry = { + toggleSession: jest.fn(), + endSession: jest.fn(), + }; + }); + it('sets participateInMetaMetrics to true', async () => { + const store = mockStore(); + const setParticipateInMetaMetricsStub = jest.fn((_, cb) => cb()); + + background.getApi.returns({ + setParticipateInMetaMetrics: setParticipateInMetaMetricsStub, + }); + + _setBackgroundConnection(background.getApi()); + + await store.dispatch(actions.setParticipateInMetaMetrics(true)); + expect(setParticipateInMetaMetricsStub).toHaveBeenCalledWith( + true, + expect.anything(), + ); + expect(window.sentry.toggleSession).toHaveBeenCalled(); + }); + }); + describe('#setUseBlockie', () => { afterEach(() => { sinon.restore(); diff --git a/ui/store/actions.ts b/ui/store/actions.ts index 1ebd83eac..176fafefa 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -1570,6 +1570,7 @@ export function updateMetamaskState( }); } }); + // Also emit an event for the selected account changing, either due to a // property update or if the entire account changes. if (isEqual(oldSelectedAccount, newSelectedAccount) === false) { @@ -2883,6 +2884,12 @@ export function setParticipateInMetaMetrics( reject(err); return; } + /** + * We need to inform sentry that the user's optin preference may have + * changed. The logic to determine which way to toggle is in the + * toggleSession handler in setupSentry.js. + */ + window.sentry?.toggleSession(); dispatch({ type: actionConstants.SET_PARTICIPATE_IN_METAMETRICS,