-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(insights): add analytics to insight sample panels #70822
Conversation
...c/app/views/performance/transactionSummary/transactionSpans/spanSummary/spanSummaryTable.tsx
Show resolved
Hide resolved
Bundle ReportChanges will increase total bundle size by 2.11kB ⬆️
|
066b656
to
ab95284
Compare
…odules (#70890) Currently, the app starts module depends on `ScreenLoadSpanSamples` in `mobile/screenload` to display the side panel. However, we therefore cannot identify which module the panel comes from, which we will need for analytics (e.g. # of times a sample span is clicked per module) - #70822. This PR moves the component under `mobile/components` and adds a moduleName prop to identify the originating module.
2950263
to
828e976
Compare
a1837a7
to
594afde
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some small nits and questions because I'm a little unfamiliar with some of the constants you modified
onClick={() => | ||
trackAnalytics('performance_views.sample_spans.opened', { | ||
organization, | ||
source: ModuleName.APP_START, | ||
}) | ||
} | ||
to={`${pathname}?${qs.stringify(query)}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative I'm curious about is if we'd be able to instrument this in the panel itself since you're already passing along the module name. e.g. in a useEffect
hook that just runs once when it's rendered unless for some reason that won't work with how we've set up the panel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we can for all of the modules
screen_load: t('Action'), | ||
app_start: t('Action'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
screen_load: t('Action'), | |
app_start: t('Action'), | |
screenLoad: t('Action'), | |
appStart: t('Action'), |
Also, maybe I'm missing context but what are these used for? 🤔
screen_load: t('Domain'), | ||
app_start: t('Domain'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
screen_load: t('Domain'), | |
app_start: t('Domain'), | |
screenLoad: t('Domain'), | |
appStart: t('Domain'), |
Similar to my other comment, I'm not exactly sure what's being mapped here to use the Domain
string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These values are used as labels for some specific dropdowns in http and db modules (see "SQL Command" and "Table" in Queries module - screenshot). They aren't used for the remaining modules, but are required in this constant due to the {[key in ModuleName]: ReactNode}
typing. I included default Action and Domain values for these remaining modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that's kind of annoying that you had to make this change. I say we can fix the typing here later so we don't have to update this list every time we add a new module but I'm fine with keeping it as-is for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, will see if I can fix that in the follow up when I move instrumentation to the panel itself
Continuation of #70769. Adds analytics to the span sample panel to track: - How often the panels are opened - How often spans are clicked - How often “Try different samples” is clicked - How often specific filters are interacted with Modules to add analytics to: - [x] HTTP - [x] Database - [x] Cache - [x] Queues - [x] Screen Loads - [x] App Starts - [x] Resources Note: Web Vitals does not have a sample panel --------- Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Continuation of #70769. Adds analytics to the span sample panel to track:
Modules to add analytics to:
Note: Web Vitals does not have a sample panel