[Metrics] Add submit-to-destination-visible span for expense creation flows#84069
Conversation
|
@codex review |
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.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b646417f8a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Review
Overall this looks solid — the concept of measuring end-to-end submit-to-destination-visible is valuable, and the destination-type bucketing will make the data actionable. A few points to consider:
Major: NVP_ prefix
NVP_PENDING_EXPENSE_CREATE_DESTINATION is purely local telemetry state, never synced to the server. The NVP_ prefix conventionally denotes server-synced Name-Value Pairs in this codebase. There is some precedent for local-only keys using this prefix (e.g. NVP_DELETE_TRANSACTION_NAVIGATE_BACK_URL), but dropping the prefix would be more accurate.
RHP_POP on narrow screens — does onLayout actually fire?
When Navigation.pop(rhpKey) runs, the underlying ReportScreen is already mounted and its wrapper View is already laid out. onLayout only fires when a View's dimensions change. On narrow/mobile screens, the RHP is a full-screen overlay — popping it doesn't change the underlying ReportScreen's layout dimensions, so onLayout likely won't fire and the span would never be ended for the rhp_pop destination type.
On wide screens the RHP occupies side space, so popping it may change available width and trigger onLayout. But on mobile this seems like it could be a gap. Worth verifying with a quick test on a narrow screen.
Minor: destinationType in OnyxValuesMapping is typed as plain string
In ONYXKEYS.ts, the type is {destinationType: string; reportID?: string}. Consider using DestinationType (ValueOf<typeof CONST.TELEMETRY.DESTINATION_TYPE>) instead of string for type safety — it would catch typos and make the allowed values discoverable.
Minor: re-export indirection
IOU/Split.ts imports setPendingExpenseCreateDestination from @libs/telemetry/markSubmitToDestinationVisibleEnd rather than from @libs/actions/ExpenseCreateDestination (the canonical source). This blurs the action/telemetry boundary. Consider importing directly from the action file.
|
Also is there a way to avoid using the onyx key to keep track of the span? |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db788bb84a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Thanks for the review @mountiny Major: NVP_ prefix RHP_POP on narrow screens Minor: destinationType typing Minor: re-export indirection Avoiding the Onyx key Follow-up: Set pending destination for split-with-receipt submits Follow-up: Validate destination before closing submit-visible span |
|
@QichenZhu 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2955c6a0a6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
PR doesn’t need product input as a Sentry metrics PR. Unassigning and unsubscribing myself. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@codex review |
|
Hi, @mountiny @QichenZhu the PR is ready for another review. I changed the structure a bit - refactored from Videos of the particular flows are in the PR videos section instead of screenshots. I've tested it with Reason was to segment by what actually happened after submit (opened a report, went to Search, or just dismissed) as it matters the most rather than by screen names. I also fixed a two edge cases along the way:
|
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d178715622
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Hard to say from the screenshots but that is possible |
Reviewer Checklist
Screenshots/VideosAndroid: mWeb Chrome1. dismiss_modal_and_open_report
2. navigate_to_search
####3. dismiss_modal_only
MacOS: Chrome / Safari1. dismiss_modal_and_open_report
2. navigate_to_search
####3. dismiss_modal_only
|
QichenZhu
left a comment
There was a problem hiding this comment.
Just added some minor style suggestions. @staszekscp's and codex's comments are more important though.
I've seen that before too, but not only in this PR and also had a negative number two times (-X ms). You can see on the screenshot you sent that But I will of course address the PR comments. |
|
@mountiny all comments addressed, @QichenZhu may you please take a final look and complete the checklist? |
mountiny
left a comment
There was a problem hiding this comment.
Going to move this forward as all the requested changes seem to be addressed
|
🚧 @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! 🧪🧪
|
|
FYI found RCA of huge/negative Sentry duration values, posted here: #83495 (comment) |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.35-0 🚀
|
|
Hi @mountiny @JakubKorytko QA team can't check Sentry. Is it internal PR? |
|
@izarutskaya yes, we can QA this internally |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.35-1 🚀
|



















Explanation of Change
Adds a new Sentry span that measures the full time from when the user submits an expense until the destination page is fully rendered and visible. This addresses the gap where we had no end-to-end visibility for most post-creation navigation paths.
What's measured
IOURequestStepConfirmation.tsx), or from Send Money (Amount step, skip confirmation) or Send Invoice (Company Info step) when those flows submit without going through confirmation.onLayoutof the primary list/view), so the user can see the result.Coverage
ManualSubmitToDestinationVisible(constant:SPAN_SUBMIT_TO_DESTINATION_VISIBLE), started with the same attributes as the existing submit span (scenario, has_receipt, from_global_create, iou_type, request_type). The span is tagged withsubmit_follow_up_action(one of:dismiss_modal_and_open_report,navigate_to_search,dismiss_modal_only) so we can segment by outcome.ReportScreenorSearchMoneyRequestReportPagewhen they gain focus or layout (report or recipient chat became visible).markNavigateAfterExpenseCreateEnd).dismissModalAndOpenReportInInboxTabafterInteractionManager.runAfterInteractions, or whenReportScreen/SearchMoneyRequestReportPageis already visible and we only dismissed the modal (they end the span in their focus/layout callbacks).How destination is known
submitFollowUpAction.ts(no Onyx). Before each post-expense-create navigation we callsetPendingSubmitFollowUpAction(followUpAction, reportID?). We only set pending (and run span-ending logic) when the span was actually started - e.g. Send Money or Send Invoice from a step that starts the span; flows that never start the span (e.g. skip-confirmation Send Money before we added span start there) do not set pending.getPendingSubmitFollowUpAction()in their callback (e.g.useFocusEffectoronLayout); when the result matches they callendSubmitFollowUpActionSpan(followUpAction, reportID), which clears the pending state and sets thesubmit_follow_up_actionattribute on the span.startSplitBill()path (e.g. from the confirmation step with receipts) sets the pending action toDISMISS_MODAL_AND_OPEN_REPORTbefore callingNavigation.dismissModalWithReport, so the span is ended when the report screen appears.endSubmitFollowUpActionSpanonly ends the span if the passedfollowUpAction(andreportIDwhen applicable) matches the current pending action; this avoids races and keeps attribution correct.cancelSubmitFollowUpActionSpan()when the pending action isNAVIGATE_TO_SEARCH(e.g. when bailing to error/empty/chart view). If the pending action isDISMISS_MODAL_ONLY, we don’t cancel - the deferredendSubmitFollowUpActionSpan(DISMISS_MODAL_ONLY)indismissModalAndOpenReportInInboxTabwill end the span.Existing spans (
SPAN_SUBMIT_EXPENSE,SPAN_NAVIGATE_AFTER_EXPENSE_CREATE) are unchanged; this adds a single end-to-end metric for all expense types and all destinations.Fixed Issues
$ #83634
PROPOSAL: N/A
Tests
Submit follow-up action types (what the span’s
submit_follow_up_actionattribute means):**dismiss_modal_and_open_report** - You were somewhere else in the app and the app re-navigated you to a report (e.g. report chat or money request report). The modal was dismissed and a report screen became visible.**navigate_to_search** - You were not on Inbox; the app navigated you to the Search/Reports page and the new expense is shown there.**dismiss_modal_only** - Submitting only dismissed the modal (e.g. you were already on the target report or on Search ROOT / Expenses). No new screen was opened; you stayed where you were or the modal just closed. (If you were on another Search sub-tab, e.g. Chats, the app navigates to Search ROOT/Expenses, so that isnavigate_to_search.)Test cases
ManualSubmitToDestinationVisiblespan withsubmit_follow_up_action: dismiss_modal_and_open_report, and when applicablereport_id, plus the submit attributes (scenario, has_receipt, from_global_create, iou_type, request_type).ManualSubmitToDestinationVisiblespan withsubmit_follow_up_action: navigate_to_searchand the submit attributes above.ManualSubmitToDestinationVisiblespan withsubmit_follow_up_action: dismiss_modal_onlyand the submit attributes above.navigate_to_search.Note: When using a recent action in step 2, the app may redirect to Inbox instead of the Search/Reports page. That does not mean the PR is broken - the span correctly attributes the outcome (e.g.
dismiss_modal_and_open_reportwhen a report opens, notnavigate_to_search).Offline tests
Same as tests though data is only sent to Sentry while being online.
QA Steps
Same as tests.
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
Example flow runs
dismiss_modal_and_open_reportdismiss_modal_and_open_report.mp4
dismiss_modal_onlydismiss_modal_only.mp4
navigate_to_searchnavigate_to_search.mp4