[NOQA] Sentry: Add spans for expense creation flow#82287
[NOQA] Sentry: Add spans for expense creation flow#82287marcaaron merged 5 commits intoExpensify:mainfrom
Conversation
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.
|
OlimpiaZurek
force-pushed
the
sentry-create-expense-flow-improvements
branch
from
1ccaf5f to
f008619
Compare
OlimpiaZurek
changed the title
OlimpiaZurek
changed the title
| * Middleware that tracks server round-trip time for expense creation commands via Sentry spans. | ||
| * For non-expense commands, this is a no-op pass-through. | ||
| */ | ||
| const ExpenseServerTiming: Middleware = (response, request) => { |
There was a problem hiding this comment.
cannot we use built-in http.client spans for this?
There was a problem hiding this comment.
I think http.client might not be enough in this case. The reason for the custom middleware is jsonCode , it's used in the response body to determine whether successData or failureDatagets applied to Onyx. http.client can't see inside the response body, so it can't distinguish a confirmed expense from a rolled-back one.
|
@bernhardoj 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}
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
This LGTM. What's the next steps?
src/libs/API/index.ts
Outdated
| addMiddleware(Pagination); | ||
|
|
||
| // ExpenseServerTiming - Tracks server round-trip time for expense creation commands via Sentry spans. | ||
| addMiddleware(ExpenseServerTiming); |
There was a problem hiding this comment.
NAB, I am curious if we should make this more generic like SentryServerTiming so that if other metrics are added in the future that we don't end up with many flow specific middlewares.
There was a problem hiding this comment.
Makes sense! Renamed to SentryServerTiming and made it data-driven, there's now a TRACKED_COMMAND_GROUPS array, so adding a new flow is just adding an entry there instead of creating a whole new middleware
| }); | ||
|
|
||
| // IMPORTANT: Every branch below must call markSubmitExpenseEnd() after dispatching the expense action. | ||
| // This ensures the telemetry span started above is always closed, including inside async getCurrentPosition callbacks. |
There was a problem hiding this comment.
How bad is this if this does not happen? We could perhaps devise a way to throw if someone adds a new branch without reading this comment - but probably unnecessary if this is benign.
There was a problem hiding this comment.
Not bad at all, just a missing span in Sentry. I also added a getSpan() guard in markSubmitExpenseEnd itself so it safely no-ops if called without a prior start. I think the // IMPORTANT comment is enough to catch missing calls in review.
|
@sosek108 all yours 🙇 |
| const onLayout = useCallback(() => { | ||
| endSpan(CONST.TELEMETRY.SPAN_NAVIGATE_TO_REPORTS_TAB); | ||
| endSpan(CONST.TELEMETRY.SPAN_NAVIGATE_TO_REPORTS_TAB_RENDER); | ||
| markNavigateAfterExpenseCreateEnd(); |
There was a problem hiding this comment.
Is it fine to call Performance.markEnd without ever starting it?
There was a problem hiding this comment.
It won't break anything, but it was running on every Search layout for no reason. Added a getSpan() check so it skips entirely when there's no active span.
| if (formHasBeenSubmitted.current) { | ||
| return; | ||
| } | ||
| cancelSpan(CONST.TELEMETRY.SPAN_SUBMIT_EXPENSE); |
There was a problem hiding this comment.
If we close the page before submitting the form, the span is never started. If we close the page after submitting the form, formHasBeenSubmitted.current is true. So, I think this does nothing?
There was a problem hiding this comment.
You're right, removed.
| const hasReceiptFiles = Object.values(receiptFiles).some((receipt) => !!receipt); | ||
| const isFromGlobalCreate = transaction?.isFromGlobalCreate ?? transaction?.isFromFloatingActionButton ?? false; | ||
|
|
||
| const {SUBMIT_EXPENSE_SCENARIO} = CONST.TELEMETRY; |
There was a problem hiding this comment.
NAB, I think this bloated the createTransaction function. Can we move it to a function outside the component?
There was a problem hiding this comment.
Agreed, moved it to src/libs/telemetry/getSubmitExpenseScenario.ts.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Filled out the author checklist as this looks ready to merge and we need this to continue work on this issue. |
|
🚧 @marcaaron 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! 🧪🧪
|
|
✋ 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/marcaaron in version: 9.3.24-1 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.24-3 🚀
|
5 participants
Explanation of Change
ManualSubmitExpensespan measuring confirm tap → API dispatch, tagged with scenario attribute (e.g. request_money_manual, distance, split, track_expense, invoice) to distinguish flows that were previously mixed under one broadManualOpenCreateExpenseManualNavigateAfterExpenseCreatespan measuring post submit navigation to the Search pageExpenseServerTimingmiddleware that creates a ManualExpenseServerResponse span for the expense write commandsFixed Issues
$ #82993
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 methodAvatar, 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