[Sentry] add span for transition from od nd#75557
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
No product considerations, removing my review |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.2.62-0 🚀
|
There was a problem hiding this comment.
Hi! regarding the HybridAppHandler and manual OD -> ND transition
This component is always used on mobile platforms. It doesn't matter if the user tapped Try New Expesnify button or not
We handle initializing onyx with some data from OD here, mostly sign-in related. It needs to happen because we always open OD first on mobile and then run ND (React Native) on top of it.
So what we're currently measuring here is time required for fetching data from the native side with getHybridAppSettings and then processing it in setupNewDotAfterTransitionFromOldDot
This doesn't measure the time needed for ManualOdNdTransition (if I correctly understand what it should do and mean, please correct me if I'm wrong)
Also startSpan can be called multiple times (it's inside useEffect with some dependencies) but the endSpan will be called only once because getHybridAppSettings will return data only once in entire ND lifecycle (see comment). We might end up starting multiple spans but end only one
There was a problem hiding this comment.
@war-in thanks for your input.
startSpan should handle multiple span starts (but not properly for this case so we need some checks), but I wonder if there is any chance that startSpan could be fired after endSpan is called?
There was a problem hiding this comment.
Yes, it definitely is
We had to add this check fix that issue. Probably there is room for improvement in that component
if (!hybridAppSettings) {
// Native method can send non-null value only once per NewDot lifecycle. It prevents issues with multiple initializations during reloads on debug builds.
Log.info('[HybridApp] `getHybridAppSettings` called more than once during single NewDot lifecycle. Skipping initialization.');
return;
}
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.62-5 🚀
|

Explanation of Change
PR introduces Manual metric to measure time to transition from OD to ND and send it to Sentry.
Fixed Issues
$#74130
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
MacOS: Desktop