Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add type checking for codecov metrics #3148

Merged
merged 2 commits into from
Aug 28, 2024
Merged

Add type checking for codecov metrics #3148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfe4f9e
Add type checking for metrics
rohitvinnakota-codecov Aug 27, 2024
0c3d5b6
Update test
rohitvinnakota-codecov Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/pages/DefaultOrgSelector/DefaultOrgSelector.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ import config from 'config'
import { SentryBugReporter } from 'sentry'

import { TrialStatuses, usePlanData } from 'services/account'
import { useStoreCodecovEventMetric } from 'services/codecovEventMetrics'
import {
EVENT_METRICS,
useStoreCodecovEventMetric,
} from 'services/codecovEventMetrics'
import { useUpdateDefaultOrganization } from 'services/defaultOrganization'
import { useStaticNavLinks } from 'services/navigation'
import { useStartTrial } from 'services/trial'
Expand Down Expand Up @@ -126,7 +129,7 @@ function DefaultOrgSelector() {
updateDefaultOrg({ username: selectedOrg })
storeEventMetric({
owner: selectedOrg,
event: 'CLICKED_BUTTON',
event: EVENT_METRICS.CLICKED_BUTTON,
jsonPayload: { action: 'Selected Default Org' },
})
if (
Expand Down
7 changes: 5 additions & 2 deletions src/pages/RepoPage/BundlesTab/BundleOnboarding/BundleOnboarding.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ import { Switch, useHistory, useLocation, useParams } from 'react-router-dom'
import { SentryRoute } from 'sentry'

import NotFound from 'pages/NotFound'
import { useStoreCodecovEventMetric } from 'services/codecovEventMetrics'
import {
EVENT_METRICS,
useStoreCodecovEventMetric,
} from 'services/codecovEventMetrics'
import { useNavLinks } from 'services/navigation'
import { useRepo } from 'services/repo'
import { useRedirect } from 'shared/useRedirect'
Expand Down Expand Up @@ -201,7 +204,7 @@ const BundleOnboarding: React.FC = () => {
useEffect(() => {
storeEventMetric({
owner,
event: 'VISITED_PAGE',
event: EVENT_METRICS.VISITED_PAGE,
jsonPayload: { page: 'Bundle Onboarding' },
})
}, [storeEventMetric, owner])
Expand Down
9 changes: 6 additions & 3 deletions src/pages/RepoPage/CoverageOnboarding/CircleCI/CircleCI.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { useParams } from 'react-router-dom'

import { useStoreCodecovEventMetric } from 'services/codecovEventMetrics'
import {
EVENT_METRICS,
useStoreCodecovEventMetric,
} from 'services/codecovEventMetrics'
import { useOrgUploadToken } from 'services/orgUploadToken'
import { useRepo } from 'services/repo'
import { useFlags } from 'shared/featureFlags'
Expand Down Expand Up @@ -100,7 +103,7 @@ function Step1({ tokenCopy, uploadToken, providerName }: Step1Props) {
clipboardOnClick={() =>
storeEventMetric({
owner,
event: 'COPIED_TEXT',
event: EVENT_METRICS.COPIED_TEXT,
jsonPayload: { text: 'Step 1 CircleCI' },
})
}
Expand Down Expand Up @@ -147,7 +150,7 @@ function Step2({ defaultBranch }: Step2Props) {
clipboardOnClick={() =>
storeEventMetric({
owner,
event: 'COPIED_TEXT',
event: EVENT_METRICS.COPIED_TEXT,
jsonPayload: { text: 'Step 2 CircleCI' },
})
}
Expand Down
13 changes: 8 additions & 5 deletions src/pages/RepoPage/CoverageOnboarding/GitHubActions/GitHubActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { useState } from 'react'
import { useParams } from 'react-router-dom'

import { useStoreCodecovEventMetric } from 'services/codecovEventMetrics'
import {
EVENT_METRICS,
useStoreCodecovEventMetric,
} from 'services/codecovEventMetrics'
import { useOrgUploadToken } from 'services/orgUploadToken'
import { useRepo } from 'services/repo'
import { useFlags } from 'shared/featureFlags'
Expand Down Expand Up @@ -252,7 +255,7 @@ jobs:
clipboardOnClick={() =>
storeEventMetric({
owner,
event: 'COPIED_TEXT',
event: EVENT_METRICS.COPIED_TEXT,
jsonPayload: { text: `coverage GHA ${framework} install` },
})
}
Expand All @@ -268,7 +271,7 @@ jobs:
clipboardOnClick={() =>
storeEventMetric({
owner,
event: 'COPIED_TEXT',
event: EVENT_METRICS.COPIED_TEXT,
jsonPayload: { text: `coverage GHA ${framework} run` },
})
}
Expand All @@ -291,7 +294,7 @@ jobs:
clipboardOnClick={() =>
storeEventMetric({
owner,
event: 'COPIED_TEXT',
event: EVENT_METRICS.COPIED_TEXT,
jsonPayload: { text: `coverage GHA ${framework} action` },
})
}
Expand Down Expand Up @@ -355,7 +358,7 @@ function Step2({ tokenCopy, uploadToken }: Step2Props) {
clipboardOnClick={() =>
storeEventMetric({
owner,
event: 'COPIED_TEXT',
event: EVENT_METRICS.COPIED_TEXT,
jsonPayload: { text: 'Step 2 GHA' },
})
}
Expand Down
7 changes: 5 additions & 2 deletions src/pages/RepoPage/CoverageOnboarding/NewRepoTab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ import { Switch, useHistory, useLocation, useParams } from 'react-router-dom'
import { SentryRoute } from 'sentry'

import NotFound from 'pages/NotFound'
import { useStoreCodecovEventMetric } from 'services/codecovEventMetrics'
import {
EVENT_METRICS,
useStoreCodecovEventMetric,
} from 'services/codecovEventMetrics'
import { useNavLinks } from 'services/navigation'
import { useRepo } from 'services/repo'
import { useRedirect } from 'shared/useRedirect'
Expand Down Expand Up @@ -135,7 +138,7 @@ function NewRepoTab() {
useEffect(() => {
storeEventMetric({
owner,
event: 'VISITED_PAGE',
event: EVENT_METRICS.VISITED_PAGE,
jsonPayload: { page: 'Coverage Onboarding' },
})
}, [storeEventMetric, owner])
Expand Down
15 changes: 9 additions & 6 deletions src/services/codecovEventMetrics/useStoreCodecovEventMetric.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ import { graphql } from 'msw'
import { setupServer } from 'msw/node'
import { MemoryRouter, Route } from 'react-router-dom'

import { useStoreCodecovEventMetric } from './useStoreCodecovEventMetric'
import {
EVENT_METRICS,
useStoreCodecovEventMetric,
} from './useStoreCodecovEventMetric'

const server = setupServer()

Expand Down Expand Up @@ -69,7 +72,7 @@ describe('useStoreCodecovEventMetric', () => {

result.current.mutate({
owner: 'codecov',
event: 'CLICKED_BUTTON',
event: EVENT_METRICS.CLICKED_BUTTON,
jsonPayload: {},
})

Expand All @@ -90,7 +93,7 @@ describe('useStoreCodecovEventMetric', () => {

result.current.mutate({
owner: 'codecov',
event: 'CLICKED_BUTTON',
event: EVENT_METRICS.CLICKED_BUTTON,
jsonPayload: {},
})

Expand All @@ -114,7 +117,7 @@ describe('useStoreCodecovEventMetric', () => {

const metric = {
owner: 'codecov',
event: 'CLICKED_BUTTON',
event: EVENT_METRICS.CLICKED_BUTTON,
jsonPayload: {},
}
mockGetItem.mockReturnValue(JSON.stringify(metric))
Expand All @@ -140,7 +143,7 @@ describe('useStoreCodecovEventMetric', () => {

const newMetric = {
owner: 'codecov',
event: 'NEW_EVENT',
event: EVENT_METRICS.CLICKED_BUTTON,
jsonPayload: {},
}

Expand All @@ -150,7 +153,7 @@ describe('useStoreCodecovEventMetric', () => {
await waitFor(() => {
const updatedMetrics = [
...mockMetrics.slice(1),
'codecov|NEW_EVENT|{}',
'codecov|CLICKED_BUTTON|{}',
]
expect(mockSetItem).toHaveBeenCalledWith(
'UserOnboardingMetricsStored',
Expand Down
15 changes: 13 additions & 2 deletions src/services/codecovEventMetrics/useStoreCodecovEventMetric.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,25 @@ const mutationQuery = `
}
}
`
// Maintain parity with list of allowed metrics here
// https://github.com/codecov/shared/blob/main/shared/django_apps/codecov_metrics/service/codecov_metrics.py
export const EVENT_METRICS = {
VISITED_PAGE: 'VISITED_PAGE',
CLICKED_BUTTON: 'CLICKED_BUTTON',
COPIED_TEXT: 'COPIED_TEXT',
COMPLETED_UPLOAD: 'COMPLETED_UPLOAD',
INSTALLED_APP: 'INSTALLED_APP',
} as const
Comment on lines +26 to +32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of creating an object here, can we just export the different metrics as const's? Using objects like this stop some minification optimizations from happening because you can't shorten names on field properties for objects, instead just exporting the const's directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can pre-face them with METRICS_ to better identify them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tradeoff, in Rohit's approach the optimization is being traded off for a better auto-complete experience in the IDE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to use a single object as a source of truth for all metrics as it is more structured that way. IMO the benefits from readability and accessibility of the structure outweigh any minification optimizations on a few lines, given just a small number of constants.

Happy to make the change if you think there are any practical performance concerns from not doing this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving this a bit more thought, I think we should take a slightly different approach then both the current implementation and my original suggestion, my original reasoning isn't all that great.

I think we should remove the object entirely because it's introduces a lot of duplicate code both the key and value are always the same, and using the object that way also introduces the possibility that only one side of the key or value is changed but not the other, making this code brittle.

The approach I think would solve all the concerns and be very ergonomic is to create a union type and just use string literals in place. Right now the typeahead will only work after you imported the object, whereas with a type union it works instantly after you open a leading quote.

Here's an example of what the union type would look like:

type EventMetric =  'VISITED_PAGE' | 'CLICKED_BUTTON' | 'COPIED_TEXT' | 'COMPLETED_UPLOAD' | 'INSTALLED_APP'

Copy link
Contributor

@ajay-sentry ajay-sentry Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, we can utilize TS enums here if we want something similarly ergonomic.
https://www.typescriptlang.org/docs/handbook/enums.html#enums-at-compile-time

This example is somewhat of a middle ground between both of your approaches, just a little easier for me to read personally without all the '|' as we add more metrics but also not as verbose as setting the keys and values to the same thing

enum EVENT_METRICS {
  VISITED_PAGE,
  CLICKED_BUTTON,
  COPIED_TEXT,
  COMPLETED_UPLOAD,
  INSTALLED_APP
}

type EventMetrics = keyof typeof EVENT_METRICS

const mymetric:EventMetrics = 'lol' // This errors because it's looking for the union string type

Copy link
Contributor

@nicholas-codecov nicholas-codecov Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some comments around the enum, to inform people to not actually ever use the enum directly. There are quite a few gotcha's with enums, that we want to avoid at all costs so we don't want people to accidentally use it and cause larger issues.

You can read more about the issues with enums, and I personally think we should avoid them, "Why I Don't Like Enums", but I'm okay with using them as long as we don't export them and just use them to build out the string literal unions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are good points, but I think for a change a small as this, how we choose to implement it will have a negligible effect on performance or any other meaningful metric.

I will merge this change as is later today unless we think there is a strong practical reason to go with one approach over the other.


type EventMetric = (typeof EVENT_METRICS)[keyof typeof EVENT_METRICS]

interface Params {
provider: string
}

interface StoreEventMetricMutationArgs {
owner: string
event: string
event: EventMetric
jsonPayload: object
}

Expand All @@ -37,7 +48,7 @@ const MAX_ENTRIES = 30

const generateMetricString = (
owner: string,
event: string,
event: EventMetric,
jsonPayload: string
) => {
return `${owner}|${event}|${jsonPayload}`
Expand Down
Loading