Perf: Add diagnostic sub-spans to ManualOpenSearchRouter#83076
Perf: Add diagnostic sub-spans to ManualOpenSearchRouter#83076
Conversation
melvin-bot
bot
requested review from
aimane-chnaif and
joekaufmanexpensify
and removed request for
a team and
joekaufmanexpensify
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8be55219db
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| parentSpan.setAttribute('cold_start', !areOptionsInitialized); | ||
| coldStartAttributeSet.current = true; |
There was a problem hiding this comment.
This sets cold_start during the first render and then permanently locks it via coldStartAttributeSet, but useOptionsList() initializes areOptionsInitialized to false and only syncs it from context in an effect (OptionListContextProvider.tsx uses useState(false) then updates in useEffect). On warm opens, this render still sees false, so the ManualOpenSearchRouter span is incorrectly tagged cold_start=true and never corrected, which breaks warm vs cold segmentation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid concern — addressed in ff0acd5. The cold_start attribute was being set during render, before useOptionsList() had synced areOptionsInitialized from context (internal useState(false) always returns false on first render). Moved the attribute setting into a useEffect with [areOptionsInitialized] dependency so it reads the correctly synced value. Warm opens now correctly get cold_start=false.
Add 4 sub-spans and 2 attributes to the ManualOpenSearchRouter Sentry span to identify which phases of the search router opening are bottlenecks. Sub-spans: - SearchRouter.ModalCloseWait: Modal.close() callback latency - SearchRouter.OptionsInit: Cold-path createOptionList() cost (guarded) - SearchRouter.ComputeOptions: JS computation in SearchAutocompleteList (guarded) - SearchRouter.ListRender: FlashList rendering + native layout Attributes: - cold_start: whether options needed initialization - trigger: 'button' or 'keyboard' Co-authored-by: Cursor <cursoragent@cursor.com>
mountiny
force-pushed
the
vit-searchRouter-diagnosticSubSpans
branch
from
8be5521 to
9a89d49
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
Move cold_start attribute setting to useEffect so it reads the synced areOptionsInitialized value instead of the initial useState(false). Add cleanup effect in SearchAutocompleteList to cancel ComputeOptions and ListRender spans if the component unmounts before they complete. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
@aimane-chnaif @JakubKorytko @staszekscp This is ready for a review if you could check it out please and @aimane-chnaif if you can test out the spans work as you would expect in sentry too, thanks! |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-02-28.at.12.17.33.pm.mov |
|
Conflicts |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff0acd5c85
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (isInitialRender && computeSpanStarted.current) { | ||
| endSpan(CONST.TELEMETRY.SPAN_SEARCH_ROUTER_COMPUTE_OPTIONS); | ||
| startSpan(CONST.TELEMETRY.SPAN_SEARCH_ROUTER_LIST_RENDER, { | ||
| name: CONST.TELEMETRY.SPAN_SEARCH_ROUTER_LIST_RENDER, |
There was a problem hiding this comment.
isInitialRender remains true until onLayout runs, so this block executes on every re-render before first layout. Since this component subscribes to many Onyx values, those pre-layout re-renders are common; each pass calls endSpan and then starts SearchRouter.ListRender again, which truncates SearchRouter.ComputeOptions and resets list-render timing, underreporting the real first-open cost in Sentry. Guard this handoff with a one-time ref so it runs exactly once per mount.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid concern — addressed in 8b8ae8d. Added a spanHandoffDone ref that gates the compute→list span handoff so it executes exactly once per mount. Before this fix, every re-render between the first render and onLayout would re-call endSpan(COMPUTE_OPTIONS) and startSpan(LIST_RENDER), resetting the list-render timing.
Additionally, during the merge with main, the loading logic (isRecentSearchesDataLoaded, skeleton) was moved from SearchRouter into SearchAutocompleteList. The loading check now runs before the span handoff as an early return, so the handoff only fires once we're past the loading state and ready to render the list.
The cold_start attribute was also moved from SearchRouter to SearchAutocompleteList since areOptionsInitialized is no longer available in SearchRouter after main's refactor.
Resolve conflicts after main moved loading logic (isRecentSearchesDataLoaded, skeleton) from SearchRouter to SearchAutocompleteList. Also: - Move cold_start attribute setting to SearchAutocompleteList (areOptionsInitialized no longer available in SearchRouter after main's refactor) - Guard compute→list span handoff with spanHandoffDone ref so it runs exactly once per mount (addresses Codex reviewer feedback) - Add eslint-disable for intentional ref-during-render telemetry accesses Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b8ae8d84b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!computeSpanStarted.current && getSpan(CONST.TELEMETRY.SPAN_OPEN_SEARCH_ROUTER)) { | ||
| startSpan(CONST.TELEMETRY.SPAN_SEARCH_ROUTER_COMPUTE_OPTIONS, { | ||
| name: CONST.TELEMETRY.SPAN_SEARCH_ROUTER_COMPUTE_OPTIONS, | ||
| op: 'function', | ||
| parentSpan: getSpan(CONST.TELEMETRY.SPAN_OPEN_SEARCH_ROUTER), |
There was a problem hiding this comment.
SearchRouter.ComputeOptions is started at the top of render before the isLoading early-return path, but it is only ended later in the first non-loading render via the handoff block. On cold opens (areOptionsInitialized is initially false), this makes the compute span include the wait for options initialization/Onyx hydration and overlap with SearchRouter.OptionsInit, so the span breakdown is no longer sequential and the compute timing is materially inflated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid concern — addressed in a3d27e3. The COMPUTE_OPTIONS span now only starts when areOptionsInitialized is true, so on cold opens it waits until after OPTIONS_INIT completes. This eliminates the overlap and ensures the compute span only measures the actual getSearchOptions() computation, not the Onyx hydration wait.
Additionally fixed the cold_start attribute which was still reading from useOptionsList()'s internal useState(false) — warm opens were always tagged as cold. Now reads from OptionsListContext directly to get the correct synchronized value on first render.
1. Guard COMPUTE_OPTIONS span behind areOptionsInitialized so it only starts when real computation happens, avoiding overlap with OPTIONS_INIT on cold opens. 2. Read cold_start from OptionsListContext directly instead of the delayed useOptionsList() internal state, which always starts as false regardless of actual initialization status. Made-with: Cursor
Add ATTRIBUTE_COLD_START and ATTRIBUTE_TRIGGER constants to match the existing pattern for telemetry attribute names, replacing bare string literals. Made-with: Cursor
|
@aimane-chnaif any concerns from your side? |
|
@arosiclair Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4016f14677
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const computeSpanStarted = useRef(false); | ||
| const spanHandoffDone = useRef(false); | ||
| // eslint-disable-next-line react-hooks/refs -- intentional: telemetry span must start during render to measure computation time | ||
| if (!computeSpanStarted.current && areOptionsInitialized && getSpan(CONST.TELEMETRY.SPAN_OPEN_SEARCH_ROUTER)) { |
There was a problem hiding this comment.
This starts SearchRouter.ComputeOptions as soon as options are initialized, even if recentSearchesMetadata is still loading and the component immediately returns the skeleton path. Because the span is only handed off/ended later in the non-loading render, it can include idle wait time and additional renders, which inflates the compute phase and corrupts the diagnostic breakdown this change is intended to provide (especially on cold opens).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 08ed61f. Hoisted isRecentSearchesDataLoaded above the compute span guard so ComputeOptions only starts once all loading states (both areOptionsInitialized and recentSearchesMetadata) have cleared. This prevents idle Onyx hydration time from inflating the span on cold opens and keeps the diagnostic breakdown sequential as intended.
JakubKorytko
left a comment
JakubKorytko
left a comment
There was a problem hiding this comment.
2 questions, no requested changes, looking really good!
| SPAN_SEARCH_ROUTER_MODAL_CLOSE_WAIT: 'SearchRouter.ModalCloseWait', | ||
| SPAN_SEARCH_ROUTER_OPTIONS_INIT: 'SearchRouter.OptionsInit', | ||
| SPAN_SEARCH_ROUTER_COMPUTE_OPTIONS: 'SearchRouter.ComputeOptions', | ||
| SPAN_SEARCH_ROUTER_LIST_RENDER: 'SearchRouter.ListRender', |
There was a problem hiding this comment.
Why are dots in values here?
There was a problem hiding this comment.
The dots are intentional to visually distinguish these as diagnostic sub-spans from the top-level parent spans in Sentry's trace view. The existing parent spans use flat PascalCase (ManualOpenReport, ManualOpenSearchRouter, etc.) while these child spans use the SearchRouter. prefix to make it immediately clear they belong to the SearchRouter flow when browsing traces. It's a common Sentry convention for hierarchical span naming. Happy to change to flat PascalCase (e.g. SearchRouterModalCloseWait) if you feel consistency with the parent span naming is more important.
| op: CONST.TELEMETRY.SPAN_OPEN_SEARCH_ROUTER, | ||
| attributes: { | ||
| trigger: 'keyboard', | ||
| [CONST.TELEMETRY.ATTRIBUTE_TRIGGER]: 'keyboard', |
There was a problem hiding this comment.
Should keyboard value be in the CONSTs as well?
There was a problem hiding this comment.
Good question. The attribute keys (trigger, cold_start) are constants because they're the contract with Sentry and are referenced in multiple files. The attribute values ('keyboard', 'button') are each used exactly once and are self-documenting, so extracting them felt like over-abstracting. That said, it's a trivial change — happy to add TRIGGER_KEYBOARD and TRIGGER_BUTTON constants if you think it's worth it for consistency.
Hoist isRecentSearchesDataLoaded above the compute span guard so ComputeOptions only starts once all loading states have cleared, preventing idle Onyx hydration time from inflating the span on cold opens. Made-with: Cursor
…nosticSubSpans Made-with: Cursor # Conflicts: # src/CONST/index.ts
OptionsListContext was renamed to OptionsListStateContext on main. Update the import and useContext call to match. Made-with: Cursor
|
@arosiclair Ready again |
|
@mountiny conflicts |
…nosticSubSpans Made-with: Cursor # Conflicts: # src/CONST/index.ts
|
@arosiclair Updated |
4 participants
Explanation of Change
The
ManualOpenSearchRouterSentry span currently has no child spans — Sentry shows 100% self-time with no breakdown, making it impossible to identify which phase is the bottleneck. This PR adds 4 diagnostic child spans (linked viaparentSpan) that decompose the parent span into sequential, non-overlapping phases:SearchRouter.ModalCloseWait— Time waiting forModal.close()callback. Identifies if the modal system adds latency before we even start mounting the search UI.SearchRouter.OptionsInit(cold path only) — Time spent increateOptionList(). Only fires when the parent span is active (guarded bygetSpan()check to avoid noise from otheruseOptionsListconsumers).SearchRouter.ComputeOptions— Total JS computation time inSearchAutocompleteList(searchOptions, autocompleteSuggestions, sections assembly). Only measured on first render.SearchRouter.ListRender— FlashList rendering + native layout time (from post-computation toonLayout). Only measured on first render.All sub-spans pass
parentSpan: getSpan(CONST.TELEMETRY.SPAN_OPEN_SEARCH_ROUTER)so they appear as children of the parentManualOpenSearchRouterspan in Sentry's trace view. If the component unmounts before spans complete (e.g., the user closes the search router quickly), a cleanupuseEffectcallscancelSpan()to prevent leaks — these get acanceledattribute to distinguish them from normal completions.The gap between sub-spans (parent minus all children) gives us the React mount + Onyx hydration overhead without requiring a dedicated span.
Additionally, 2 attributes are added to the parent span for Sentry segmentation:
cold_start:true/false— whether options needed initialization when SearchRouter mounted (set viauseEffectto read the correctly synced context value)trigger:'button'/'keyboard'— how the search was opened (button path was previously missing this)Overhead is negligible (~8
Date.now()calls per open). Sub-spans only fire on the initial render, guarded by refs.Fixed Issues
$ #79353
Tests
Offline tests
QA Steps
ManualOpenSearchRouterspans — verify child spans (SearchRouter.ModalCloseWait,SearchRouter.ComputeOptions,SearchRouter.ListRender) appear nested under the parent spancold_startandtriggerattributes are present on the parent spanPR 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