perf: add tags to UI startup trace (#27550)

## **Description** Add the following tags to the `UI Startup` trace to help identify correlations in startup performance: - `wallet.account_count` - Total number of all accounts in wallet. - `wallet.nft_count` - Total number of all NFTs in the wallet, across all accounts and chains. - `wallet.notification_count` - Total number of notifications in the wallet. - `wallet.pending_approval` - Approval type of the first pending approval. e.g. `transaction`, `eth_signTypedData` - `wallet.token_count` - Total number of ERC-20 tokens in the wallet, across all chains and accounts. - `wallet.transaction_count` - Total number of transactions currently in the wallet, across all accounts, chains, and statuses. - `wallet.unlocked` - `true` or `false` based on if the wallet is currently locked and requires a password. - `wallet.ui_type` - Type of UI being loaded. e.g. `popup`, `notification`, `fullscreen` Tags with a `number` value are Sentry measurements to allow querying with greater than and less than logic. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27550?quickstart=1) ## **Related issues** Fixes: [#3379](MetaMask/MetaMask-planning#3379) [#3273](MetaMask/MetaMask-planning#3273) ## **Manual testing steps** ## **Screenshots/Recordings** ### **Before** ### **After** ## **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: Mark Stacey <[email protected]>
MetaMask · Oct 8, 2024 · 83d5331 · 83d5331
1 parent ad53037
commit 83d5331
Show file tree

Hide file tree

Showing 12 changed files with 525 additions and 64 deletions.
diff --git a/app/scripts/lib/setupSentry.js b/app/scripts/lib/setupSentry.js
@@ -302,7 +302,7 @@ async function getMetaMetricsEnabled() {
 
 function setSentryClient() {
   const clientOptions = getClientOptions();
-  const { dsn, environment, release } = clientOptions;
+  const { dsn, environment, release, tracesSampleRate } = clientOptions;
 
   /**
    * Sentry throws on initialization as it wants to avoid polluting the global namespace and
@@ -322,6 +322,7 @@ function setSentryClient() {
     environment,
     dsn,
     release,
+    tracesSampleRate,
   });
 
   Sentry.registerSpanErrorInstrumentation();

diff --git a/package.json b/package.json
@@ -367,9 +367,9 @@
     "@popperjs/core": "^2.4.0",
     "@reduxjs/toolkit": "patch:@reduxjs/toolkit@npm%3A1.9.7#~/.yarn/patches/@reduxjs-toolkit-npm-1.9.7-b14925495c.patch",
     "@segment/loosely-validate-event": "^2.0.0",
-    "@sentry/browser": "^8.19.0",
-    "@sentry/types": "^8.19.0",
-    "@sentry/utils": "^8.19.0",
+    "@sentry/browser": "^8.33.1",
+    "@sentry/types": "^8.33.1",
+    "@sentry/utils": "^8.33.1",
     "@swc/core": "1.4.11",
     "@trezor/connect-web": "patch:@trezor/connect-web@npm%3A9.3.0#~/.yarn/patches/@trezor-connect-web-npm-9.3.0-040ab10d9a.patch",
     "@zxing/browser": "^0.1.4",

diff --git a/shared/lib/trace.test.ts b/shared/lib/trace.test.ts
@@ -1,4 +1,5 @@
 import {
+  setMeasurement,
   Span,
   startSpan,
   startSpanManual,
@@ -10,6 +11,7 @@ jest.mock('@sentry/browser', () => ({
   withIsolationScope: jest.fn(),
   startSpan: jest.fn(),
   startSpanManual: jest.fn(),
+  setMeasurement: jest.fn(),
 }));
 
 const NAME_MOCK = TraceName.Transaction;
@@ -32,7 +34,8 @@ describe('Trace', () => {
   const startSpanMock = jest.mocked(startSpan);
   const startSpanManualMock = jest.mocked(startSpanManual);
   const withIsolationScopeMock = jest.mocked(withIsolationScope);
-  const setTagsMock = jest.fn();
+  const setMeasurementMock = jest.mocked(setMeasurement);
+  const setTagMock = jest.fn();
 
   beforeEach(() => {
     jest.resetAllMocks();
@@ -41,13 +44,20 @@ describe('Trace', () => {
       startSpan: startSpanMock,
       startSpanManual: startSpanManualMock,
       withIsolationScope: withIsolationScopeMock,
+      setMeasurement: setMeasurementMock,
     };
 
     startSpanMock.mockImplementation((_, fn) => fn({} as Span));
 
+    startSpanManualMock.mockImplementation((_, fn) =>
+      fn({} as Span, () => {
+        // Intentionally empty
+      }),
+    );
+
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     withIsolationScopeMock.mockImplementation((fn: any) =>
-      fn({ setTags: setTagsMock }),
+      fn({ setTag: setTagMock }),
     );
   });
 
@@ -91,8 +101,12 @@ describe('Trace', () => {
         expect.any(Function),
       );
 
-      expect(setTagsMock).toHaveBeenCalledTimes(1);
-      expect(setTagsMock).toHaveBeenCalledWith(TAGS_MOCK);
+      expect(setTagMock).toHaveBeenCalledTimes(2);
+      expect(setTagMock).toHaveBeenCalledWith('tag1', 'value1');
+      expect(setTagMock).toHaveBeenCalledWith('tag2', true);
+
+      expect(setMeasurementMock).toHaveBeenCalledTimes(1);
+      expect(setMeasurementMock).toHaveBeenCalledWith('tag3', 123, 'none');
     });
 
     it('invokes Sentry if no callback provided', () => {
@@ -117,8 +131,12 @@ describe('Trace', () => {
         expect.any(Function),
       );
 
-      expect(setTagsMock).toHaveBeenCalledTimes(1);
-      expect(setTagsMock).toHaveBeenCalledWith(TAGS_MOCK);
+      expect(setTagMock).toHaveBeenCalledTimes(2);
+      expect(setTagMock).toHaveBeenCalledWith('tag1', 'value1');
+      expect(setTagMock).toHaveBeenCalledWith('tag2', true);
+
+      expect(setMeasurementMock).toHaveBeenCalledTimes(1);
+      expect(setMeasurementMock).toHaveBeenCalledWith('tag3', 123, 'none');
     });
 
     it('invokes Sentry if no callback provided with custom start time', () => {
@@ -145,8 +163,12 @@ describe('Trace', () => {
         expect.any(Function),
       );
 
-      expect(setTagsMock).toHaveBeenCalledTimes(1);
-      expect(setTagsMock).toHaveBeenCalledWith(TAGS_MOCK);
+      expect(setTagMock).toHaveBeenCalledTimes(2);
+      expect(setTagMock).toHaveBeenCalledWith('tag1', 'value1');
+      expect(setTagMock).toHaveBeenCalledWith('tag2', true);
+
+      expect(setMeasurementMock).toHaveBeenCalledTimes(1);
+      expect(setMeasurementMock).toHaveBeenCalledWith('tag3', 123, 'none');
     });
   });
 

diff --git a/shared/lib/trace.ts b/shared/lib/trace.ts
@@ -1,10 +1,13 @@
 import * as Sentry from '@sentry/browser';
-import { Primitive, StartSpanOptions } from '@sentry/types';
+import { MeasurementUnit, StartSpanOptions } from '@sentry/types';
 import { createModuleLogger } from '@metamask/utils';
 // TODO: Remove restricted import
 // eslint-disable-next-line import/no-restricted-paths
 import { log as sentryLogger } from '../../app/scripts/lib/setupSentry';
 
+/**
+ * The supported trace names.
+ */
 export enum TraceName {
   BackgroundConnect = 'Background Connect',
   DeveloperTest = 'Developer Test',
@@ -36,29 +39,88 @@ type PendingTrace = {
   startTime: number;
 };
 
+/**
+ * A context object to associate traces with each other and generate nested traces.
+ */
 export type TraceContext = unknown;
 
+/**
+ * A callback function that can be traced.
+ */
 export type TraceCallback<T> = (context?: TraceContext) => T;
 
+/**
+ * A request to create a new trace.
+ */
 export type TraceRequest = {
+  /**
+   * Custom data to associate with the trace.
+   */
   data?: Record<string, number | string | boolean>;
+
+  /**
+   * A unique identifier when not tracing a callback.
+   * Defaults to 'default' if not provided.
+   */
   id?: string;
+
+  /**
+   * The name of the trace.
+   */
   name: TraceName;
+
+  /**
+   * The parent context of the trace.
+   * If provided, the trace will be nested under the parent trace.
+   */
   parentContext?: TraceContext;
+
+  /**
+   * Override the start time of the trace.
+   */
   startTime?: number;
+
+  /**
+   * Custom tags to associate with the trace.
+   */
   tags?: Record<string, number | string | boolean>;
 };
 
+/**
+ * A request to end a pending trace.
+ */
 export type EndTraceRequest = {
+  /**
+   * The unique identifier of the trace.
+   * Defaults to 'default' if not provided.
+   */
   id?: string;
+
+  /**
+   * The name of the trace.
+   */
   name: TraceName;
+
+  /**
+   * Override the end time of the trace.
+   */
   timestamp?: number;
 };
 
 export function trace<T>(request: TraceRequest, fn: TraceCallback<T>): T;
 
 export function trace(request: TraceRequest): TraceContext;
 
+/**
+ * Create a Sentry transaction to analyse the duration of a code flow.
+ * If a callback is provided, the transaction will be automatically ended when the callback completes.
+ * If the callback returns a promise, the transaction will be ended when the promise resolves or rejects.
+ * If no callback is provided, the transaction must be manually ended using `endTrace`.
+ *
+ * @param request - The data associated with the trace, such as the name and tags.
+ * @param fn - The optional callback to record the duration of.
+ * @returns The context of the trace, or the result of the callback if provided.
+ */
 export function trace<T>(
   request: TraceRequest,
   fn?: TraceCallback<T>,
@@ -70,6 +132,12 @@ export function trace<T>(
   return traceCallback(request, fn);
 }
 
+/**
+ * End a pending trace that was started without a callback.
+ * Does nothing if the pending trace cannot be found.
+ *
+ * @param request - The data necessary to identify and end the pending trace.
+ */
 export function endTrace(request: EndTraceRequest) {
   const { name, timestamp } = request;
   const id = getTraceId(request);
@@ -101,6 +169,10 @@ function traceCallback<T>(request: TraceRequest, fn: TraceCallback<T>): T {
     const start = Date.now();
     let error: unknown;
 
+    if (span) {
+      initSpan(span, request);
+    }
+
     return tryCatchMaybePromise<T>(
       () => fn(span),
       (currentError) => {
@@ -131,6 +203,10 @@ function startTrace(request: TraceRequest): TraceContext {
       span?.end(timestamp);
     };
 
+    if (span) {
+      initSpan(span, request);
+    }
+
     const pendingTrace = { end, request, startTime };
     const key = getTraceKey(request);
     tracesByKey.set(key, pendingTrace);
@@ -149,7 +225,7 @@ function startSpan<T>(
   request: TraceRequest,
   callback: (spanOptions: StartSpanOptions) => T,
 ) {
-  const { data: attributes, name, parentContext, startTime, tags } = request;
+  const { data: attributes, name, parentContext, startTime } = request;
   const parentSpan = (parentContext ?? null) as Sentry.Span | null;
 
   const spanOptions: StartSpanOptions = {
@@ -161,8 +237,7 @@ function startSpan<T>(
   };
 
   return sentryWithIsolationScope((scope: Sentry.Scope) => {
-    scope.setTags(tags as Record<string, Primitive>);
-
+    initScope(scope, request);
     return callback(spanOptions);
   });
 }
@@ -182,6 +257,40 @@ function getPerformanceTimestamp(): number {
   return performance.timeOrigin + performance.now();
 }
 
+/**
+ * Initialise the isolated Sentry scope created for each trace.
+ * Includes setting all non-numeric tags.
+ *
+ * @param scope - The Sentry scope to initialise.
+ * @param request - The trace request.
+ */
+function initScope(scope: Sentry.Scope, request: TraceRequest) {
+  const tags = request.tags ?? {};
+
+  for (const [key, value] of Object.entries(tags)) {
+    if (typeof value !== 'number') {
+      scope.setTag(key, value);
+    }
+  }
+}
+
+/**
+ * Initialise the Sentry span created for each trace.
+ * Includes setting all numeric tags as measurements so they can be queried numerically in Sentry.
+ *
+ * @param _span - The Sentry span to initialise.
+ * @param request - The trace request.
+ */
+function initSpan(_span: Sentry.Span, request: TraceRequest) {
+  const tags = request.tags ?? {};
+
+  for (const [key, value] of Object.entries(tags)) {
+    if (typeof value === 'number') {
+      sentrySetMeasurement(key, value, 'none');
+    }
+  }
+}
+
 function tryCatchMaybePromise<T>(
   tryFn: () => T,
   catchFn: (error: unknown) => void,
@@ -251,3 +360,17 @@ function sentryWithIsolationScope<T>(callback: (scope: Sentry.Scope) => T): T {
 
   return actual(callback);
 }
+
+function sentrySetMeasurement(
+  key: string,
+  value: number,
+  unit: MeasurementUnit,
+) {
+  const actual = globalThis.sentry?.setMeasurement;
+
+  if (!actual) {
+    return;
+  }
+
+  actual(key, value, unit);
+}