[Payment due @Krishna2323] [No QA] Add receipt prepare span and capture attributes to scan telemetry - #99175
Conversation
|
@MelvinBot review |
Review👍 Looks good. The new Span lifecycle trace (why it's balanced)
Import swap — Optional nits (non-blocking):
Note: this is a static review only. The changed code (camera capture spans) can't be meaningfully exercised in a headless web environment since it requires a real camera device — verification is best done by inspecting the emitted spans in Sentry on a real browser/device. |
|
@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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d61713131a
ℹ️ 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".
|
Left one minor comment, besides that LGTM. |
|
^^ @Krishna2323 kind bump here |
|
@VickyStash sorry for delay, I'll start reviewing within an hour. |
| startSpan(CONST.TELEMETRY.SPAN_RECEIPT_PREPARE, { | ||
| name: CONST.TELEMETRY.SPAN_RECEIPT_PREPARE, | ||
| op: CONST.TELEMETRY.SPAN_RECEIPT_PREPARE, | ||
| parentSpan: getSpan(CONST.TELEMETRY.SPAN_SHUTTER_TO_CONFIRMATION), | ||
| attributes: {[CONST.TELEMETRY.ATTRIBUTE_PLATFORM]: 'native'}, | ||
| }); |
There was a problem hiding this comment.
This block is duplicated in CameraCapture.tsx:133, with only the platform attribute being different. Could we extract a startReceiptPrepareSpan(platform) helper next to startScanProcessSpan, so the span start/end logic stays in the same place? Also, lets use constants for native and web.
There was a problem hiding this comment.
Added startReceiptPrepareSpan util function and reused our getPlatform util function: b28dac3
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariandroid_hybrid.mp4android_mWeb.mp4 |
| name: CONST.TELEMETRY.SPAN_RECEIPT_PREPARE, | ||
| op: CONST.TELEMETRY.SPAN_RECEIPT_PREPARE, | ||
| parentSpan: getSpan(CONST.TELEMETRY.SPAN_SHUTTER_TO_CONFIRMATION), | ||
| attributes: {[CONST.TELEMETRY.ATTRIBUTE_PLATFORM]: getPlatform()}, |
There was a problem hiding this comment.
@VickyStash getPlatform() returns ios/android/mobileweb, but sibling scan spans stamp native/web. Filtering the chain on platform:native will miss this span, pass the platform string in instead? We already have constants for native and web.
There was a problem hiding this comment.
We already have constants for native and web
For web we have CONST.PLATFORM.WEB, but for native, only the capitalized Native string exists as CONST.OS.NATIVE.
Both consts are used in typing (ex: typeof CONST.PLATFORM), so we shouldn't adjust/change it here I think.
Maybe we can add a new TELEMETRY.SPAN_PLATFORM with native and reuse it across spans and siblings to maintain consistency? @Krishna2323 WDYT?
|
🎯 @Krishna2323, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
🚧 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.4.65-0 🚀
|
|
No help site changes are required for this PR. I did not create a draft PR. Why: every change is internal Sentry performance instrumentation. Nothing in the diff alters what a user sees or does.
No user-facing copy, no button or label text, no settings, no new or changed feature behavior. The receipt-scanning articles stay accurate as written — Create an expense and Attach and edit receipts on expenses. If a future PR in this telemetry series changes the camera UI — for instance the flash control, landscape capture, or the multi-scan flow — that one would need a docs review. @VickyStash, there is no help site PR to link because no docs changes are needed. If you disagree and want an article updated, reply with what should change and I'll open the draft PR. |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.4.65-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Add receipt prepare span and capture attributes to scan telemetry
ManualReceiptPreparespan covers the previously un-instrumented gap between capture end and scan processing start (receipt adoption, buildReceiptFiles), closing theManualShutterToConfirmationchain.capture_method(takePhotovstakeSnapshot),flash_used, and photo dimensions onManualReceiptCaptureto explain the native capture tail.Fixed Issues
$ #99773
PROPOSAL: N/A
Tests
Offline tests
N/A
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)Avatar, 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.