From fbd68d4a3f7ffb6f142515bed962fbaca7241f3e Mon Sep 17 00:00:00 2001 From: Pedro Figueiredo Date: Fri, 31 Mar 2023 14:22:33 +0100 Subject: [PATCH] Introduce action metrics for mv3 service worker restart (#18346) * fix dapp interaction e2e test * wip * add sentry post request mock * fix console errors * fix scripting console error and remove e2e test unnecessary check * clean up * remove e2e test * stop skipping test * fixing build mv3 job * fixing unit tests * fixing unit tests * fixing unit tests * update coverages * revert skip mv3 e2e test * remove IN_TEST on the npm script * remove console.log * revert aria label changes * revert aria label changes * revert permission changes * revert permission changes * implement sw restart delay tracking * fix rebase --- app/scripts/background.js | 4 ++++ app/scripts/controllers/app-state.js | 7 ++++++ app/scripts/metamask-controller.js | 30 +++++++++++++++++++++++++- package.json | 2 ++ shared/constants/metametrics.js | 2 ++ shared/constants/test-flags.js | 1 + test/e2e/fixture-builder.js | 4 ++++ test/e2e/mv3/dapp-interactions.spec.js | 1 + test/e2e/webdriver/driver.js | 16 ++++++-------- ui/store/action-queue/index.test.js | 7 ++++++ ui/store/action-queue/index.ts | 17 +++++++++++++++ 11 files changed, 81 insertions(+), 10 deletions(-) create mode 100644 shared/constants/test-flags.js diff --git a/app/scripts/background.js b/app/scripts/background.js index 956487f89..2fdca5a16 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -556,6 +556,10 @@ export function setupController(initState, initLangCode, overrides) { if (message.name === WORKER_KEEP_ALIVE_MESSAGE) { // To test un-comment this line and wait for 1 minute. An error should be shown on MetaMask UI. remotePort.postMessage({ name: ACK_KEEP_ALIVE_MESSAGE }); + + controller.appStateController.setServiceWorkerLastActiveTime( + Date.now(), + ); } }); } diff --git a/app/scripts/controllers/app-state.js b/app/scripts/controllers/app-state.js index 7d1c83a4e..f7669e055 100644 --- a/app/scripts/controllers/app-state.js +++ b/app/scripts/controllers/app-state.js @@ -51,6 +51,7 @@ export default class AppStateController extends EventEmitter { '0x5': true, '0x539': true, }, + serviceWorkerLastActiveTime: 0, }); this.timer = null; @@ -362,4 +363,10 @@ export default class AppStateController extends EventEmitter { getCurrentPopupId() { return this.store.getState().currentPopupId; } + + setServiceWorkerLastActiveTime(serviceWorkerLastActiveTime) { + this.store.updateState({ + serviceWorkerLastActiveTime, + }); + } } diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 426fc9058..93670073d 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -121,6 +121,7 @@ import { isMain, isFlask } from '../../shared/constants/environment'; // eslint-disable-next-line import/order import { DesktopController } from '@metamask/desktop/dist/controllers/desktop'; ///: END:ONLY_INCLUDE_IN +import { ACTION_QUEUE_METRICS_E2E_TEST } from '../../shared/constants/test-flags'; import { onMessageReceived, checkForMultipleVersionsRunning, @@ -695,6 +696,7 @@ export default class MetamaskController extends EventEmitter { this.keyringController.memStore.subscribe((state) => this._onKeyringControllerUpdate(state), ); + this.keyringController.on('unlock', () => this._onUnlock()); this.keyringController.on('lock', () => this._onLock()); @@ -1190,6 +1192,25 @@ export default class MetamaskController extends EventEmitter { }, ); + if (isManifestV3 && globalThis.isFirstTimeProfileLoaded === false) { + const { serviceWorkerLastActiveTime } = + this.appStateController.store.getState(); + const metametricsPayload = { + category: EVENT.SOURCE.SERVICE_WORKERS, + event: EVENT_NAMES.SERVICE_WORKER_RESTARTED, + properties: { + service_worker_restarted_time: + Date.now() - serviceWorkerLastActiveTime, + }, + }; + + try { + this.metaMetricsController.trackEvent(metametricsPayload); + } catch (e) { + log.warn('Failed to track service worker restart metric:', e); + } + } + this.metamaskMiddleware = createMetamaskMiddleware({ static: { eth_syncing: false, @@ -2627,7 +2648,7 @@ export default class MetamaskController extends EventEmitter { try { // Automatic login via config password const password = process.env.CONF?.PASSWORD; - if (password) { + if (password && !process.env.IN_TEST) { await this.submitPassword(password); } // Automatic login via storage encryption key @@ -2960,6 +2981,13 @@ export default class MetamaskController extends EventEmitter { * @returns {} keyState */ async addNewAccount(accountCount) { + const isActionMetricsQueueE2ETest = + this.appStateController.store.getState()[ACTION_QUEUE_METRICS_E2E_TEST]; + + if (process.env.IN_TEST && isActionMetricsQueueE2ETest) { + await new Promise((resolve) => setTimeout(resolve, 5_000)); + } + const [primaryKeyring] = this.keyringController.getKeyringsByType( KeyringType.hdKeyTree, ); diff --git a/package.json b/package.json index c7aa8f8ed..b6849a24d 100644 --- a/package.json +++ b/package.json @@ -21,6 +21,7 @@ "build:test": "SEGMENT_HOST='https://api.segment.io' SEGMENT_WRITE_KEY='FAKE' SENTRY_DSN_DEV=https://fake@sentry.io/0000000 PORTFOLIO_URL=http://127.0.0.1:8080 yarn build test", "build:test:flask": "yarn build test --build-type flask", "build:test:mv3": "ENABLE_MV3=true SEGMENT_HOST='https://api.segment.io' SEGMENT_WRITE_KEY='FAKE' SENTRY_DSN_DEV=https://fake@sentry.io/0000000 PORTFOLIO_URL=http://127.0.0.1:8080 yarn build test", + "build:test:dev:mv3": "ENABLE_MV3=true SEGMENT_HOST='https://api.segment.io' SEGMENT_WRITE_KEY='FAKE' SENTRY_DSN_DEV=https://fake@sentry.io/0000000 PORTFOLIO_URL=http://127.0.0.1:8080 yarn build:dev testDev --apply-lavamoat=false", "test": "yarn lint && yarn test:unit && yarn test:unit:jest", "dapp": "node development/static-server.js node_modules/@metamask/test-dapp/dist --port 8080", "dapp-chain": "GANACHE_ARGS='-b 2' concurrently -k -n ganache,dapp -p '[{time}][{name}]' 'yarn ganache:start' 'sleep 5 && yarn dapp'", @@ -32,6 +33,7 @@ "test:unit:mocha": "node ./test/run-unit-tests.js --mocha", "test:e2e:chrome": "SELENIUM_BROWSER=chrome node test/e2e/run-all.js", "test:e2e:chrome:snaps": "SELENIUM_BROWSER=chrome node test/e2e/run-all.js --snaps", + "test:e2e:chrome:mv3": "SELENIUM_BROWSER=chrome node test/e2e/run-all.js --mv3", "test:e2e:firefox": "SELENIUM_BROWSER=firefox node test/e2e/run-all.js", "test:e2e:firefox:snaps": "SELENIUM_BROWSER=firefox node test/e2e/run-all.js --snaps", "test:e2e:single": "node test/e2e/run-e2e-test.js", diff --git a/shared/constants/metametrics.js b/shared/constants/metametrics.js index de3244c42..1eeb0a314 100644 --- a/shared/constants/metametrics.js +++ b/shared/constants/metametrics.js @@ -374,6 +374,7 @@ export const EVENT_NAMES = { ONBOARDING_WALLET_IMPORT_ATTEMPTED: 'Wallet Import Attempted', ONBOARDING_WALLET_VIDEO_PLAY: 'SRP Intro Video Played', ONBOARDING_TWITTER_CLICK: 'External Link Clicked', + SERVICE_WORKER_RESTARTED: 'Service Worker Restarted', }; export const EVENT = { @@ -447,6 +448,7 @@ export const EVENT = { DAPP: 'dapp', USER: 'user', }, + SERVICE_WORKERS: 'service_workers', }, LOCATION: { TOKEN_DETAILS: 'token_details', diff --git a/shared/constants/test-flags.js b/shared/constants/test-flags.js new file mode 100644 index 000000000..a5018d6be --- /dev/null +++ b/shared/constants/test-flags.js @@ -0,0 +1 @@ +export const ACTION_QUEUE_METRICS_E2E_TEST = 'action_queue_metrics_e2e_test'; diff --git a/test/e2e/fixture-builder.js b/test/e2e/fixture-builder.js index 81b4d6385..5e3695a05 100644 --- a/test/e2e/fixture-builder.js +++ b/test/e2e/fixture-builder.js @@ -4,6 +4,9 @@ const { } = require('@metamask/snaps-utils'); const { merge } = require('lodash'); const { CHAIN_IDS } = require('../../shared/constants/network'); +const { + ACTION_QUEUE_METRICS_E2E_TEST, +} = require('../../shared/constants/test-flags'); const { SMART_CONTRACTS } = require('./seeder/smart-contracts'); function defaultFixture() { @@ -309,6 +312,7 @@ function onboardingFixture() { [CHAIN_IDS.GOERLI]: true, [CHAIN_IDS.LOCALHOST]: true, }, + [ACTION_QUEUE_METRICS_E2E_TEST]: false, }, NetworkController: { networkId: '1337', diff --git a/test/e2e/mv3/dapp-interactions.spec.js b/test/e2e/mv3/dapp-interactions.spec.js index 8020047cf..294182cc9 100644 --- a/test/e2e/mv3/dapp-interactions.spec.js +++ b/test/e2e/mv3/dapp-interactions.spec.js @@ -12,6 +12,7 @@ describe('MV3 - Dapp interactions', function () { balance: convertToHexValue(25000000000000000000), }, ], + concurrent: { port: 8546, chainId: 1338 }, }; it('should continue to support dapp interactions after service worker re-start', async function () { await withFixtures( diff --git a/test/e2e/webdriver/driver.js b/test/e2e/webdriver/driver.js index 09bdccb1b..2b2fcb386 100644 --- a/test/e2e/webdriver/driver.js +++ b/test/e2e/webdriver/driver.js @@ -428,10 +428,6 @@ class Driver { const artifactDir = `./test-artifacts/${this.browser}/${title}`; const filepathBase = `${artifactDir}/test-failure`; await fs.mkdir(artifactDir, { recursive: true }); - const isPageError = await this.isElementPresent('.error-page__details'); - if (isPageError) { - await this.clickElement('.error-page__details'); - } const screenshot = await this.driver.takeScreenshot(); await fs.writeFile(`${filepathBase}-screenshot.png`, screenshot, { encoding: 'base64', @@ -482,20 +478,22 @@ class Driver { // 4Byte 'Failed to load resource: the server responded with a status of 502 (Bad Gateway)', ]; + const { errors } = this; const cdpConnection = await this.driver.createCDPConnection('page'); await this.driver.onLogEvent(cdpConnection, (event) => { if (event.type === 'error') { - const eventDescription = event.args.filter( + const eventDescriptions = event.args.filter( (err) => err.description !== undefined, ); - const [{ description }] = eventDescription; + + const [eventDescription] = eventDescriptions; const ignore = ignoredErrorMessages.some((message) => - description.includes(message), + eventDescription?.description.includes(message), ); if (!ignore) { - errors.push(description); - logBrowserError(failOnConsoleError, description); + errors.push(eventDescription?.description); + logBrowserError(failOnConsoleError, eventDescription?.description); } } }); diff --git a/ui/store/action-queue/index.test.js b/ui/store/action-queue/index.test.js index 157ac3cb5..d93aa28a8 100644 --- a/ui/store/action-queue/index.test.js +++ b/ui/store/action-queue/index.test.js @@ -41,6 +41,7 @@ describe('ActionQueue', () => { expect(background.backgroundFunction.called).toStrictEqual(false); }); }); + describe('submitRequestToBackground', () => { it('calls promisified background method if the stream is connected', async () => { const background = { @@ -74,6 +75,7 @@ describe('ActionQueue', () => { readable: false, }, backgroundFunction3: sinon.stub().yields(), + trackMetaMetricsEvent: sinon.stub().yields(), }; _setBackgroundConnection(background); const requestPromise = submitRequestToBackground('backgroundFunction3'); @@ -133,6 +135,7 @@ describe('ActionQueue', () => { trace.secondStarted = Date.now(); setTimeout(() => cb(null, 'second'), 10); }, + trackMetaMetricsEvent: sinon.stub().yields(), }; _setBackgroundConnection(background); const scheduled = Promise.all([ @@ -159,6 +162,7 @@ describe('ActionQueue', () => { trace.secondStarted = Date.now(); setTimeout(() => cb(null, 'second'), 10); }, + trackMetaMetricsEvent: sinon.stub().yields(), }; _setBackgroundConnection(background); const scheduled = Promise.all([ @@ -189,6 +193,7 @@ describe('ActionQueue', () => { trace.secondStarted = Date.now(); setTimeout(() => cb(null, 'second'), 10); }, + trackMetaMetricsEvent: sinon.stub().yields(), }; _setBackgroundConnection(background); const scheduled = Promise.all([ @@ -217,6 +222,7 @@ describe('ActionQueue', () => { }, 5); }, second: sinon.stub().yields(), + trackMetaMetricsEvent: sinon.stub().yields(), }; _setBackgroundConnection(background); const scheduled = Promise.race([ @@ -251,6 +257,7 @@ describe('ActionQueue', () => { setTimeout(() => cb(null, 'second'), 10); }, third: sinon.stub().yields(), + trackMetaMetricsEvent: sinon.stub().yields(), }; flowControl.triggerRaceCondition = () => { flowControl.waitFor = submitRequestToBackground('third'); diff --git a/ui/store/action-queue/index.ts b/ui/store/action-queue/index.ts index 86cdb0cef..53b8ce822 100644 --- a/ui/store/action-queue/index.ts +++ b/ui/store/action-queue/index.ts @@ -1,5 +1,8 @@ import pify from 'pify'; +import { EVENT, EVENT_NAMES } from '../../../shared/constants/metametrics'; import { isManifestV3 } from '../../../shared/modules/mv3.utils'; +import { trackMetaMetricsEvent } from '../actions'; + // // A simplified pify maybe? // function pify(apiObject) { // return Object.keys(apiObject).reduce((promisifiedAPI, key) => { @@ -187,6 +190,20 @@ async function processActionRetryQueue() { } processingQueue = true; try { + if (actionRetryQueue.length > 0) { + const metametricsPayload = { + category: EVENT.SOURCE.SERVICE_WORKERS, + event: EVENT_NAMES.SERVICE_WORKER_RESTARTED, + properties: { + service_worker_action_queue_methods: actionRetryQueue.map( + (action) => action.request.method, + ), + }, + }; + + trackMetaMetricsEvent(metametricsPayload); + } + while ( background?.connectionStream.readable && actionRetryQueue.length > 0