From 7700daec2a373997516780da01b4630f7b1c28c9 Mon Sep 17 00:00:00 2001 From: tommasini <46944231+tommasini@users.noreply.github.com> Date: Wed, 16 Oct 2024 15:45:32 +0100 Subject: [PATCH] chore: Add tags to custom traces (#11623) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Tags with measures, only added to engine initialisation span for now ![image](https://github.com/user-attachments/assets/1a25b9a8-c9fa-4c14-a36a-882061e0cc7a) app launch times pipeline: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/3492f0bf-1c24-453c-b9ff-35cf5928bd1a ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.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: Aslau Mario-Daniel --- app/selectors/nftController.ts | 13 +- app/selectors/tokensController.ts | 21 +++ app/store/index.ts | 2 + app/util/sentry/tags/index.test.ts | 229 +++++++++++++++++++++++++++++ app/util/sentry/tags/index.ts | 30 ++++ app/util/trace.test.ts | 54 +++++-- app/util/trace.ts | 141 ++++++++++++++++-- e2e/fixtures/fixture-builder.js | 12 ++ 8 files changed, 470 insertions(+), 32 deletions(-) create mode 100644 app/util/sentry/tags/index.test.ts create mode 100644 app/util/sentry/tags/index.ts diff --git a/app/selectors/nftController.ts b/app/selectors/nftController.ts index a2a3a669074..b6a217f727a 100644 --- a/app/selectors/nftController.ts +++ b/app/selectors/nftController.ts @@ -1,5 +1,5 @@ import { createSelector } from 'reselect'; -import { NftControllerState } from '@metamask/assets-controllers'; +import { Nft, NftControllerState } from '@metamask/assets-controllers'; import { RootState } from '../reducers'; const selectNftControllerState = (state: RootState) => @@ -15,3 +15,14 @@ export const selectAllNfts = createSelector( selectNftControllerState, (nftControllerState: NftControllerState) => nftControllerState.allNfts, ); + +export const selectAllNftsFlat = createSelector( + selectAllNfts, + (nftsByChainByAccount) => { + const nftsByChainArray = Object.values(nftsByChainByAccount); + return nftsByChainArray.reduce((acc, nftsByChain) => { + const nftsArrays = Object.values(nftsByChain); + return acc.concat(...nftsArrays); + }, [] as Nft[]); + }, +); diff --git a/app/selectors/tokensController.ts b/app/selectors/tokensController.ts index f63f4c857a6..ccebe64ebfb 100644 --- a/app/selectors/tokensController.ts +++ b/app/selectors/tokensController.ts @@ -37,3 +37,24 @@ export const selectDetectedTokens = createSelector( (tokensControllerState: TokensControllerState) => tokensControllerState?.detectedTokens, ); + +const selectAllTokens = createSelector( + selectTokensControllerState, + (tokensControllerState: TokensControllerState) => + tokensControllerState?.allTokens, +); + +export const selectAllTokensFlat = createSelector( + selectAllTokens, + (tokensByAccountByChain) => { + if (Object.values(tokensByAccountByChain).length === 0) { + return []; + } + const tokensByAccountArray = Object.values(tokensByAccountByChain); + + return tokensByAccountArray.reduce((acc, tokensByAccount) => { + const tokensArray = Object.values(tokensByAccount); + return acc.concat(...tokensArray); + }, [] as Token[]); + }, +); diff --git a/app/store/index.ts b/app/store/index.ts index 84a500f8784..9cc858e9ac0 100644 --- a/app/store/index.ts +++ b/app/store/index.ts @@ -16,6 +16,7 @@ import thunk from 'redux-thunk'; import persistConfig from './persistConfig'; import { AppStateEventProcessor } from '../core/AppStateEventListener'; +import { getTraceTags } from '../util/sentry/tags'; // TODO: Improve type safety by using real Action types instead of `any` // TODO: Replace "any" with type @@ -119,6 +120,7 @@ const createStoreAndPersistor = async (appStartTime: number) => { { name: TraceName.EngineInitialization, op: TraceOperation.EngineInitialization, + tags: getTraceTags(store.getState?.()), }, () => { EngineService.initalizeEngine(store); diff --git a/app/util/sentry/tags/index.test.ts b/app/util/sentry/tags/index.test.ts new file mode 100644 index 00000000000..5511ab7cf92 --- /dev/null +++ b/app/util/sentry/tags/index.test.ts @@ -0,0 +1,229 @@ +import { RootState } from '../../../reducers'; +import { getTraceTags } from './'; +import initialRootState, { + backgroundState, +} from '../../../util/test/initial-root-state'; +import { userInitialState } from '../../../reducers/user'; +import { createMockAccountsControllerState } from '../../../util/test/accountsControllerTestUtils'; + +describe('Tags Utils', () => { + beforeEach(() => { + jest.resetAllMocks(); + }); + + describe('getTraceTags', () => { + it('includes if unlocked', () => { + const state = { + ...initialRootState, + user: { ...userInitialState, userLoggedIn: true }, + }; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.unlocked']).toStrictEqual(true); + }); + + it('includes if not unlocked', () => { + const state = { + ...initialRootState, + user: { ...userInitialState, userLoggedIn: false }, + }; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.unlocked']).toStrictEqual(false); + }); + + it('includes pending approval type', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + ApprovalController: { + ...backgroundState.ApprovalController, + pendingApprovals: { + 1: { + type: 'eth_sendTransaction', + }, + }, + }, + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.pending_approval']).toStrictEqual( + 'eth_sendTransaction', + ); + }); + + it('includes first pending approval type if multiple', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + + ApprovalController: { + ...backgroundState.ApprovalController, + pendingApprovals: { + 1: { + type: 'eth_sendTransaction', + }, + 2: { + type: 'personal_sign', + }, + }, + }, + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.pending_approval']).toStrictEqual( + 'eth_sendTransaction', + ); + }); + + it('includes account count', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + AccountsController: createMockAccountsControllerState([ + '0x1234', + '0x4321', + ]), + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.account_count']).toStrictEqual(2); + }); + + it('includes nft count', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + NftController: { + ...backgroundState.NftController, + allNfts: { + '0x1234': { + '0x1': [ + { + tokenId: '1', + }, + { + tokenId: '2', + }, + ], + '0x2': [ + { + tokenId: '3', + }, + { + tokenId: '4', + }, + ], + }, + '0x4321': { + '0x3': [ + { + tokenId: '5', + }, + ], + }, + }, + }, + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.nft_count']).toStrictEqual(5); + }); + + it('includes notification count', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + NotificationServicesController: { + metamaskNotificationsList: [{}, {}, {}], + }, + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.notification_count']).toStrictEqual(3); + }); + + it('includes token count', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + TokensController: { + allTokens: { + '0x1': { + '0x1234': [{}, {}], + '0x4321': [{}], + }, + '0x2': { + '0x5678': [{}], + }, + }, + }, + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.token_count']).toStrictEqual(4); + }); + + it('includes transaction count', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + TransactionController: { + transactions: [ + { + id: 1, + chainId: '0x1', + }, + { + id: 2, + chainId: '0x1', + }, + { + id: 3, + chainId: '0x2', + }, + ], + }, + }, + }, + } as unknown as RootState; + const tags = getTraceTags(state); + + expect(tags?.['wallet.transaction_count']).toStrictEqual(3); + }); + }); +}); diff --git a/app/util/sentry/tags/index.ts b/app/util/sentry/tags/index.ts new file mode 100644 index 00000000000..796bb9212fe --- /dev/null +++ b/app/util/sentry/tags/index.ts @@ -0,0 +1,30 @@ +import { RootState } from '../../../reducers'; +import { selectAllNftsFlat } from '../../../selectors/nftController'; +import { selectInternalAccounts } from '../../../selectors/accountsController'; +import { selectAllTokensFlat } from '../../../selectors/tokensController'; +import { getNotificationsList } from '../../../selectors/notifications'; +import { selectTransactions } from '../../../selectors/transactionController'; +import { selectPendingApprovals } from '../../../selectors/approvalController'; + +export function getTraceTags(state: RootState) { + if (!Object.keys(state?.engine?.backgroundState).length) return; + const unlocked = state.user.userLoggedIn; + const accountCount = selectInternalAccounts(state).length; + const nftCount = selectAllNftsFlat(state).length; + const notificationCount = getNotificationsList(state).length; + const tokenCount = selectAllTokensFlat(state).length; + const transactionCount = selectTransactions(state).length; + const pendingApprovals = Object.values(selectPendingApprovals(state)); + + const firstApprovalType = pendingApprovals?.[0]?.type; + + return { + 'wallet.account_count': accountCount, + 'wallet.nft_count': nftCount, + 'wallet.notification_count': notificationCount, + 'wallet.pending_approval': firstApprovalType, + 'wallet.token_count': tokenCount, + 'wallet.transaction_count': transactionCount, + 'wallet.unlocked': unlocked, + }; +} diff --git a/app/util/trace.test.ts b/app/util/trace.test.ts index cb90722c9cb..b9e541ebdc5 100644 --- a/app/util/trace.test.ts +++ b/app/util/trace.test.ts @@ -1,17 +1,19 @@ -import { startSpan, startSpanManual, withScope } from '@sentry/react-native'; +import { + Scope, + setMeasurement, + startSpan, + startSpanManual, + withScope, +} from '@sentry/react-native'; import { Span } from '@sentry/types'; -import { - endTrace, - trace, - TraceName, - TRACES_CLEANUP_INTERVAL, -} from './trace'; +import { endTrace, trace, TraceName, TRACES_CLEANUP_INTERVAL } from './trace'; jest.mock('@sentry/react-native', () => ({ withScope: jest.fn(), startSpan: jest.fn(), startSpanManual: jest.fn(), + setMeasurement: jest.fn(), })); const NAME_MOCK = TraceName.Middleware; @@ -36,15 +38,23 @@ describe('Trace', () => { const startSpanMock = jest.mocked(startSpan); const startSpanManualMock = jest.mocked(startSpanManual); const withScopeMock = jest.mocked(withScope); - const setTagsMock = jest.fn(); + const setMeasurementMock = jest.mocked(setMeasurement); + const setTagMock = jest.fn(); beforeEach(() => { jest.resetAllMocks(); startSpanMock.mockImplementation((_, fn) => fn({} as Span)); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - withScopeMock.mockImplementation((fn: any) => fn({ setTags: setTagsMock })); + startSpanManualMock.mockImplementation((_, fn) => + fn({} as Span, () => { + // Intentionally empty + }), + ); + + withScopeMock.mockImplementation((fn: (arg: Scope) => unknown) => + fn({ setTag: setTagMock } as unknown as Scope), + ); }); describe('trace', () => { @@ -87,8 +97,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', () => { @@ -113,8 +127,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', () => { @@ -141,8 +159,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/app/util/trace.ts b/app/util/trace.ts index fd6b4c9dfb3..05ad23d9dd8 100644 --- a/app/util/trace.ts +++ b/app/util/trace.ts @@ -2,15 +2,19 @@ import { startSpan as sentryStartSpan, startSpanManual, withScope, + setMeasurement, + Scope, } from '@sentry/react-native'; import performance from 'react-native-performance'; -import type { Primitive, Span, StartSpanOptions } from '@sentry/types'; +import type { Span, StartSpanOptions, MeasurementUnit } from '@sentry/types'; import { createModuleLogger, createProjectLogger } from '@metamask/utils'; // Cannot create this 'sentry' logger in Sentry util file because of circular dependency const projectLogger = createProjectLogger('sentry'); const log = createModuleLogger(projectLogger, 'trace'); - +/** + * The supported trace names. + */ export enum TraceName { DeveloperTest = 'Developer Test', Middleware = 'Middleware', @@ -56,24 +60,72 @@ export interface PendingTrace { startTime: number; timeoutId: NodeJS.Timeout; } - +/** + * 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 = (context?: TraceContext) => T; - +/** + * A request to create a new trace. + */ export interface TraceRequest { + /** + * Custom data to associate with the trace. + */ data?: Record; + + /** + * 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; + /** + * Custom operation name to associate with the trace. + */ op?: string; } - +/** + * A request to end a pending trace. + */ export interface 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; } @@ -81,6 +133,16 @@ export function trace(request: TraceRequest, fn: TraceCallback): 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( request: TraceRequest, fn?: TraceCallback, @@ -92,6 +154,12 @@ export function trace( 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); @@ -124,6 +192,10 @@ function traceCallback(request: TraceRequest, fn: TraceCallback): T { const start = Date.now(); let error: unknown; + if (span) { + initSpan(span, request); + } + return tryCatchMaybePromise( () => fn(span), (currentError) => { @@ -154,6 +226,10 @@ function startTrace(request: TraceRequest): TraceContext { span?.end(timestamp); }; + if (span) { + initSpan(span, request); + } + const timeoutId = setTimeout(() => { log('Trace cleanup due to timeout', name, id); end(); @@ -178,14 +254,7 @@ function startSpan( request: TraceRequest, callback: (spanOptions: StartSpanOptions) => T, ) { - const { - data: attributes, - name, - parentContext, - startTime, - tags, - op, - } = request; + const { data: attributes, name, parentContext, startTime, op } = request; const parentSpan = (parentContext ?? null) as Span | null; const spanOptions: StartSpanOptions = { @@ -199,7 +268,7 @@ function startSpan( }; return withScope((scope) => { - scope.setTags(tags as Record); + initScope(scope, request); return callback(spanOptions); }) as T; @@ -216,6 +285,40 @@ function getTraceKey(request: TraceRequest) { return [name, id].join(':'); } +/** + * 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: 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: Span, request: TraceRequest) { + const tags = request.tags ?? {}; + + for (const [key, value] of Object.entries(tags)) { + if (typeof value === 'number') { + sentrySetMeasurement(key, value, 'none'); + } + } +} + function getPerformanceTimestamp(): number { return performance.timeOrigin + performance.now(); } @@ -248,3 +351,11 @@ function tryCatchMaybePromise( return undefined; } + +function sentrySetMeasurement( + key: string, + value: number, + unit: MeasurementUnit, +) { + setMeasurement(key, value, unit); +} diff --git a/e2e/fixtures/fixture-builder.js b/e2e/fixtures/fixture-builder.js index c4e987b3f01..b850fab5970 100644 --- a/e2e/fixtures/fixture-builder.js +++ b/e2e/fixtures/fixture-builder.js @@ -421,6 +421,18 @@ class FixtureBuilder { pendingApprovalCount: 0, approvalFlows: [], }, + NotificationServicesController: { + subscriptionAccountsSeen: [], + isMetamaskNotificationsFeatureSeen: false, + isNotificationServicesEnabled: false, + isFeatureAnnouncementsEnabled: false, + metamaskNotificationsList: [], + metamaskNotificationsReadList: [], + isUpdatingMetamaskNotifications: false, + isFetchingMetamaskNotifications: false, + isUpdatingMetamaskNotificationsAccount: [], + isCheckingAccountsPresence: false, + }, }, }, privacy: {