Skip to content

Commit a0ea6e4

Browse files
chore(runway): cherry-pick chore: add advanced charts traces cp-7.76.0 (#29792)
- chore: add advanced charts traces cp-7.76.0 (#29497) ## **Description** Adds Sentry performance traces for token-overview advanced chart visibility (time from series change until skeleton clears), with separate trace types for dashboards to distinguish initial load / asset or currency changes from time-range-only updates. Also fixes a bug in metrics consent detection during early Sentry initialization. ## Changes ### Advanced Chart Performance Tracing **AdvancedChart component:** - Added optional `onSkeletonHidden` callback that fires once when loading/layout settles and the chart is ready - Guarded by refs to reset when series key or HTML content changes **PriceAdvanced component:** - Starts `trace()` when `ohlcvSeriesKey` changes - Ends trace via `endTrace()` when: - Skeleton hides (chart ready) - Chart error occurs - Trace is superseded by a newer series - Falls back to legacy chart - `getAdvancedChartVisibilityTraceRequest()` selects appropriate `TraceName` and `TraceOperation` based on what changed: - Same asset + currency with only range change → `TokenOverviewAdvancedChartTimeRangeVisible` trace - Otherwise → `TokenOverviewAdvancedChartInitialVisible` trace **trace.ts:** - Added new `TraceName` values: - `TokenOverviewAdvancedChartInitialVisible` - for initial load or asset/currency changes - `TokenOverviewAdvancedChartTimeRangeVisible` - for time range selector changes only - Added new `TraceOperation` values: - `token_overview.advanced_chart` - initial load or asset/currency change - `token_overview.advanced_chart_time_range` - time range change only ### Metrics Consent Fix **Problem:** The original implementation attempted to read from `analytics.isEnabled()` during Sentry initialization in `index.js:45`. However, this occurs before Engine and Redux are initialized, causing `analytics.isEnabled()` to incorrectly return `false` even for opted-in users (it checks in-memory state that doesn't exist yet). **Solution:** `hasMetricsConsent()` now reads directly from AnalyticsController's persisted state in FilesystemStorage (`persist:AnalyticsController`) before falling back to the legacy `METRICS_OPT_IN` storage key. This approach: - Works before Engine/Redux initialization (FilesystemStorage is always available) - Reads the actual persisted opt-in value from disk (`{"optedIn": true/false}`) - Follows the same pattern as `ControllerStorage.getAllPersistedState()` in `persistConfig/index.ts` - Avoids dependency on deprecated `METRICS_OPT_IN` as the primary source **MetaMetricsAndDataCollectionSection:** - Calls `updateCachedConsent()` when user toggles analytics participation - Ensures in-memory consent cache stays synchronized with AnalyticsController state ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: Adds tracing for advanced charts ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/ASSETS-3129 ## **Manual testing steps** ```gherkin Feature: Advanced Chart Performance Tracing Scenario: user opens token details and chart loads successfully Given the user is on the home screen When user taps on a token to open token details Then a single "Starting sampled root span" log appears for "Token Overview Advanced Chart Initial Visible" And a single "Finishing" log appears for the same span ID And no duplicate "Token Overview" traces appear Scenario: user navigates back before chart finishes loading Given the user is on token details with the chart still loading (skeleton visible) When user quickly navigates back to the home screen Then a "Finishing" log appears for the "Token Overview Advanced Chart Initial Visible" span And the finish log appears after the navigation back (after "[UserInteraction]" log) Scenario: user navigates back after chart has loaded Given the user is on token details and the chart has fully loaded When user navigates back to the home screen Then no "Token Overview" trace logs appear during navigation back Scenario: user changes time range on the chart Given the user is on token details with the chart loaded on 1D range When user taps on the 1W time range selector Then a "Finishing" log appears for the previous "Token Overview Advanced Chart Initial Visible" span And a new "Starting sampled root span" log appears for "Token Overview Advanced Chart Time Range Visible" And a "Finishing" log appears for the new span when the chart reloads ``` ``` How to verify locally: 1. Enable Sentry trace logging by filtering console output for "Tracing" 2. Navigate to a token's details screen 3. Confirm you see exactly ONE "Starting sampled root span" for "Token Overview Advanced Chart Initial Visible" followed by ONE "Finishing" log for the same span ID 4. Navigate back — confirm no additional trace start/finish for that span 5. Re-enter the token, change time range — confirm the old trace ends with superseded and a new "Time Range Visible" trace starts and finishes ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** <!-- Every checklist item must be consciously assessed before marking this PR as "Ready for review". A checked box means you deliberately considered that responsibility, not that you literally performed every action listed. Unchecked boxes are ambiguous: they are not an implicit "N/A" and they are not a silent "skip". See `docs/readme/ready-for-review.md` for the full checklist semantics. --> - [x] 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). - [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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. #### Performance checks (if applicable) - [ ] I've tested on Android - Ideally on a mid-range device; emulator is acceptable - [ ] I've tested with a power user scenario - Use these [power-user SRPs](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/edit-v2/401401446401?draftShareId=9d77e1e1-4bdc-4be1-9ebb-ccd916988d93) to import wallets with many accounts and tokens - [ ] I've instrumented key operations with Sentry traces for production performance metrics - See [`trace()`](/app/util/trace.ts) for usage and [`addToken`](/app/components/Views/AddAsset/components/AddCustomToken/AddCustomToken.tsx#L274) for an example For performance guidelines and tooling, see the [Performance Guide](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/400085549067/Performance+Guide+for+Engineers). ## **Pre-merge reviewer checklist** <!-- Reviewer checklist items follow the same semantics as the author checklist: an unchecked box is ambiguous, a checked box means the reviewer consciously assessed that responsibility. See `docs/readme/ready-for-review.md`. --> - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] 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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Adds new Sentry tracing lifecycle hooks around the token overview advanced chart and changes how metrics consent is determined during early app startup, which could affect whether performance data is captured or suppressed. > > **Overview** > Adds Sentry performance traces for token overview *advanced chart visibility*, starting a trace on `ohlcvSeriesKey` changes and ending it when the chart skeleton disappears, errors, is superseded by a newer request, falls back to the legacy chart, or unmounts (including `assetId` and truncated error details). > > Extends `AdvancedChart` with an `onSkeletonHidden` callback that fires once when the native skeleton overlay is removed (resetting on series/HTML reload), and adds new `TraceName`/`TraceOperation` constants to distinguish *initial/asset changes* vs *time-range-only* updates. > > Fixes early Sentry init consent detection by updating `hasMetricsConsent()` to read `persist:AnalyticsController` from `redux-persist-filesystem-storage` (fallback to legacy `METRICS_OPT_IN`), and keeps the in-memory consent cache in sync via `updateCachedConsent()` from settings; tests were updated/added for these behaviors. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7ec6bbd. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> [b14edae](b14edae) Co-authored-by: sahar-fehri <sahar.fehri@consensys.net>
1 parent 14e7dee commit a0ea6e4

8 files changed

Lines changed: 579 additions & 3 deletions

File tree

app/components/UI/AssetOverview/Price/Price.advanced.test.tsx

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,15 @@ import { AnalyticsEventBuilder } from '../../../../util/analytics/AnalyticsEvent
1111

1212
jest.mock('../../../hooks/useAnalytics/useAnalytics');
1313

14+
const mockTrace = jest.fn();
15+
const mockEndTrace = jest.fn();
16+
17+
jest.mock('../../../../util/trace', () => ({
18+
...jest.requireActual('../../../../util/trace'),
19+
trace: (...args: unknown[]) => mockTrace(...args),
20+
endTrace: (...args: unknown[]) => mockEndTrace(...args),
21+
}));
22+
1423
const mockSetIsChartBeingTouched = jest.fn();
1524
jest.mock('../PriceChart/PriceChart.context', () => ({
1625
usePriceChart: () => ({
@@ -830,4 +839,203 @@ describe('PriceAdvanced', () => {
830839
expect(mockSetIsChartBeingTouched).toHaveBeenCalledWith(false);
831840
});
832841
});
842+
843+
describe('performance tracing', () => {
844+
beforeEach(() => {
845+
mockTrace.mockClear();
846+
mockEndTrace.mockClear();
847+
});
848+
849+
it('starts initial visibility trace when component mounts with advanced chart', () => {
850+
render(<PriceAdvanced {...baseProps} />);
851+
852+
expect(mockTrace).toHaveBeenCalledWith(
853+
expect.objectContaining({
854+
name: expect.stringContaining('Advanced Chart Initial Visible'),
855+
op: expect.stringContaining('token_overview.advanced_chart'),
856+
}),
857+
);
858+
});
859+
860+
it('ends trace when onSkeletonHidden is called with matching series key', () => {
861+
const { getByTestId } = render(<PriceAdvanced {...baseProps} />);
862+
const advancedChart = getByTestId('mock-advanced-chart');
863+
864+
mockEndTrace.mockClear();
865+
866+
act(() => {
867+
advancedChart.props.onSkeletonHidden?.();
868+
});
869+
870+
expect(mockEndTrace).toHaveBeenCalledWith(
871+
expect.objectContaining({
872+
name: expect.stringContaining('Advanced Chart Initial Visible'),
873+
}),
874+
);
875+
});
876+
877+
it('ends trace with error data when onError is called', () => {
878+
const { getByTestId } = render(<PriceAdvanced {...baseProps} />);
879+
const advancedChart = getByTestId('mock-advanced-chart');
880+
881+
mockEndTrace.mockClear();
882+
883+
act(() => {
884+
advancedChart.props.onError?.('WebView failed to load');
885+
});
886+
887+
expect(mockEndTrace).toHaveBeenCalledWith(
888+
expect.objectContaining({
889+
name: expect.stringContaining('Advanced Chart Initial Visible'),
890+
data: expect.objectContaining({
891+
errorMessage: 'WebView failed to load',
892+
}),
893+
}),
894+
);
895+
});
896+
897+
it('starts time range visibility trace when time range changes', () => {
898+
const { getByTestId } = render(<PriceAdvanced {...baseProps} />);
899+
900+
mockTrace.mockClear();
901+
902+
act(() => {
903+
fireEvent.press(getByTestId('select-1W'));
904+
});
905+
906+
expect(mockTrace).toHaveBeenCalledWith(
907+
expect.objectContaining({
908+
name: expect.stringContaining('Time Range Visible'),
909+
op: expect.stringContaining('time_range'),
910+
}),
911+
);
912+
});
913+
914+
it('supersedes previous trace when series key changes before skeleton hidden', () => {
915+
const { getByTestId } = render(<PriceAdvanced {...baseProps} />);
916+
917+
mockEndTrace.mockClear();
918+
919+
act(() => {
920+
fireEvent.press(getByTestId('select-1W'));
921+
});
922+
923+
expect(mockEndTrace).toHaveBeenCalledWith(
924+
expect.objectContaining({
925+
data: expect.objectContaining({
926+
superseded: true,
927+
}),
928+
}),
929+
);
930+
});
931+
932+
it('ends trace with fallbackToLegacy when switching to legacy chart', () => {
933+
const { rerender } = render(<PriceAdvanced {...baseProps} />);
934+
935+
mockEndTrace.mockClear();
936+
937+
mockUseOHLCVChart.mockReturnValueOnce({
938+
ohlcvData: [],
939+
isLoading: false,
940+
error: undefined,
941+
hasMore: false,
942+
nextCursor: null,
943+
hasEmptyData: true,
944+
});
945+
946+
rerender(<PriceAdvanced {...baseProps} />);
947+
948+
expect(mockEndTrace).toHaveBeenCalledWith(
949+
expect.objectContaining({
950+
data: expect.objectContaining({
951+
fallbackToLegacy: true,
952+
}),
953+
}),
954+
);
955+
});
956+
957+
it('includes assetId in trace data when available', () => {
958+
mockTrace.mockClear();
959+
960+
render(<PriceAdvanced {...baseProps} />);
961+
962+
expect(mockTrace).toHaveBeenCalledWith(
963+
expect.objectContaining({
964+
data: expect.objectContaining({
965+
assetId: expect.any(String),
966+
}),
967+
}),
968+
);
969+
});
970+
971+
it('does not start trace when falling back to legacy chart immediately', () => {
972+
mockTrace.mockClear();
973+
974+
mockUseOHLCVChart.mockReturnValueOnce({
975+
ohlcvData: [],
976+
isLoading: false,
977+
error: undefined,
978+
hasMore: false,
979+
nextCursor: null,
980+
hasEmptyData: true,
981+
});
982+
983+
render(<PriceAdvanced {...baseProps} />);
984+
985+
expect(mockTrace).not.toHaveBeenCalled();
986+
});
987+
988+
it('ends trace with unmounted flag when component unmounts with open trace', () => {
989+
const { unmount } = render(<PriceAdvanced {...baseProps} />);
990+
991+
mockEndTrace.mockClear();
992+
993+
unmount();
994+
995+
expect(mockEndTrace).toHaveBeenCalledWith(
996+
expect.objectContaining({
997+
name: expect.stringContaining('Advanced Chart Initial Visible'),
998+
data: expect.objectContaining({
999+
unmounted: true,
1000+
}),
1001+
}),
1002+
);
1003+
});
1004+
1005+
it('does not end trace on unmount when trace was already completed', () => {
1006+
const { getByTestId, unmount } = render(<PriceAdvanced {...baseProps} />);
1007+
const advancedChart = getByTestId('mock-advanced-chart');
1008+
1009+
act(() => {
1010+
advancedChart.props.onSkeletonHidden?.();
1011+
});
1012+
1013+
mockEndTrace.mockClear();
1014+
1015+
unmount();
1016+
1017+
expect(mockEndTrace).not.toHaveBeenCalled();
1018+
});
1019+
1020+
it('truncates error message to 200 characters', () => {
1021+
const { getByTestId } = render(<PriceAdvanced {...baseProps} />);
1022+
const advancedChart = getByTestId('mock-advanced-chart');
1023+
1024+
mockEndTrace.mockClear();
1025+
1026+
const longError = 'A'.repeat(300);
1027+
1028+
act(() => {
1029+
advancedChart.props.onError?.(longError);
1030+
});
1031+
1032+
expect(mockEndTrace).toHaveBeenCalledWith(
1033+
expect.objectContaining({
1034+
data: expect.objectContaining({
1035+
errorMessage: 'A'.repeat(200),
1036+
}),
1037+
}),
1038+
);
1039+
});
1040+
});
8331041
});

0 commit comments

Comments
 (0)