[feat] Add metric to monitor app install shares #2532
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2532 +/- ##
=======================================
Coverage 98.31% 98.31%
=======================================
Files 803 803
Lines 10333 10335 +2
Branches 2589 2589
=======================================
+ Hits 10159 10161 +2
Misses 172 172
Partials 2 2
Continue to review full report in Codecov by Sentry.
|
✅ Deploy Preview for gazebo-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2532 +/- ##
=====================================
Coverage 98.32 98.32
=====================================
Files 803 803
Lines 10333 10335 +2
Branches 2589 2575 -14
=====================================
+ Hits 10159 10161 +2
Misses 172 172
Partials 2 2
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2532 +/- ##
=======================================
Coverage 98.31% 98.31%
=======================================
Files 803 803
Lines 10333 10335 +2
Branches 2575 2589 +14
=======================================
+ Hits 10159 10161 +2
Misses 172 172
Partials 2 2
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## main #2532 +/- ##
=======================================
Coverage 98.31% 98.31%
=======================================
Files 803 803
Lines 10333 10335 +2
Branches 2589 2594 +5
=======================================
+ Hits 10159 10161 +2
Misses 172 172
Partials 2 2
Continue to review full report in Codecov by Sentry.
|
rohitvinnakota-codecov
changed the title
There was a problem hiding this comment.
Thanks for giving Sentry Metrics a try! For reference docs can be found here: getsentry/sentry-javascript#9938
| @@ -14,6 +15,7 @@ import TopBanner, { saveToLocalStorage } from 'ui/TopBanner' | |||
| const APP_INSTALL_BANNER_KEY = 'request-install-banner' | |||
| const COPY_APP_INSTALL_STRING = | |||
| "Hello, could you help approve the installation of the Codecov app on GitHub for our organization? Here's the link: https://github.com/apps/codecov/installations/select_target" | |||
| const SHARE_REQUEST_METRICS_KEY = 'user_shared_request' | |||
There was a problem hiding this comment.
if we have this more widespread might make sense to colocate all metrics keys in a shared file somewhere so no risk of duplicates at all.
Also in Sentry we typically use . for the delimiter, and _ to represent compound words, and use plural for metrics, so user.shared.requests in this case.
Do we also want to add request.install prefix or something similar?
request_install.user.shared.requests
There was a problem hiding this comment.
actually we don't use plural I think I just hallucinated that - sorry for the fake news lol
request_install.user.shared.request makes the most sense to me
There was a problem hiding this comment.
@AbhiPrasad, thoughts on moving this to our sentry.ts file and creating an as const object that contain our keys, we could also use functions to create a base key and extend the remainder of the key, something like:
// sentry.ts
const METRIC_KEYS = {
REQUEST_INSTALL: (key: string) => `request_install.${key}`
} as const
// random file
const handleClick = () => {
metrics.increment(METRIC_KEYS.REQUEST_INSTALL('user.shared.request'))
}There was a problem hiding this comment.
I think a function abstraction introduces a lot of complexity that we don't need right now, let's start simple with just plain strings? We can move to introduce abstraction when there are more metrics and we feel like we need to enforce naming.
// sentry/metricKey.ts
export const REQUEST_INSTALL_USER_SHARED_REQUEST = 'request_install.user.shared.request';// random file
import { REQUEST_INSTALL_USER_SHARED_REQUEST } from 'blah/blah/sentry/metricKey.ts';
metrics.increment(REQUEST_INSTALL_USER_SHARED_REQUEST);
You can also write a wrapper function around metrics.increment and try to enforce the key names via a string union or even TS template literal types.
There was a problem hiding this comment.
I can document the current process(it's fairly straightforward).
It's hard to define an abstraction that makes sense without seeing more of these in the codebase so I am willing to wait till then and use this approach for now.
| it('should capture the user shared request metric', async () => { | ||
| const { user, mockGetItem } = setup({}) | ||
| render(<RequestInstallBanner />, { wrapper: wrapper('/gh/codecov') }) | ||
|
|
||
| mockGetItem.mockReturnValue(null) | ||
|
|
||
| const btn = screen.getByRole('button', { name: /Share Request/ }) | ||
| expect(btn).toBeInTheDocument() | ||
| await user.click(btn) | ||
|
|
||
| expect(Sentry.metrics.increment).toHaveBeenCalledWith( | ||
| 'request_install.user.shared.request' | ||
| ) | ||
| }) |
There was a problem hiding this comment.
This is more of a philosophical question, do we want to be explicitly testing that this metric gets picked up, since we're even mocking it out we're not testing any of the functionality of it? The big question is that do we gain anything meaningful?
There was a problem hiding this comment.
I'm treating Sentry.metrics as a 3rd party library relative to the gazebo repo, and think it's not necessary to test the internals of the metrics functions.
It is useful to have the tests for metrics still since we are testing whether we attempt to capture something.
If there are refactors to components in the future, the tests and descriptions should provide a way to maintain this behaviour, and also that we are capturing the right kind of metric.
rohitvinnakota-codecov
deleted the
rvinnakota/add-ddm-metric-app-install
branch
Description
We want to track relevant metrics on Gazebo, so that we may provide better UX, performance, and detect feedback early. This PR is a first pass at adding a metric each time a user displays intent to share a request for a Codecov installation to their app admin.
Once we have a dashboard with the metric rendering on Sentry, we can start to add additional metrics we think are important.
This PR closes codecov/engineering-team#1044
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.