Skip to content
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

Set sentry autoTrackSessions default #20132

Merged
merged 9 commits into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/scripts/controllers/metametrics.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down
10 changes: 10 additions & 0 deletions app/scripts/controllers/metametrics.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,14 @@ describe('MetaMetricsController', function () {
const now = new Date();
let clock;
beforeEach(function () {
globalThis.sentry = {
startSession: sinon.fake(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use jest.fn() here to mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not without rewriting the tests to jest. Currently in mocha

/** NOOP */
}),
endSession: sinon.fake(() => {
/** NOOP */
}),
};
clock = sinon.useFakeTimers(now.getTime());
sinon.stub(Utils, 'generateRandomId').returns('DUMMY_RANDOM_ID');
});
Expand Down Expand Up @@ -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);
Expand All @@ -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);
});
});
Expand Down
96 changes: 95 additions & 1 deletion app/scripts/lib/setupSentry.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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
DDDDDanica marked this conversation as resolved.
Show resolved Hide resolved
) {
startSession();
} else if (
isMetaMetricsEnabled === false &&
options.autoSessionTracking === true
DDDDDanica marked this conversation as resolved.
Show resolved Hide resolved
) {
endSession();
}
};

return {
...Sentry,
startSession,
endSession,
toggleSession,
};
}

/**
Expand Down
19 changes: 19 additions & 0 deletions app/scripts/sentry-install.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
30 changes: 30 additions & 0 deletions test/e2e/tests/errors.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines +42 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await driver.fill('#password', 'correct horse battery staple');
await driver.press('#password', driver.Key.ENTER);
await unlockWallet(driver)

// Trigger error
driver.executeScript('window.stateHooks.throwTestError()');
driver.delay(3000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: is there a way to bypass the delay here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we need to wait sometime to give time for a network request to occur

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the inverse test, we wait for a specific network request. I want to give a similar pause but we can't wait for anything in particular.

// 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(
{
Expand Down
16 changes: 16 additions & 0 deletions types/global.d.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,31 @@
// 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;

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<R> {
Expand Down
6 changes: 4 additions & 2 deletions ui/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
26 changes: 26 additions & 0 deletions ui/store/actions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
7 changes: 7 additions & 0 deletions ui/store/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down