[NoQA][Sentry] Adds a metric with effective render time of Reports tab#79455
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Pull request overview
This PR adds a new Sentry metric ManualOnLayoutContentReports to measure the effective render time of the Reports tab, specifically tracking the time between when data is available and when it's actually displayed to the user. This provides a cleaner metric for UI performance analysis compared to the existing ManualNavigateToReportsTab metric, which includes network request time.
Changes:
- Added new telemetry constant
SPAN_ON_LAYOUT_CONTENT_REPORTS - Implemented span lifecycle management in the Search component: started when skeleton completes rendering, ended when actual content renders
- Added cancel calls for the new span in all error/empty state paths
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/CONST/index.ts | Adds the new telemetry span constant SPAN_ON_LAYOUT_CONTENT_REPORTS |
| src/components/Search/index.tsx | Implements span lifecycle: starts in onLayoutSkeleton, ends in onLayout, and cancels in error/empty paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/Search/index.tsx
Outdated
| startSpan(CONST.TELEMETRY.SPAN_ON_LAYOUT_CONTENT_REPORTS, { | ||
| name: CONST.TELEMETRY.SPAN_ON_LAYOUT_CONTENT_REPORTS, | ||
| op: CONST.TELEMETRY.SPAN_ON_LAYOUT_CONTENT_REPORTS, | ||
| }); |
There was a problem hiding this comment.
The span SPAN_ON_LAYOUT_CONTENT_REPORTS is only started in onLayoutSkeleton, which is only called when shouldShowLoadingState is true (line 962). However, if the data is already loaded when the component renders and shouldShowLoadingState is false from the start, the skeleton is never shown, onLayoutSkeleton is never called, and this span is never started. Yet the onLayout callback on line 950 will still try to end this span. This means the metric won't be captured in scenarios where data loads quickly or is already cached, which could be a significant portion of user interactions.
Consider starting the span earlier in the component lifecycle (e.g., where SPAN_ON_LAYOUT_SKELETON_REPORTS is started in NavigationTabBar) or conditionally starting it in the normal render path when shouldShowLoadingState is false, to ensure the metric is captured in all cases.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
e424710 to
5e7174c
Compare
5e7174c to
4134d24
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
No product considerations. Removing my review |
|
It looks like Reassure Performance Tests / perf-tests (pull_request) is failing. Please try merging main to fix it. Ping me when this PR is ready. |
|
@huult done. The check looks fine now. |
|
@leshniak Please resolve the conflict. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-01-15.at.10.39.05.mp4Android: mWeb ChromeScreen.Recording.2026-01-15.at.10.37.56.mp4iOS: HybridAppScreen.Recording.2026-01-15.at.10.42.04.mp4iOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-01-15.at.09.59.26.mp4 |
| inputQuery: queryJSON?.inputQuery, | ||
| }); | ||
| } | ||
| }, [shouldShowLoadingState]); |
There was a problem hiding this comment.
| }, [shouldShowLoadingState]); | |
| // Exclude `queryJSON?.inputQuery` since it’s only telemetry metadata and would cause the span to start multiple times. | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [shouldShowLoadingState]); |
Please add // eslint-disable-next-line react-hooks/exhaustive-deps if we intentionally don’t want to add queryJSON?.inputQuery to the dependency array, and add a comment explaining why this rule is disabled.
Screen.Recording.2026-01-15.at.09.59.26.movIt looks like it works. Currently, we measure the duration only when data is available, empty and error states are skipped. |
|
I've addressed the comments, conflicts are fixed too. |
|
@leshniak please resovle the conflict |
Resolved |
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.3.3-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.3.3-8 🚀
|
Explanation of Change
The current metric
ManualNavigateToReportsTabincludes the time of API requests, which incorporates all network-related conditions into the metric. Although it is a good overall measure of UX, it is a noisy metric for tracking UI issues. This metric measures the time between the points when we already have the data and when we actually display them to the user, to provide a better picture for further analysis.Fixed Issues
$ #77174
PROPOSAL:
Tests
I've checked using
console.logif these points actually measures the desired time.Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari