-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
perf: Create custom spans for account overview tabs #28086
Merged
MajorLift
merged 30 commits into
develop
from
jongsun/perf/trace/241009-account-watcher
Nov 13, 2024
Merged
Changes from all commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
8ecf0b2
Revert "feat: codefence Account Watcher for flask (#27543)"
MajorLift 9924b50
Add trace names for account list item and account overview tab
MajorLift a4c46b7
Add trace calls for `AccountListItem` span
MajorLift 394d487
Add trace calls for `AccountOverviewTab` span
MajorLift 39cc6f2
Abort `AccountListItem` span if account tab is switched
MajorLift 656dfdb
Abort `AccountOverviewTab` span if account tab is switched before cur…
MajorLift 9f90b84
Abort `AccountOverviewTab` span if new account is selected in account…
MajorLift 04d0694
Abort `AccountListItem` span if new account is selected in account li…
MajorLift ccc6a34
Revert "Revert "feat: codefence Account Watcher for flask (#27543)""
MajorLift e9b9aef
Remove `AccountOverviewTab`, `AccountListItem` spans
MajorLift c53f8bb
Define custom spans `AccountOverview{AssetList,Nfts,Activity}Tab`
MajorLift b9fa09e
Place `endTrace` calls for new custom spans in corresponding `Account…
MajorLift b79a95e
Define utility functions `{start,end}Traces` for batch handling of tr…
MajorLift b5046fd
Define new redux selector `getDefaultHomeActiveTabName`
MajorLift 694e73d
Trigger and terminate custom spans when clicking on account list item…
MajorLift c0f9811
`endTrace` calls for "Tokens" and "Nfts" tab conditionally trigger wh…
MajorLift 21bbcf6
Trigger `AccountOverviewAssetListTab` span when detected token select…
MajorLift 4fa73c1
Nfts tab span termination also requires `nftsStillFetchingIndication`…
MajorLift a5c2ae2
Define types, constants for account overview tabs and replace `fromTa…
MajorLift 6bbef8e
Revert "Define utility functions `{start,end}Traces` for batch handli…
MajorLift 26b2031
Merge branch 'develop' into jongsun/perf/trace/241009-account-watcher
MajorLift f49728b
Linter fixes
MajorLift 0d914ec
Align types for `defaultHomeActiveTabName`
MajorLift 9c33083
Fix import order
MajorLift cfcc492
Remove `AccountOverviewTabKeys` enum union type
MajorLift df17187
Move `detectNfts` dispatch call out of component library and into `ac…
MajorLift f3c989b
Add `endTrace` call for case where `NftsTab` displays the empty banner
MajorLift 3790fc4
Merge branch 'develop' into jongsun/perf/trace/241009-account-watcher
MajorLift 2718cc4
Merge branch 'develop' into jongsun/perf/trace/241009-account-watcher
MajorLift 7eae322
Remove NftsTab `endTrace` call that triggers prematurely
MajorLift File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://react.dev/learn/you-might-not-need-an-effect#recap
useEffect
to run code "when this variable changes" is done A LOT, but it's actually an anti-patternThis comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied, but with some questions: Wouldn't this be a case where
useEffect
is warranted since we're interfacing with an external/non-local system and triggering a side effect that depends on reactive variables?I'm not sure if introducing impure logic to the top-level scope of a react component is in keeping with the article's recommendations. This usage also doesn't seem to fall under any of the example cases discussed in the article (no computations, no altering state in this or any relative component, no event handlers, etc.).
That said, removing the
useEffect
hooks doesn't seem to trigger extra re-renders or cause malfunctions, so this might be more a question of principle rather than practicality. Still would be nice to understand what the best practice is here (e.g. should we also removeuseEffect
hooks aroundtrackEvent
calls?).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone back to using
useEffect
hooks while implementing the additional spans:I tried removing them, but ran into issues where the spans were not behaving as intended e.g. FCP lasting longer than FMP, FMP lasting longer than fully loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the react docs in more detail, and it seems to indicate that
trace
/endTrace
calls must always be placed insideuseEffect
hooks, unless triggered by an event:useEffect
that's explicitly covered in the docs.trace
,endTrace
are side effects that synchronize with an external system (i.e. sentry). They're also implemented with POST requests, which are definitely Effects.endTrace
calls are measuring load time, their only trigger is the component being displayed. They're also conditionally dependent on reactive values in the component's rendering logic, which leads me to question whether they can be guaranteed to execute correctly or deterministically, if they're not suspended byuseEffect
until after the render completes.Re-renders aren't the only performance consideration here. Removing
useEffect
makes its inner function re-run on every render instead of only when the dependencies are updated. Avoiding these wasteful re-runs is the purpose of populating the dependency array inuseEffect
hooks.To my understanding, ensuring that a react component remains pure is a higher priority concern than reducing re-renders. Directly introducing a side effect into the component body goes beyond the prescribed "last resort" of using
useEffect
and brings us completely outside of the react paradigm. I don't see that an analytics logging call warrants such a measure.useEffect
works by reacting to reactive values, anduseEffect
dependencies are reactive values, meaning an effect triggering a re-render in response to a reactive value being updated is expected behavior, not a performance penalty. We shouldn't be removinguseEffect
just because they cause re-renders, without examining whether they're doing so unnecessarily, or as an unavoidable part of the hook's core functionality.The following use cases for removing an Effect do not seem to apply here, since we're not transforming data, performing a pure calculation, or producing any cacheable artifact.
Edit: Upon discussion..
useEffect
for trace calls that rely on reactive values in render logic.