Skip to content

Commit

Permalink
perf: Create custom spans for account overview tabs (#28086)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28086?quickstart=1)

### Motivation

- Creating a Sentry custom span that would capture this performance
issue:
  - #27500
- It was determined that the issue is not captured by the existing "UI
Startup" custom span
(MetaMask/MetaMask-planning#3398).

### Approach

I've decided to widen the scope of this ticket so that it covers
performance issues with all accounts in general (not just those created
with the `account-watcher` snap).

To this end, I've defined three new custom spans: 

- `'Account Overview - Asset List Tab'`
- `'Account Overview - Nfts Tab'`
- `'Account Overview - Activity Tab'`

#### Behavior

- **Triggers** a) when an account list item is clicked in the account
list menu component, or b) when an account overview tab (e.g. "Tokens",
"NFTs", "Activity") is clicked, or c) (Only for `"Account Overview -
Asset List Tab"`) the "Import" button in the detected token selection
popover is clicked.
- **Terminates** when one of the following happens (race):
  - A different account is clicked in the account list menu.
  - An account overview tab is clicked.
- The newly selected tab component finishes loading -- including its
resources.
- If there is no data to display, the loading spinner/message
disappears, and the appropriate filler content renders.

#### Helpers

- Added selector for the `defaultHomeActiveTabName` property of the
`metamask` slice.
- Added enum `AccountOverviewTabKey` and objects
`ACCOUNT_OVERVIEW_TAB_KEY_TO_METAMETRICS_EVENT_NAME_MAP`,
`ACCOUNT_OVERVIEW_TAB_KEY_TO_TRACE_NAME_MAP`.

### Results

- The custom spans function as expected.
- The spans successfully distinguish periods of inactivity with loading
time.
- The spans continue to capture delays while the UI toggles between
accounts and account overview tabs.
<img width="1677" alt="Screenshot 2024-11-12 at 8 52 58 AM"
src="https://github.com/user-attachments/assets/62eda693-f488-46f5-a808-6f25a1763d52">

- The custom spans capture the performance issues that motivated this
ticket
  
- #27500 (see 1.46m
long Nfts Tab span and 5.49s Asset List Tab span)
<img width="1214" alt="Screenshot 2024-11-13 at 1 24 17 AM"
src="https://github.com/user-attachments/assets/9c1f0108-f194-4074-bcc4-1a96f94781d5">
  
- #28130 (see 1.28m
long Nfts Tab span and Error message: "Cannot destructure property
'imageOriginal' ...")
<img width="1214" alt="Screenshot 2024-11-13 at 1 30 00 AM"
src="https://github.com/user-attachments/assets/9baaf40c-d4da-465b-aef5-cbd684d6f450">

## **Related issues**

- Closes MetaMask/MetaMask-planning#3516

## **Manual testing steps**

1. Click one or more of the account overview tabs ("Tokens", "NFTs",
"Activity").
2. Click Account Menu
3. Switch account and repeat.
  - Add accounts as necessary.
- For load testing, add whale account(s) using the experimental "Watch
Account" feature.


## **Screenshots/Recordings**

## **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.
  • Loading branch information
MajorLift authored Nov 13, 2024
1 parent 6bb750a commit 15f7125
Show file tree
Hide file tree
Showing 16 changed files with 113 additions and 35 deletions.
7 changes: 5 additions & 2 deletions app/scripts/controllers/app-state-controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
ORIGIN_METAMASK,
POLLING_TOKEN_ENVIRONMENT_TYPES,
} from '../../../shared/constants/app';
import { AccountOverviewTabKey } from '../../../shared/constants/app-state';
import { AppStateController } from './app-state-controller';
import type {
AllowedActions,
Expand Down Expand Up @@ -209,9 +210,11 @@ describe('AppStateController', () => {

describe('setDefaultHomeActiveTabName', () => {
it('sets the default home tab name', () => {
appStateController.setDefaultHomeActiveTabName('testTabName');
appStateController.setDefaultHomeActiveTabName(
AccountOverviewTabKey.Activity,
);
expect(appStateController.store.getState().defaultHomeActiveTabName).toBe(
'testTabName',
AccountOverviewTabKey.Activity,
);
});
});
Expand Down
7 changes: 5 additions & 2 deletions app/scripts/controllers/app-state-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
import { DEFAULT_AUTO_LOCK_TIME_LIMIT } from '../../../shared/constants/preferences';
import { LastInteractedConfirmationInfo } from '../../../shared/types/confirm';
import { SecurityAlertResponse } from '../lib/ppom/types';
import { AccountOverviewTabKey } from '../../../shared/constants/app-state';
import type {
Preferences,
PreferencesControllerGetStateAction,
Expand All @@ -35,7 +36,7 @@ import type {
export type AppStateControllerState = {
timeoutMinutes: number;
connectedStatusPopoverHasBeenShown: boolean;
defaultHomeActiveTabName: string | null;
defaultHomeActiveTabName: AccountOverviewTabKey | null;
browserEnvironment: Record<string, string>;
popupGasPollTokens: string[];
notificationGasPollTokens: string[];
Expand Down Expand Up @@ -326,7 +327,9 @@ export class AppStateController extends EventEmitter {
*
* @param defaultHomeActiveTabName - the tab name
*/
setDefaultHomeActiveTabName(defaultHomeActiveTabName: string | null): void {
setDefaultHomeActiveTabName(
defaultHomeActiveTabName: AccountOverviewTabKey | null,
): void {
this.store.updateState({
defaultHomeActiveTabName,
});
Expand Down
20 changes: 20 additions & 0 deletions shared/constants/app-state.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { TraceName } from '../lib/trace';
import { MetaMetricsEventName } from './metametrics';

export enum AccountOverviewTabKey {
Tokens = 'tokens',
Nfts = 'nfts',
Activity = 'activity',
}

export const ACCOUNT_OVERVIEW_TAB_KEY_TO_METAMETRICS_EVENT_NAME_MAP = {
[AccountOverviewTabKey.Tokens]: MetaMetricsEventName.TokenScreenOpened,
[AccountOverviewTabKey.Nfts]: MetaMetricsEventName.NftScreenOpened,
[AccountOverviewTabKey.Activity]: MetaMetricsEventName.ActivityScreenOpened,
} as const;

export const ACCOUNT_OVERVIEW_TAB_KEY_TO_TRACE_NAME_MAP = {
[AccountOverviewTabKey.Tokens]: TraceName.AccountOverviewAssetListTab,
[AccountOverviewTabKey.Nfts]: TraceName.AccountOverviewNftsTab,
[AccountOverviewTabKey.Activity]: TraceName.AccountOverviewActivityTab,
} as const;
3 changes: 3 additions & 0 deletions shared/lib/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import { log as sentryLogger } from '../../app/scripts/lib/setupSentry';
*/
export enum TraceName {
AccountList = 'Account List',
AccountOverviewAssetListTab = 'Account Overview Asset List Tab',
AccountOverviewNftsTab = 'Account Overview Nfts Tab',
AccountOverviewActivityTab = 'Account Overview Activity Tab',
BackgroundConnect = 'Background Connect',
DeveloperTest = 'Developer Test',
FirstRender = 'First Render',
Expand Down
7 changes: 7 additions & 0 deletions ui/components/app/assets/nfts/nfts-tab/nfts-tab.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
} from '../../../../../../shared/constants/metametrics';
import { getCurrentLocale } from '../../../../../ducks/locale/locale';
import Spinner from '../../../../ui/spinner';
import { endTrace, TraceName } from '../../../../../../shared/lib/trace';

export default function NftsTab() {
const useNftDetection = useSelector(getUseNftDetection);
Expand Down Expand Up @@ -93,6 +94,12 @@ export default function NftsTab() {
currentLocale,
]);

useEffect(() => {
if (!nftsLoading && !nftsStillFetchingIndication) {
endTrace({ name: TraceName.AccountOverviewNftsTab });
}
}, [nftsLoading, nftsStillFetchingIndication]);

if (!hasAnyNfts && nftsStillFetchingIndication) {
return (
<Box className="nfts-tab__loading">
Expand Down
9 changes: 8 additions & 1 deletion ui/components/app/assets/token-list/token-list.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { ReactNode, useMemo } from 'react';
import React, { ReactNode, useEffect, useMemo } from 'react';
import { shallowEqual, useSelector } from 'react-redux';
import TokenCell from '../token-cell';
import { useI18nContext } from '../../../../hooks/useI18nContext';
Expand All @@ -19,6 +19,7 @@ import {
import { useAccountTotalFiatBalance } from '../../../../hooks/useAccountTotalFiatBalance';
import { getConversionRate } from '../../../../ducks/metamask/metamask';
import { useNativeTokenBalance } from '../asset-list/native-token/use-native-token-balance';
import { endTrace, TraceName } from '../../../../../shared/lib/trace';

type TokenListProps = {
onTokenClick: (arg: string) => void;
Expand Down Expand Up @@ -66,6 +67,12 @@ export default function TokenList({
contractExchangeRates,
]);

useEffect(() => {
if (!loading) {
endTrace({ name: TraceName.AccountOverviewAssetListTab });
}
}, [loading]);

return loading ? (
<Box
display={Display.Flex}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import Popover from '../../../ui/popover';
import Box from '../../../ui/box';
import Button from '../../../ui/button';
import DetectedTokenDetails from '../detected-token-details/detected-token-details';
import { trace, endTrace, TraceName } from '../../../../../shared/lib/trace';

const DetectedTokenSelectionPopover = ({
tokensListDetected,
Expand Down Expand Up @@ -64,7 +65,11 @@ const DetectedTokenSelectionPopover = ({
<Button
className="detected-token-selection-popover__import-button"
type="primary"
onClick={onImport}
onClick={() => {
endTrace({ name: TraceName.AccountOverviewAssetListTab });
trace({ name: TraceName.AccountOverviewAssetListTab });
onImport();
}}
disabled={selectedTokens.length === 0}
>
{t('importWithCount', [`(${selectedTokens.length})`])}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import React, {
useCallback,
Fragment,
useContext,
useEffect,
} from 'react';
import PropTypes from 'prop-types';
import { useSelector } from 'react-redux';
Expand Down Expand Up @@ -53,6 +54,7 @@ import { getMultichainAccountUrl } from '../../../helpers/utils/multichain/block
import { MetaMetricsContext } from '../../../contexts/metametrics';
import { useMultichainSelector } from '../../../hooks/useMultichainSelector';
import { getMultichainNetwork } from '../../../selectors/multichain';
import { endTrace, TraceName } from '../../../../shared/lib/trace';

const PAGE_INCREMENT = 10;

Expand Down Expand Up @@ -258,6 +260,11 @@ export default function TransactionList({
// Check if the current account is a bitcoin account
const isBitcoinAccount = useSelector(isSelectedInternalAccountBtc);
const trackEvent = useContext(MetaMetricsContext);

useEffect(() => {
endTrace({ name: TraceName.AccountOverviewActivityTab });
}, []);

const multichainNetwork = useMultichainSelector(
getMultichainNetwork,
selectedAccount,
Expand Down
20 changes: 19 additions & 1 deletion ui/components/multichain/account-list-menu/account-list-menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import {
getOriginOfCurrentTab,
getSelectedInternalAccount,
getUpdatedAndSortedAccounts,
getDefaultHomeActiveTabName,
///: BEGIN:ONLY_INCLUDE_IF(solana)
getIsSolanaSupportEnabled,
///: END:ONLY_INCLUDE_IF
Expand Down Expand Up @@ -114,7 +115,11 @@ import {
AccountConnections,
MergedInternalAccount,
} from '../../../selectors/selectors.types';
import { endTrace, TraceName } from '../../../../shared/lib/trace';
import { endTrace, trace, TraceName } from '../../../../shared/lib/trace';
import {
ACCOUNT_OVERVIEW_TAB_KEY_TO_TRACE_NAME_MAP,
AccountOverviewTabKey,
} from '../../../../shared/constants/app-state';
///: BEGIN:ONLY_INCLUDE_IF(solana)
import {
SOLANA_WALLET_NAME,
Expand Down Expand Up @@ -251,6 +256,9 @@ export const AccountListMenu = ({
),
[updatedAccountsList, allowedAccountTypes],
);
const defaultHomeActiveTabName: AccountOverviewTabKey = useSelector(
getDefaultHomeActiveTabName,
);
///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps)
const addSnapAccountEnabled = useSelector(getIsAddSnapAccountEnabled);
///: END:ONLY_INCLUDE_IF
Expand Down Expand Up @@ -340,6 +348,16 @@ export const AccountListMenu = ({
location: 'Main Menu',
},
});
endTrace({
name: ACCOUNT_OVERVIEW_TAB_KEY_TO_TRACE_NAME_MAP[
defaultHomeActiveTabName
],
});
trace({
name: ACCOUNT_OVERVIEW_TAB_KEY_TO_TRACE_NAME_MAP[
defaultHomeActiveTabName
],
});
dispatch(setSelectedAccount(account.address));
};
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
} from './account-overview-btc';

const defaultProps: AccountOverviewBtcProps = {
defaultHomeActiveTabName: '',
defaultHomeActiveTabName: null,
onTabClick: jest.fn(),
setBasicFunctionalityModalOpen: jest.fn(),
onSupportLinkClick: jest.fn(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('AccountOverviewEth', () => {
});
it('shows all tabs', () => {
const { queryByTestId } = render({
defaultHomeActiveTabName: '',
defaultHomeActiveTabName: null,
onTabClick: jest.fn(),
setBasicFunctionalityModalOpen: jest.fn(),
onSupportLinkClick: jest.fn(),
Expand Down
40 changes: 23 additions & 17 deletions ui/components/multichain/account-overview/account-overview-tabs.tsx
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
import React, { useCallback, useContext, useMemo } from 'react';
import { useDispatch } from 'react-redux';
import { useHistory } from 'react-router-dom';
import { endTrace, trace } from '../../../../shared/lib/trace';
import { useI18nContext } from '../../../hooks/useI18nContext';
import { ASSET_ROUTE } from '../../../helpers/constants/routes';
import {
///: BEGIN:ONLY_INCLUDE_IF(build-main)
SUPPORT_LINK,
///: END:ONLY_INCLUDE_IF
} from '../../../../shared/lib/ui-utils';
import {
MetaMetricsEventCategory,
MetaMetricsEventName,
} from '../../../../shared/constants/metametrics';
import { MetaMetricsEventCategory } from '../../../../shared/constants/metametrics';
import { MetaMetricsContext } from '../../../contexts/metametrics';
import NftsTab from '../../app/assets/nfts/nfts-tab';
import AssetList from '../../app/assets/asset-list';
Expand All @@ -37,6 +36,12 @@ import {
///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
import InstitutionalHomeFooter from '../../../pages/home/institutional/institutional-home-footer';
///: END:ONLY_INCLUDE_IF
import {
ACCOUNT_OVERVIEW_TAB_KEY_TO_METAMETRICS_EVENT_NAME_MAP,
ACCOUNT_OVERVIEW_TAB_KEY_TO_TRACE_NAME_MAP,
AccountOverviewTabKey,
} from '../../../../shared/constants/app-state';
import { detectNfts } from '../../../store/actions';
import { AccountOverviewCommonProps } from './common';

export type AccountOverviewTabsProps = AccountOverviewCommonProps & {
Expand All @@ -60,6 +65,7 @@ export const AccountOverviewTabs = ({
const history = useHistory();
const t = useI18nContext();
const trackEvent = useContext(MetaMetricsContext);
const dispatch = useDispatch();

const tabProps = useMemo(
() => ({
Expand All @@ -69,24 +75,24 @@ export const AccountOverviewTabs = ({
[],
);

const getEventFromTabName = (tabName: string) => {
switch (tabName) {
case 'nfts':
return MetaMetricsEventName.NftScreenOpened;
case 'activity':
return MetaMetricsEventName.ActivityScreenOpened;
default:
return MetaMetricsEventName.TokenScreenOpened;
}
};

const handleTabClick = useCallback(
(tabName: string) => {
(tabName: AccountOverviewTabKey) => {
onTabClick(tabName);
if (tabName === AccountOverviewTabKey.Nfts) {
dispatch(detectNfts());
}
trackEvent({
category: MetaMetricsEventCategory.Home,
event: getEventFromTabName(tabName),
event: ACCOUNT_OVERVIEW_TAB_KEY_TO_METAMETRICS_EVENT_NAME_MAP[tabName],
});
if (defaultHomeActiveTabName) {
endTrace({
name: ACCOUNT_OVERVIEW_TAB_KEY_TO_TRACE_NAME_MAP[
defaultHomeActiveTabName
],
});
}
trace({ name: ACCOUNT_OVERVIEW_TAB_KEY_TO_TRACE_NAME_MAP[tabName] });
},
[onTabClick],
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const render = (props: AccountOverviewUnknownProps) => {
describe('AccountOverviewUnknown', () => {
it('shows only the activity tab', () => {
const { queryByTestId } = render({
defaultHomeActiveTabName: '',
defaultHomeActiveTabName: null,
onTabClick: jest.fn(),
setBasicFunctionalityModalOpen: jest.fn(),
onSupportLinkClick: jest.fn(),
Expand Down
4 changes: 3 additions & 1 deletion ui/components/multichain/account-overview/common.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { AccountOverviewTabKey } from '../../../../shared/constants/app-state';

export type AccountOverviewCommonProps = {
onTabClick: (tabName: string) => void;
setBasicFunctionalityModalOpen: () => void;
///: BEGIN:ONLY_INCLUDE_IF(build-main)
onSupportLinkClick: () => void;
///: END:ONLY_INCLUDE_IF
defaultHomeActiveTabName: string;
defaultHomeActiveTabName: AccountOverviewTabKey | null;
};
7 changes: 0 additions & 7 deletions ui/components/ui/tabs/tabs.component.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
import React, { useState } from 'react';
import PropTypes from 'prop-types';
import classnames from 'classnames';
import { useDispatch } from 'react-redux';
import Box from '../box';
import {
BackgroundColor,
DISPLAY,
JustifyContent,
} from '../../../helpers/constants/design-system';
import { detectNfts } from '../../../store/actions';

const Tabs = ({
defaultActiveTabKey,
Expand All @@ -22,7 +20,6 @@ const Tabs = ({
const _getValidChildren = () => {
return React.Children.toArray(children).filter(Boolean);
};
const dispatch = useDispatch();

/**
* Returns the index of the child with the given key
Expand All @@ -44,10 +41,6 @@ const Tabs = ({
setActiveTabIndex(tabIndex);
onTabClick?.(tabKey);
}

if (tabKey === 'nfts') {
dispatch(detectNfts());
}
};

const renderTabs = () => {
Expand Down
4 changes: 4 additions & 0 deletions ui/selectors/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1299,6 +1299,10 @@ export function getOriginOfCurrentTab(state) {
return state.activeTab.origin;
}

export function getDefaultHomeActiveTabName(state) {
return state.metamask.defaultHomeActiveTabName;
}

export function getIpfsGateway(state) {
return state.metamask.ipfsGateway;
}
Expand Down

0 comments on commit 15f7125

Please sign in to comment.