[No QA] feat: Add Sentry span for OpenApp/ReconnectApp network request on app startup#90667
Conversation
melvin-bot
Bot
requested review from
chiragsalian and
flaviadefaria
and removed request for
a team
|
@chiragsalian 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: 551389ddf3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds a Sentry telemetry span that measures the duration from native app process start (captured via the Nitro AppStartTime module) until completion of the initial OpenApp/ReconnectApp network request, to quantify the benefit of react-native-nitro-fetch prefetching introduced in #88110.
Changes:
- New
appStartupNetworkRequestSpan.tsmodule exposingmarkStart/markEndfor a Sentry inactive span tagged with the issued API command and a wall-clock duration since native start. activeSpans.startSpannow accepts an optionalnativeAppStartTimeEpochMs, converts it to aperformance-relative origin, and starts the startup-network span when theManualAppStartupspan begins.HttpUtils.processHTTPRequestextracts the command once and callsmarkEndAppStartupNetworkRequestSpanin.finally(...)to close the span on firstOPEN_APP/RECONNECT_APPcompletion.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libs/telemetry/appStartupNetworkRequestSpan.ts | New module managing the startup-network Sentry span lifecycle. |
| src/libs/telemetry/activeSpans.ts | Adds nativeAppStartTimeEpochMs option and triggers the startup-network span start. |
| src/libs/HttpUtils.ts | Hoists the API command parse and ends the startup-network span in .finally. |
| src/libs/AppStartupNetworkRequest.ts | New constant set of startup network commands (OPEN_APP, RECONNECT_APP). |
| src/CONST/index.ts | Adds SPAN_APP_STARTUP_LAST_NETWORK and ATTRIBUTE_DURATION_SINCE_NATIVE_APP_STARTUP_MS telemetry constants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
chrispader
changed the title
OpenApp/ReconnectApp network request on app startupOpenApp/ReconnectApp network request on app startup
|
I'll make a couple more changes to this PR |
| } | ||
| activeSpans.set(spanId, {span, startTime: performance.now()}); | ||
|
|
||
| let startTimeForLog: number; |
There was a problem hiding this comment.
I've changed the logging to also take into account the startTime passed to the span options. For the ManualAppStartup and ManualAppStartupNetworkRequest spans, we don't use performance.now() as the start timestamp, but the native app startup timestamp.
There was a problem hiding this comment.
In any other case, when a startTime is provided, we should also log the duration based on that.
chrispader
changed the title
OpenApp/ReconnectApp network request on app startupOpenApp/ReconnectApp network request on app startup
|
If this requires C+ review, I can help so we can unblock #88110 |
|
@mkhutornyi yes please can you review and check if the span works as expected in sentry? thanks |
roryabraham
changed the title
OpenApp/ReconnectApp network request on app startupOpenApp/ReconnectApp network request on app startup
|
@mkhutornyi did you get to test whether this works in Sentry? 🙌🏼 It would be awesome to get this merged this week and in this deployment, so we can already gather some data this week, so we can merge the NitroFetch PR for the deployment next week |
|
on this today |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
| @@ -64,7 +73,7 @@ function cancelSpan(spanId: string) { | |||
| entry.span.setAttribute(CONST.TELEMETRY.ATTRIBUTE_CANCELED, true); | |||
| // In Sentry there are only OK or ERROR status codes. | |||
| // We treat canceled spans as OK, so we can properly track spans that are not finished at all (their status would be different) | |||
| entry.span.setStatus({code: 1}); | |||
| entry.span.setStatus({code: CONST.TELEMETRY.SPAN_STATUS_CODE.OK}); | |||
There was a problem hiding this comment.
I prefer built-in SPAN_STATUS_OK imported from '@sentry/core'
|
Please pull main |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks! Since @roryabraham already approved, I will merge this so Chris can work on the nitro fetch PR
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.80-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This PR adds internal Sentry telemetry spans for measuring |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.81-2 🚀
|
@roryabraham @mountiny
Explanation of Change
This PR adds a sentry span that measures from native app startup (before JS runtime is started) up until the completion of the initial network request on app startup (
OpenApporReconnectApp)This span is introduced to measure the performance benefits of using
react-native-nitro-fetchinstead of vanilla JSfetch(), added in #88110.Instead of relying on the starting mark that we usually use for spans in Sentry - which is only after the JS bundle has started executing - we rely on the native app startup time from the
AppStartTimeModulenitro module. In order to see the actual benefit of prefetching with NitroFetch, we have to start measuring from before the JS bundle executes and before the queries have been prefetched natively.Fixed Issues
$ #90671
PROPOSAL:
Tests
Make sure the span appears in Sentry.
Offline tests
None needed.
QA Steps
None needed.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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