[No QA] fix: introduce useStartSpansOnRender hook that registers spans on first render - #97885
Conversation
useStartSpansOnRender hook that registers spans on first renderuseStartSpansOnRender hook that registers spans on first render
roryabraham
left a comment
There was a problem hiding this comment.
LGTM, great find 👍🏼
@MelvinBot Are there other places where we're starting spans in effects and ending them in effects of a child component?
|
@roryabraham answering the question Melvin missed
I think Here is my bot's analysis
|
|
PR doesn’t need product input as a perf tooling PR. Unassigning and unsubscribing myself. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.53-0 🚀
|
|
🤖 No help site changes required — no draft PR created. This PR is a Both changed files are internal observability code with no user-facing impact:
There are no new or changed features, UI, settings, tab names, buttons, workflows, billing, or integration behavior — nothing that the help site articles under @war-in, if you believe there is a user-facing behavior change I've missed that the help site should cover, let me know and I'll draft the docs PR. |
Explanation of Change
NavigationRootReadywas started in auseEffectbut is ended fromNavigationContainer'sonReady, which React flushes first — so the end was dropped and the span leaked, landing ascancelledon 36–53% of native traces (single span evidence, aggregated spans that didn't finish) . NewuseStartSpansOnRenderhook registers spans during render instead, which also makes the span cover the navigator's render cost.Fixed Issues
$
PROPOSAL:
Tests
npm run ios/npm run android).[Sentry][NavigationRootReady].[Sentry][NavigationRootReady] Starting span[Sentry][NavigationRootReady] Ending span (Nms)main(before this change) only theStarting spanline appears on native and theEnding spanline is never logged, because the end ran before the span was registered and was silently dropped.Offline tests
N/A
QA Steps
N/A - telemetry change
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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