Repository navigation
[NOQA] fix: start telemetry spans with no parent - #101877
Conversation
|
@Krishna2323 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] |
|
Can review if a C+ review is required. |
|
PR doesn’t need product input as a performance tooling PR. Unassigning and unsubscribing myself. |
Krishna2323
left a comment
There was a problem hiding this comment.
The description still says parentSpan: null, but the code uses forceTransaction, which keeps the trace link. Please update it and fill in the Tests and QA sections.
| startTimeForLog: number; | ||
| }; | ||
|
|
||
| type TrackedSpanOptions = StartSpanOptions & {inheritScope?: boolean}; |
There was a problem hiding this comment.
NAB: Can we drop inheritScope and have OnyxDerived pass forceTransaction: false? The ?? already respects it, and the other five callers set forceTransaction directly.
| parentSpan, | ||
| // A span with no parent is sent as its own transaction, one per recompute. | ||
| onlyIfParent: true, | ||
| inheritScope: true, |
There was a problem hiding this comment.
NAB: forceTransaction: false here does the same thing through Sentry's own option. That lets us delete TrackedSpanOptions and the destructuring in startSpan.
|
|
||
| import type {StartSpanOptions} from '@sentry/core'; | ||
|
|
||
| const mockStartInactiveSpan = jest.fn((options: StartSpanOptions) => ({ |
There was a problem hiding this comment.
NAB: Could this match the typed mock in ActiveSpansBenchmarkTest.ts:17 (jest.fn<MockInactiveSpan, [StartSpanOptions]>)? The options field on the fake span also looks unused.
|
|
||
| it('lets a caller opt back into the span on the scope', () => { | ||
| // Given a caller that asks to inherit, as the Onyx derived recomputes do because they are short and want whatever transaction is open | ||
| const options = {name: 'InheritingSpan', inheritScope: true}; |
There was a problem hiding this comment.
NAB: This never runs OnyxDerived, so removing its opt-out wouldn't fail anything. Could we assert forceTransaction is false next to onlyIfParent in onyxDerivedComputeSpanTest.ts:93?
Applied all suggestions and updated the PR description. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@Julesssss friendly bump |
|
Sorry, been delayed by app deploy this week 😩 |
|
🚧 Julesssss 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/Julesssss in version: 9.5.2-0 🚀
|
|
No help site update is needed. This PR only changes how internal Sentry telemetry spans start, so nothing in the app looks or works differently for users. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.2-4 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
startSpaninsrc/libs/telemetry/activeSpans.tscreates everyManual*span. When the caller gives no parent, Sentry attaches the new span to whatever span is active on the scope.startSpannow passesforceTransaction: truein that case, so the span starts as its own transaction. It still carries the trace ID and parent span ID, so Sentry keeps it on the same trace.forceTransactionalways wins.OnyxDerivedsets it tofalsebecause its recomputes are short, useonlyIfParent, and should join the open transaction instead of each sending one of their own.Root cause
Since the Sentry SDK upgrade in #100711, a tap leaves a
ui.action.touchidle span on the scope, and our spans became its children. That idle span ends about a second after the last activity under it, and@sentry/corethen force-ends any child still running:The span is cut short and marked
span.status:cancelled. Dashboards filterspan.status:ok, so the capture never reaches a chart at all.Why
ManualShutterToConfirmationis hit hardestIt is tap-triggered, so an idle span is on the scope when it opens. It stays open across capture, navigation and mount, so it is almost always still running when that idle span closes, and it loses 84% against 54% for
ManualConfirmationMount. It is also mobile-only, so nothing dilutes the loss:ManualOpenReportloses 67% on mobile too, but desktop dominates its board figure and it barely moved.The board number went up rather than down because iOS lost 77% of captures against Android's 20%, leaving a pool that went from 44% to 52% Android while Android runs about 2.5x iOS here. On a build from before the upgrade, iOS p75 went 166 to 165 and Android p75 went 315 to 313 week over week.
Fixed Issues
$ #102071
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 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