[No QA] Fix mixed clock domains in span duration logs - #98152
Conversation
|
@eVoloshchak 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] |
What is |
roryabraham
left a comment
There was a problem hiding this comment.
I think this code looks good, but I'm confused by the testing steps
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
mountiny
left a comment
There was a problem hiding this comment.
If I understand this right, it only influences dev measurements, right? Sentry spans are fine
Oh sorry, i use a tool called |
@mountiny yes, exactly |
|
@roryabraham looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
tests were passing? |
|
🚧 roryabraham 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/roryabraham in version: 9.4.53-0 🚀
|
|
🤖 No help site changes required. This PR is a purely internal telemetry fix. It changes only:
Both changes affect only @chrispader, if you believe any of this should be reflected on the help site (e.g. a behavior I've mis-scoped as internal-only), let me know what user-facing surface it maps to and I'll draft the docs update. |
@roryabraham @mountiny @tgolen
Explanation of Change
Sentry span start times can be Unix epoch timestamps while
performance.now()is relative to the process start. The existing lifecycle log subtracted those different clock domains directly, producing very large negative durations for native startup spans.This change converts the supplied Sentry start time into the
performance.now()clock domain once when tracking begins. Duration calculations then remain monotonic, while the ending log uses an epoch timestamp consistently with the starting log. A unit test covers a native timestamp that predates JS initialization.Further used by #98144 to improve local benchmark tooling.
Fixed Issues
$ #98280
PROPOSAL:
Tests
npm run test tests/unit/ActiveSpansTest.ts --runInBand --watchman=false.750msduration for a span that starts 500ms before JS initialization and ends 250ms afterward.npm run lint-changedand verify lint passes.npm run typecheck-tsgoand verify type checking passes.Offline tests
Offline testing is not applicable because this only corrects an in-memory timestamp calculation and performs no network access.
QA Steps
No QA is required. The behavior is internal telemetry logging and is covered by the regression test above.
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
No screenshots or videos are included because this change has no user-interface impact.