From db7fc502167b9d706dd12397a1ec42297648bb70 Mon Sep 17 00:00:00 2001 From: kumavis Date: Wed, 10 Nov 2021 19:27:04 -0800 Subject: [PATCH] metametrics - ensure segment submission failures do not bubble up (#12573) * metametrics - ensure segment submission failures do not bubble up * metametrics - differentiate between trackEvent and submitEvent * metametrics - validate event in trackEvent * metametrics - re-throw error on clean stack * lint fix * controllers/metametrics - take a captureException option * controllers/metametrics - capture and report any errors in trackPage * Update app/scripts/controllers/metametrics.js Co-authored-by: Mark Stacey Co-authored-by: Mark Stacey --- app/scripts/controllers/metametrics.js | 110 +++++++++++++------- app/scripts/controllers/metametrics.test.js | 22 ++-- app/scripts/metamask-controller.js | 2 + 3 files changed, 87 insertions(+), 47 deletions(-) diff --git a/app/scripts/controllers/metametrics.js b/app/scripts/controllers/metametrics.js index 1539cbcba..28e84d7b0 100644 --- a/app/scripts/controllers/metametrics.js +++ b/app/scripts/controllers/metametrics.js @@ -7,6 +7,14 @@ import { METAMETRICS_BACKGROUND_PAGE_OBJECT, } from '../../../shared/constants/metametrics'; +const defaultCaptureException = (err) => { + // throw error on clean stack so its captured by platform integrations (eg sentry) + // but does not interupt the call stack + setTimeout(() => { + throw err; + }); +}; + /** * @typedef {import('../../../shared/constants/metametrics').MetaMetricsContext} MetaMetricsContext * @typedef {import('../../../shared/constants/metametrics').MetaMetricsEventPayload} MetaMetricsEventPayload @@ -51,7 +59,9 @@ export default class MetaMetricsController { version, environment, initState, + captureException = defaultCaptureException, }) { + this._captureException = captureException; const prefState = preferencesStore.getState(); this.chainId = getCurrentChainId(); this.network = getNetworkIdentifier(); @@ -258,32 +268,52 @@ export default class MetaMetricsController { * view */ trackPage({ name, params, environmentType, page, referrer }, options) { - if (this.state.participateInMetaMetrics === false) { - return; - } + try { + if (this.state.participateInMetaMetrics === false) { + return; + } - if (this.state.participateInMetaMetrics === null && !options?.isOptInPath) { - return; + if ( + this.state.participateInMetaMetrics === null && + !options?.isOptInPath + ) { + return; + } + const { metaMetricsId } = this.state; + const idTrait = metaMetricsId ? 'userId' : 'anonymousId'; + const idValue = metaMetricsId ?? METAMETRICS_ANONYMOUS_ID; + this.segment.page({ + [idTrait]: idValue, + name, + properties: { + params, + locale: this.locale, + network: this.network, + chain_id: this.chainId, + environment_type: environmentType, + }, + context: this._buildContext(referrer, page), + }); + } catch (err) { + this._captureException(err); } - const { metaMetricsId } = this.state; - const idTrait = metaMetricsId ? 'userId' : 'anonymousId'; - const idValue = metaMetricsId ?? METAMETRICS_ANONYMOUS_ID; - this.segment.page({ - [idTrait]: idValue, - name, - properties: { - params, - locale: this.locale, - network: this.network, - chain_id: this.chainId, - environment_type: environmentType, - }, - context: this._buildContext(referrer, page), - }); } /** - * track a metametrics event, performing necessary payload manipulation and + * submits a metametrics event, not waiting for it to complete or allowing its error to bubble up + * @param {MetaMetricsEventPayload} payload - details of the event + * @param {MetaMetricsEventOptions} [options] - options for handling/routing the event + */ + trackEvent(payload, options) { + // validation is not caught and handled + this.validatePayload(payload); + this.submitEvent(payload, options).catch((err) => + this._captureException(err), + ); + } + + /** + * submits (or queues for submission) a metametrics event, performing necessary payload manipulation and * routing the event to the appropriate segment source. Will split events * with sensitiveProperties into two events, tracking the sensitiveProperties * with the anonymousId only. @@ -291,21 +321,8 @@ export default class MetaMetricsController { * @param {MetaMetricsEventOptions} [options] - options for handling/routing the event * @returns {Promise} */ - async trackEvent(payload, options) { - // event and category are required fields for all payloads - if (!payload.event || !payload.category) { - throw new Error( - `Must specify event and category. Event was: ${ - payload.event - }. Category was: ${payload.category}. Payload keys were: ${Object.keys( - payload, - )}. ${ - typeof payload.properties === 'object' - ? `Payload property keys were: ${Object.keys(payload.properties)}` - : '' - }`, - ); - } + async submitEvent(payload, options) { + this.validatePayload(payload); if (!this.state.participateInMetaMetrics && !options?.isOptIn) { return; @@ -345,4 +362,25 @@ export default class MetaMetricsController { await Promise.all(events); } + + /** + * validates a metametrics event + * @param {MetaMetricsEventPayload} payload - details of the event + */ + validatePayload(payload) { + // event and category are required fields for all payloads + if (!payload.event || !payload.category) { + throw new Error( + `Must specify event and category. Event was: ${ + payload.event + }. Category was: ${payload.category}. Payload keys were: ${Object.keys( + payload, + )}. ${ + typeof payload.properties === 'object' + ? `Payload property keys were: ${Object.keys(payload.properties)}` + : '' + }`, + ); + } + } } diff --git a/app/scripts/controllers/metametrics.test.js b/app/scripts/controllers/metametrics.test.js index bb0d15bed..9918d6319 100644 --- a/app/scripts/controllers/metametrics.test.js +++ b/app/scripts/controllers/metametrics.test.js @@ -196,14 +196,14 @@ describe('MetaMetricsController', function () { }); }); - describe('trackEvent', function () { + describe('submitEvent', function () { it('should not track an event if user is not participating in metametrics', function () { const mock = sinon.mock(segment); const metaMetricsController = getMetaMetricsController({ participateInMetaMetrics: false, }); mock.expects('track').never(); - metaMetricsController.trackEvent({ + metaMetricsController.submitEvent({ event: 'Fake Event', category: 'Unit Test', properties: { @@ -230,7 +230,7 @@ describe('MetaMetricsController', function () { ...DEFAULT_EVENT_PROPERTIES, }, }); - metaMetricsController.trackEvent( + metaMetricsController.submitEvent( { event: 'Fake Event', category: 'Unit Test', @@ -260,7 +260,7 @@ describe('MetaMetricsController', function () { ...DEFAULT_EVENT_PROPERTIES, }, }); - metaMetricsController.trackEvent( + metaMetricsController.submitEvent( { event: 'Fake Event', category: 'Unit Test', @@ -289,7 +289,7 @@ describe('MetaMetricsController', function () { ...DEFAULT_EVENT_PROPERTIES, }, }); - metaMetricsController.trackEvent( + metaMetricsController.submitEvent( { event: 'Fake Event', category: 'Unit Test', @@ -317,7 +317,7 @@ describe('MetaMetricsController', function () { ...DEFAULT_EVENT_PROPERTIES, }, }); - metaMetricsController.trackEvent({ + metaMetricsController.submitEvent({ event: 'Fake Event', category: 'Unit Test', properties: { @@ -331,7 +331,7 @@ describe('MetaMetricsController', function () { const metaMetricsController = getMetaMetricsController(); const flushStub = sinon.stub(segment, 'flush'); const flushCalled = waitUntilCalled(flushStub, segment); - metaMetricsController.trackEvent( + metaMetricsController.submitEvent( { event: 'Fake Event', category: 'Unit Test', @@ -344,13 +344,13 @@ describe('MetaMetricsController', function () { it('should throw if event or category not provided', function () { const metaMetricsController = getMetaMetricsController(); assert.rejects( - () => metaMetricsController.trackEvent({ event: 'test' }), + () => metaMetricsController.submitEvent({ event: 'test' }), /Must specify event and category\./u, 'must specify category', ); assert.rejects( - () => metaMetricsController.trackEvent({ category: 'test' }), + () => metaMetricsController.submitEvent({ category: 'test' }), /Must specify event and category\./u, 'must specify event', ); @@ -360,7 +360,7 @@ describe('MetaMetricsController', function () { const metaMetricsController = getMetaMetricsController(); assert.rejects( () => - metaMetricsController.trackEvent( + metaMetricsController.submitEvent( { event: 'Fake Event', category: 'Unit Test', @@ -375,7 +375,7 @@ describe('MetaMetricsController', function () { it('should track sensitiveProperties in a separate, anonymous event', function () { const metaMetricsController = getMetaMetricsController(); const spy = sinon.spy(segment, 'track'); - metaMetricsController.trackEvent({ + metaMetricsController.submitEvent({ event: 'Fake Event', category: 'Unit Test', sensitiveProperties: { foo: 'bar' }, diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index bdf26f80f..16b41ec54 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -17,6 +17,7 @@ import LedgerBridgeKeyring from '@metamask/eth-ledger-bridge-keyring'; import LatticeKeyring from 'eth-lattice-keyring'; import EthQuery from 'eth-query'; import nanoid from 'nanoid'; +import { captureException } from '@sentry/browser'; import { AddressBookController, ApprovalController, @@ -190,6 +191,7 @@ export default class MetamaskController extends EventEmitter { version: this.platform.getVersion(), environment: process.env.METAMASK_ENVIRONMENT, initState: initState.MetaMetricsController, + captureException, }); const gasFeeMessenger = this.controllerMessenger.getRestricted({