[No QA] [Sentry] OpenReport metrics#74961
Conversation
|
|
||
| function startSpan(spanId: string, options: StartSpanOptions) { | ||
| // End any existing span for this name | ||
| endSpan(spanId); |
There was a problem hiding this comment.
Should we instead delete the current open span if it never really finished?
There was a problem hiding this comment.
From Deveelopers perspective we should just delete it so the Sentry will handle its timeout (optimistically) but having many unfinished spans could harm performance.
So very valid question - I'll try to find best answer to this
There was a problem hiding this comment.
I think we should at least add an attribute to it, like cancelled or something, so we can filter them out, but we can do that in a follow up.
There was a problem hiding this comment.
I've noted that as next step on the issue. This is actually great idea 🎉
There was a problem hiding this comment.
I think that not always we'd want to cancel span. Sometimes maybe better approach is to reuse existing span?
For example: User clicks a LHN row and waits for report to open, in the meantime he clicks again on that row -> should we cancel first one?
|
🚧 @rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
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.
|
TODO
|
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
sosek108
changed the title
melvin-bot
Bot
requested review from
JmillsExpensify and
jjcoffee
and removed request for
a team
| } | ||
|
|
||
| // Start a Sentry span for report navigation | ||
| if (route.startsWith('r/') || route.startsWith('search/r/')) { |
There was a problem hiding this comment.
cc @WojtekBoman We might need to add any new report route here
| } | ||
|
|
||
| // Start a Sentry span for report navigation | ||
| if (route.startsWith('r/') || route.startsWith('search/r/')) { |
There was a problem hiding this comment.
I think starting a span here won't cover the majority of event listeners where the actual call to navigate() happens really late (which is very often the case).
There was a problem hiding this comment.
I've noted that as next step
| import type {StartSpanOptions} from '@sentry/core'; | ||
| import * as Sentry from '@sentry/react-native'; | ||
|
|
||
| const activeSpans = new Map<string, ReturnType<typeof Sentry.startInactiveSpan>>(); |
There was a problem hiding this comment.
Isn't this creating a pretty bad precedence? This portion of code (and storing in memory) is very imperative and forces us to start keeping track (also solving bugs) with manually handling this Map. This should all live in the SDK I think and not be handled by the app.
There was a problem hiding this comment.
We have two major roads to take:
- one is to use automatic navigation spans
- one is to use manual spans.
Automatic navigation spans proved to not be so useful as we don't have power on them. I've tested it and some of them counted two different navigations into one, some of them ends after some actions in background finish that are not relevant for user perceived performance (eg. /api/Log)
The API of manual spans is callback based so it looks like this:
Sentry.startSpanManual({}, (span, finish) => {
// here goes the logic which is measured
finish() // call finish to close and send span
})This does not work for our navigation because navigation is finished before heavy logic in the target component.
Most of cases I think will be enough to put into callback but some of them needs to be stored by us. I don't see using Map as a bad practice myself. But I'm open for discussion
There was a problem hiding this comment.
Thinking outside of the box of 2 options, there's also a 3rd one with autoinstrumenting pressables. Have we looked into that?
There was a problem hiding this comment.
want me to do this right now or as follow up?
| */ | ||
| function markOpenReportEnd() { | ||
| function markOpenReportEnd(reportId: string) { | ||
| endSpan(`${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${reportId}`); |
There was a problem hiding this comment.
This accounts only for this main event, so doesn't allow us to parametrize the origin of this action (which UI part triggered navigation). Do we want to cover that later?
There was a problem hiding this comment.
I plan to add this later. So we have quick very naive win with "we track something" and then we can go to more precise tracking.
There was a problem hiding this comment.
The span name shouldn't contain the reportId. Can you pass it in as a span parameter instead?
There was a problem hiding this comment.
I think this should also not include _<id>, am I wrong?
There was a problem hiding this comment.
this is just for internal reference, not being sent to sentry
|
|
||
| function startSpan(spanId: string, options: StartSpanOptions) { | ||
| // End any existing span for this name | ||
| endSpan(spanId); |
There was a problem hiding this comment.
I think we should at least add an attribute to it, like cancelled or something, so we can filter them out, but we can do that in a follow up.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No review required from product.
|
✋ 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/rlinoz in version: 9.2.59-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.2.59-5 🚀
|
6 participants
Explanation of Change
This PR introduces manual metrics to calculate
OpenReportduration and send it to Sentry. We're starting span right after user initializes the navigation and end it when report actions is rendered.Fixed Issues
$ #74132
PROPOSAL:
Tests
ManualOpenReportspanOffline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
dev_androi.mov
Android: mWeb Chrome
iOS: Native
dev_ios.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
dev_web.mov
MacOS: Desktop
dev_desktop.mov