Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf: Create custom spans for account overview tabs #28086

Merged
merged 30 commits into from
Nov 13, 2024
Merged

perf: Create custom spans for account overview tabs #28086

merged 30 commits into from
Nov 13, 2024

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Oct 24, 2024 •
edited
Loading

Description

Open in GitHub Codespaces

Motivation

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

Related issues

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

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.

@MajorLift MajorLift added the team-tiger Tiger team (for tech debt reduction + performance improvements) label Oct 24, 2024
@MajorLift MajorLift self-assigned this Oct 24, 2024
@github-actions GitHub Actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@MajorLift MajorLift force-pushed the jongsun/perf/trace/241009-account-watcher branch from 14aaab4 to 20698f5 Compare October 24, 2024 18:10
@MajorLift MajorLift changed the title Create custom spans that capture performance issues in the account overview tabs perf: Create custom spans that capture performance issues in the account overview tabs Oct 24, 2024
@MajorLift MajorLift force-pushed the jongsun/perf/trace/241009-account-watcher branch from 20698f5 to 9e55b3e Compare October 25, 2024 12:42
@metamaskbot
Copy link
Collaborator

Builds ready [9e55b3e]
Page Load Metrics (1941 ± 66 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17262264193413163
domContentLoaded16712147189712158
load16822276194113866
domInteractive26211675627
backgroundConnect11128463215
firstReactRender44194923416
getState597292713
initialActions01000
loadScripts1226154114109746
setupStore1098362713
uiStartup18762728218120699
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 1.12 KiB (0.01%)
  • common: 82 Bytes (0.00%)

@MajorLift MajorLift force-pushed the jongsun/perf/trace/241009-account-watcher branch from 9e55b3e to 808a0a8 Compare October 25, 2024 14:53
@metamaskbot
Copy link
Collaborator

Builds ready [808a0a8]
Page Load Metrics (2028 ± 177 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31236231947535257
domContentLoaded171735292001367176
load172535612028368177
domInteractive178244199
backgroundConnect986312110
firstReactRender52279984723
getState561262210
initialActions0482105
loadScripts125526351491280134
setupStore11106332713
uiStartup192537882255388186
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 1.18 KiB (0.02%)
  • common: 82 Bytes (0.00%)

@MajorLift MajorLift force-pushed the jongsun/perf/trace/241009-account-watcher branch 2 times, most recently from 3265b1b to 15d005c Compare October 28, 2024 12:33
@metamaskbot
Copy link
Collaborator

Builds ready [15d005c]
Page Load Metrics (2100 ± 172 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint184535762100374179
domContentLoaded183335012063367176
load184735052100359172
domInteractive28102542110
backgroundConnect776332010
firstReactRender54164992412
getState66016178
initialActions01000
loadScripts133929411569349168
setupStore1270262110
uiStartup205737752304375180
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 68 Bytes (0.00%)
  • ui: 1.18 KiB (0.02%)
  • common: 82 Bytes (0.00%)

@MajorLift MajorLift force-pushed the jongsun/perf/trace/241009-account-watcher branch from 15d005c to c734e0a Compare October 28, 2024 16:30
@metamaskbot
Copy link
Collaborator

Builds ready [c734e0a]
Page Load Metrics (1949 ± 96 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29023451870416200
domContentLoaded16682275190717986
load16862324194919996
domInteractive25243705727
backgroundConnect9135403416
firstReactRender482751005426
getState482232411
initialActions01000
loadScripts11971632139313062
setupStore1197472713
uiStartup187528882199267128
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 68 Bytes (0.00%)
  • ui: 1.18 KiB (0.02%)
  • common: 82 Bytes (0.00%)

@MajorLift MajorLift mentioned this pull request Oct 28, 2024
Open
@MajorLift MajorLift marked this pull request as ready for review October 28, 2024 17:19
@MajorLift MajorLift requested review from a team as code owners October 28, 2024 17:19
@MajorLift MajorLift added area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. area-Sentry error reporting to sentry labels Oct 28, 2024
@MajorLift MajorLift force-pushed the jongsun/perf/trace/241009-account-watcher branch from e5d8844 to 67b9639 Compare October 28, 2024 17:53
@MajorLift MajorLift marked this pull request as draft October 28, 2024 17:59
@MajorLift MajorLift force-pushed the jongsun/perf/trace/241009-account-watcher branch from 67b9639 to bae0053 Compare October 28, 2024 18:05
@MajorLift MajorLift changed the title perf: Create custom spans that capture performance issues in the account overview tabs perf: Create custom spans "Account List Item", "Account Overview Tab" Oct 28, 2024
@MajorLift MajorLift changed the title perf: Create custom spans "Account List Item", "Account Overview Tab" perf: Create custom spans "Account List Item", "Account Overview Tab" Oct 28, 2024
@MajorLift MajorLift marked this pull request as ready for review October 28, 2024 18:29
@metamaskbot
Copy link
Collaborator

Builds ready [bae0053]
Page Load Metrics (2021 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17802304201013565
domContentLoaded17632289197714168
load17782362202114771
domInteractive1990482010
backgroundConnect1293302411
firstReactRender52155942512
getState568392512
initialActions01000
loadScripts12891696146411756
setupStore1274282110
uiStartup19832598223315474
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 1.21 KiB (0.02%)
  • common: 82 Bytes (0.00%)

@MajorLift MajorLift marked this pull request as draft October 28, 2024 19:18
@MajorLift MajorLift force-pushed the jongsun/perf/trace/241009-account-watcher branch from 5f56a80 to b30323f Compare October 30, 2024 10:29
@metamaskbot
Copy link
Collaborator

Builds ready [80bdaed]
Page Load Metrics (1813 ± 89 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15752343180918388
domContentLoaded15242328178918388
load15732344181318589
domInteractive14101532512
backgroundConnect9100272512
firstReactRender47164842813
getState45312147
initialActions01000
loadScripts10761815132716680
setupStore1167282110
uiStartup17442549200519392
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 2.47 KiB (0.03%)
  • common: 271 Bytes (0.00%)

@MajorLift
Copy link
Contributor Author

MajorLift commented Nov 5, 2024 •
edited
Loading

A second issue is actually you don't want to end the timer when you switch tabs. If you click the NFTs tab, then before it finishes loading you click the Tokens tab, I think you actually want to just cancel that trace, not end it.

I'm not sure about this. Wouldn't this introduce an artificial left-skewedness to the distribution? Discarding all data points where the user navigates away because the current tab takes too long to load might lead to a pretty strong selection effect. On the lower end, the current tab not having finished loading by the time the user is able to click a tab also implies a high floor, so I imagine selectively removing those data points would have a significant effect.

An alternative might be capturing aborted load times in a separate data set, but I'm not sure if that's particularly useful to us. Since we're mostly interested in outlier values and the percentiles used to define them, I think aggregating the data might give us better insights (even if our data is essentially a mixture of two distributions that are similar but offset or curtailed).

Not 100% on that last point, though. Maybe separating the datasets provides additional insights or is useful in some way that I'm not considering.

Edit: Upon discussion..

  • Add a tag to the aborted loads so they can be optionally filtered.

@MajorLift MajorLift marked this pull request as draft November 5, 2024 19:42
@MajorLift MajorLift force-pushed the jongsun/perf/trace/241009-account-watcher branch 2 times, most recently from 05131aa to 36b9da1 Compare November 6, 2024 12:43
@MajorLift MajorLift force-pushed the jongsun/perf/trace/241009-account-watcher branch from 3b7255d to f3c989b Compare November 11, 2024 10:04
@metamaskbot
Copy link
Collaborator

Builds ready [3790fc4]
Page Load Metrics (2072 ± 231 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint170837042077480231
domContentLoaded169636262044466224
load170537062072481231
domInteractive18266605627
backgroundConnect996292512
firstReactRender562031092914
getState45915178
initialActions01000
loadScripts122127871517383184
setupStore65514157
uiStartup189840322308506243
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 2.61 KiB (0.03%)
  • common: 271 Bytes (0.00%)

@MajorLift MajorLift marked this pull request as ready for review November 11, 2024 11:25
@metamaskbot
Copy link
Collaborator

Builds ready [2718cc4]
Page Load Metrics (1884 ± 103 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint165825801890212102
domContentLoaded164225091852212102
load166025841884214103
domInteractive148444199
backgroundConnect977362412
firstReactRender533921206833
getState4116132411
initialActions01000
loadScripts11921874137017283
setupStore65412147
uiStartup185328882118287138
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 2.61 KiB (0.03%)
  • common: 271 Bytes (0.00%)

@MajorLift
Copy link
Contributor Author

@HowardBraham Following up on review feedback:

  • Use useEffect for trace calls that rely on reactive values in render logic.

Restored useEffect hooks around trace calls.

  • Verify that superfluous re-renders are minimized.

    • The NftsTab component takes 5 renders to load, which is one more than in the develop branch. The additional render is due to a parent component loading.
    • AssetList, TokenList, TransactionList do not add re-renders compared to develop.

  • Add custom spans for "when does anything at all display", as opposed to "when does the last resource finish loading."

How would you feel about splitting this into its own ticket: #28401?

Adding these spans is causing e2e failures (https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/110256/workflows/e5f21f65-65da-496f-824a-be04166ff3a4/jobs/4125000/tests) that I'm looking into, but in the meantime it doesn't seem necessary to keep the spans in this PR blocked from going into production.

Also, the additional spans aren't strictly necessary to detect the account-watcher performance issue. In contrast, there doesn't seem to be a straightforward way to avoid using a measure that includes resource load time, since the issue appears to cause render delays that are disproportionate to the response times for resource fetch requests.

I agree that it's still worth following up with an investigation into whether the performance issue also affects account overview tabs FCP time, since that would provide a metric that is decoupled from resource load bandwidth. I'm not seeing meaningful differences on local, but that could easily change with production data.

Documenting a related comment from @Gudahtt:

I'd recommend only adding the most valuable traces.
We do want teams to add new traces to monitor the performance of critical flows, ... [not] "nice to have" performance data, just the most valuable metrics that you want to actively monitor.
— https://consensys.slack.com/archives/C07KB8HRZ4J/p1731073187970779?thread_ts=1731066384.125969&cid=C07KB8HRZ4J

  • Add a tag to the aborted loads so they can be optionally filtered.

This requires the ability to add tags to a custom span while calling endTrace (or Sentry.Span['end']), which should probably be scoped as its own ticket. Working on this here: #28400.

HowardBraham
HowardBraham previously approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@HowardBraham HowardBraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if a PR doesn't actually break functionality, but just could be better, I'm usually in favor of merging and then improving later.

MajorLift
MajorLift commented Nov 13, 2024
View reviewed changes
ui/components/app/assets/nfts/nfts-tab/nfts-tab.js Outdated
@@ -84,6 +85,7 @@ export default function NftsTab() {
referrer: ORIGIN_METAMASK,
},
});
endTrace({ name: TraceName.AccountOverviewNftsTab });
Copy link
Contributor Author

@MajorLift MajorLift Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HowardBraham Had to remove this line as it always triggers when nftsLoading is set to false, even if the detectNfts background request is still pending. I'll use this to implement TraceName.AccountOverviewNftsTabFCP in the PR for additional spans.

Suggested change
endTrace({ name: TraceName.AccountOverviewNftsTab });

I added screenshots in the PR description Results section showing that the NftsTab span now captures both performance issues targeted by this ticket.

@metamaskbot
Copy link
Collaborator

Builds ready [7eae322]
Page Load Metrics (1925 ± 69 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17062393193816278
domContentLoaded16832192189413464
load17082271192514369
domInteractive1692512110
backgroundConnect1085272110
firstReactRender47157102199
getState580182110
initialActions01000
loadScripts11871611139611455
setupStore65915189
uiStartup18832665213917584
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 2.55 KiB (0.03%)
  • common: 271 Bytes (0.00%)

@MajorLift MajorLift added this pull request to the merge queue Nov 13, 2024
Merged via the queue into develop with commit 15f7125 Nov 13, 2024
88 checks passed
@MajorLift MajorLift deleted the jongsun/perf/trace/241009-account-watcher branch November 13, 2024 17:08
@github-actions github-actions bot locked and limited conversation to collaborators Nov 13, 2024
@metamaskbot metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Nov 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vinnyhoward vinnyhoward vinnyhoward approved these changes

@HowardBraham HowardBraham HowardBraham approved these changes

Assignees

@MajorLift MajorLift

Labels
area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. area-Sentry error reporting to sentry release-12.8.0 Issue or pull request that will be included in release 12.8.0 team-tiger Tiger team (for tech debt reduction + performance improvements) team-wallet-framework
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MajorLift @metamaskbot @HowardBraham @vinnyhoward