Iterate on EventTracker

spalmurray · spalmurray · commit 6e309e510436 · 2025-01-13T16:34:34.000-05:00
diff --git a/src/layouts/BaseLayout/BaseLayout.tsx b/src/layouts/BaseLayout/BaseLayout.tsx
@@ -10,6 +10,7 @@ import NetworkErrorBoundary from 'layouts/shared/NetworkErrorBoundary'
 import SilentNetworkErrorWrapper from 'layouts/shared/SilentNetworkErrorWrapper'
 import ToastNotifications from 'layouts/ToastNotifications'
 import { RepoBreadcrumbProvider } from 'pages/RepoPage/context'
+import { useEventContext } from 'services/events/events'
 import { useImpersonate } from 'services/impersonate'
 import { useTracking } from 'services/tracking'
 import GlobalBanners from 'shared/GlobalBanners'
@@ -77,6 +78,7 @@ interface URLParams {
 function BaseLayout({ children }: React.PropsWithChildren) {
   const { provider, owner, repo } = useParams<URLParams>()
   useTracking()
+  useEventContext()
   const { isImpersonating } = useImpersonate()
   const {
     isFullExperience,
diff --git a/src/layouts/Header/components/UserDropdown/UserDropdown.tsx b/src/layouts/Header/components/UserDropdown/UserDropdown.tsx
@@ -35,7 +35,7 @@ function UserDropdown() {
     },
   })
 
-  const { provider, owner, repo } = useParams<URLParams>()
+  const { provider } = useParams<URLParams>()
   const isGh = providerToName(provider) === 'GitHub'
   const history = useHistory()
 
@@ -46,11 +46,11 @@ function UserDropdown() {
             to: { pageName: 'codecovAppInstallation' },
             children: 'Install Codecov app',
             onClick: () =>
-              eventTracker(provider, owner, repo).track({
+              eventTracker().track({
                 type: 'Button Clicked',
                 properties: {
-                  buttonType: 'Install Github App',
-                  buttonLocation: 'UserDropdown',
+                  buttonType: 'Install GitHub App',
+                  buttonLocation: 'User dropdown',
                 },
               }),
           } as DropdownItem,
diff --git a/src/pages/OwnerPage/HeaderBanners/GithubConfigBanner/GithubConfigBanner.jsx b/src/pages/OwnerPage/HeaderBanners/GithubConfigBanner/GithubConfigBanner.jsx
@@ -26,8 +26,8 @@ const GithubConfigBanner = () => {
                 eventTracker(provider, owner).track({
                   type: 'Button Clicked',
                   properties: {
-                    buttonType: 'Install Github App',
-                    buttonLocation: 'GithubConfigBanner',
+                    buttonType: 'Install GitHub App',
+                    buttonLocation: 'Configure GitHub app banner',
                   },
                 })
               }
diff --git a/src/services/events/amplitude/amplitude.ts b/src/services/events/amplitude/amplitude.ts
@@ -1,19 +1,16 @@
 import * as amplitude from '@amplitude/analytics-browser'
 
-import { Provider } from 'shared/api/helpers'
-import {
-  InternalProvider,
-  providerToInternalProvider,
-} from 'shared/utils/provider'
+import { providerToInternalProvider } from 'shared/utils/provider'
 
-import { EventTracker } from '../events'
-import { Event } from '../types'
+import { Event, EventContext, EventTracker, Identity } from '../types'
 
 const AMPLITUDE_API_KEY = process.env.REACT_APP_AMPLITUDE_API_KEY
 
 export function initAmplitude() {
   if (!AMPLITUDE_API_KEY) {
-    return
+    throw new Error(
+      'AMPLITUDE_API_KEY is not defined. Amplitude events will not be tracked.'
+    )
   }
   amplitude.init(AMPLITUDE_API_KEY, {
     // Disable all autocapture - may change this in the future
@@ -23,40 +20,21 @@ export function initAmplitude() {
 }
 
 export class AmplitudeEventTracker implements EventTracker {
-  #provider?: InternalProvider
-  #owner?: string
-  #providerOwner?: string
-  #repo?: string
+  context: EventContext = {}
+  identity?: Identity
 
-  constructor(provider?: Provider, owner?: string, repo?: string) {
-    if (!AMPLITUDE_API_KEY) {
-      throw new Error(
-        'AMPLITUDE_API_KEY is not defined. Ensure the environment variable is defined before attempting to initialize AmplitudeEventTracker.'
-      )
+  identify(identity: Identity) {
+    if (JSON.stringify(this.identity) === JSON.stringify(identity)) {
+      // Don't identify this user again this session.
+      return
     }
-    this.#provider = provider ? providerToInternalProvider(provider) : undefined
-    this.#owner = owner
-    this.#providerOwner =
-      this.#provider && this.#owner
-        ? formatProviderOwner(this.#provider, this.#owner)
-        : undefined
-    this.#repo = repo
-  }
 
-  identify({
-    userOwnerId,
-    username,
-  }: {
-    userOwnerId: number
-    username: string
-  }) {
-    amplitude.setUserId(userOwnerId.toString())
+    amplitude.setUserId(identity.userOwnerId.toString())
     const identifyEvent = new amplitude.Identify()
-    if (this.#provider) {
-      identifyEvent.set('provider', this.#provider)
-    }
-    identifyEvent.set('username', username)
+    identifyEvent.set('provider', providerToInternalProvider(identity.provider))
     amplitude.identify(identifyEvent)
+
+    this.identity = identity
   }
 
   track(event: Event) {
@@ -65,23 +43,19 @@ export class AmplitudeEventTracker implements EventTracker {
       event_type: event.type,
       // eslint-disable-next-line camelcase
       event_properties: {
-        owner: this.#providerOwner,
-        repo: this.#repo,
         ...event.properties,
+        ...this.context,
       },
       // This attaches the event to the owner's user group as well
-      groups: this.#providerOwner
+      groups: this.context.owner?.id
         ? {
-            owner: this.#providerOwner,
+            owner: this.context.owner.id,
           }
         : undefined,
     })
   }
-}
 
-function formatProviderOwner(provider: InternalProvider, ownerName: string) {
-  // Helper function to format the owner group name.
-  // The reason for this is owner names are not unique on their own, but
-  // provider/owner names are.
-  return `${provider}/${ownerName}`
+  setContext(context: EventContext) {
+    this.context = context
+  }
 }
diff --git a/src/services/events/events.ts b/src/services/events/events.ts
@@ -1,48 +1,64 @@
-import { Provider } from 'shared/api/helpers'
+import { captureException } from '@sentry/react'
+import { useEffect } from 'react'
+import { useParams } from 'react-router'
+
+import { useRepo } from 'services/repo'
+import { useOwner } from 'services/user'
 
 import { AmplitudeEventTracker, initAmplitude } from './amplitude/amplitude'
 import { StubbedEventTracker } from './stub'
-import { Event } from './types'
-
-const AMPLITUDE_API_KEY = process.env.REACT_APP_AMPLITUDE_API_KEY
-
-export abstract class EventTracker {
-  // Identifies the user this session belongs to.
-  identify({
-    userOwnerId: _userOwnerId,
-    username: _username,
-  }: {
-    userOwnerId: number
-    username: string
-  }): void {
-    throw new Error(
-      'EventTracker is abstract. Method identify must be implemented.'
-    )
-  }
+import { EventTracker } from './types'
 
-  track(_event: Event): void {
-    throw new Error(
-      'EventTracker is abstract. Method track must be implemented.'
-    )
+// EventTracker singleton
+let EVENT_TRACKER: EventTracker = new StubbedEventTracker()
+
+export function initEventTracker(): void {
+  // Sets the global EventTracker singleton and calls necessary init functions
+  try {
+    initAmplitude()
+    EVENT_TRACKER = new AmplitudeEventTracker()
+  } catch (e) {
+    if (process.env.REACT_APP_ENV === 'production') {
+      // If in production, we need to know this has occured.
+      captureException(e)
+    }
   }
 }
 
-export function initEventTracker(): void {
-  // Calls any init functions for EventTracker implementations
-  initAmplitude()
+// Returns the global EventTracker instance.
+export function eventTracker(): EventTracker {
+  return EVENT_TRACKER
 }
 
-// Returns an EventTracker. Provide any of provider, owner, repo that are
-// relevant to the event you're going to track. They will be attached to
-// any events you send with the returned EventTracker instance.
-export function eventTracker(
-  provider?: Provider,
-  owner?: string,
-  repo?: string
-): EventTracker {
-  if (!AMPLITUDE_API_KEY) {
-    // If the API key is not defined, we'll just return a stubbed EventTracker.
-    return new StubbedEventTracker()
-  }
-  return new AmplitudeEventTracker(provider, owner, repo)
+// Hook to keep the global EventTracker's context up-to-date.
+export function useEventContext() {
+  const { provider, owner, repo } = useParams<{
+    provider?: string
+    owner?: string
+    repo?: string
+  }>()
+
+  const { data: ownerData } = useOwner({ username: owner })
+  const { data: repoData } = useRepo({
+    provider: provider || '',
+    owner: owner || '',
+    repo: repo || '',
+    opts: { enabled: !!(provider && owner && repo) },
+  })
+
+  useEffect(() => {
+    EVENT_TRACKER.setContext({
+      owner: ownerData?.ownerid
+        ? {
+            id: ownerData?.ownerid,
+          }
+        : undefined,
+      repo: repoData?.repository
+        ? {
+            id: repoData?.repository?.repoid,
+            isPrivate: repoData?.repository.private || undefined,
+          }
+        : undefined,
+    })
+  }, [ownerData, repoData])
 }
diff --git a/src/services/events/stub.ts b/src/services/events/stub.ts
@@ -1,13 +1,7 @@
-import { EventTracker } from './events'
-import { Event } from './types'
+import { Event, EventContext, EventTracker, Identity } from './types'
 
 export class StubbedEventTracker implements EventTracker {
-  identify({
-    userOwnerId: _userOwnerId,
-    username: _username,
-  }: {
-    userOwnerId: number
-    username: string
-  }): void {}
+  identify(_identity: Identity): void {}
   track(_event: Event): void {}
+  setContext(_context: EventContext): void {}
 }
diff --git a/src/services/events/types.ts b/src/services/events/types.ts
@@ -1,15 +1,29 @@
-// Add new events to the union type here.
-// Please keep the values of `type` very generic as we have a limited number of
-// them. Instead, add more detail in `properties` where possible.
-// Adding event types this way provides type safety for names and event
+import { Provider } from 'shared/api/helpers'
+
+//
+// Add new events to the the Event union type below!
+//
+// Adding event types in this way provides type safety for names and event
 // properties.
 // E.g., every 'Button Clicked' event must have the buttonType property.
+//
+// Guidelines:
+//  - Event names should:
+//    - be of the form "[Noun] [Past-tense verb]",
+//    - have each word capitalized,
+//    - describe an action taken by the user.
+//  - Keep the values of `type` very generic as we have a limited number of
+//    them. Instead, add more detail in `properties` where possible.
+//  - Try to keep event property names unique to the event type to avoid
+//    accidental correlation of unrelated events.
+//  - Never include names, only use ids. E.g., use repoid instead of repo name.
+//
 
 export type Event =
   | {
       type: 'Button Clicked'
       properties: {
-        buttonType: 'Install Github App' | 'Configure Repo'
+        buttonType: 'Install GitHub App' | 'Configure Repo'
         buttonLocation?: string
       }
     }
@@ -19,3 +33,44 @@ export type Event =
         pageName: 'OwnerPage'
       }
     }
+
+export type Identity = {
+  userOwnerId: number
+  provider: Provider
+}
+
+// Describes the context to be attached to events. We can extend this as needs
+// arise in the future.
+export type EventContext = {
+  // owner the event is being performed ON, not BY.
+  owner?: {
+    id: number
+  }
+  repo?: {
+    id: number
+    isPrivate?: boolean
+  }
+}
+
+export abstract class EventTracker {
+  // Identifies the user this session belongs to.
+  identify(_identity: Identity): void {
+    throw new Error(
+      'EventTracker is abstract. Method identify must be implemented.'
+    )
+  }
+
+  // Tracks an event
+  track(_event: Event): void {
+    throw new Error(
+      'EventTracker is abstract. Method track must be implemented.'
+    )
+  }
+
+  // Sets the current EventContext
+  setContext(_context: EventContext): void {
+    throw new Error(
+      'EventTracker is abstract. Method setContext must be implemented.'
+    )
+  }
+}
diff --git a/src/services/repo/useRepo.tsx b/src/services/repo/useRepo.tsx
@@ -11,6 +11,7 @@ import {
 
 const RepositorySchema = z.object({
   __typename: z.literal('Repository'),
+  repoid: z.number(),
   private: z.boolean().nullable(),
   uploadToken: z.string().nullable(),
   defaultBranch: z.string().nullable(),
@@ -47,6 +48,7 @@ const query = `
         repository(name: $repo) {
           __typename
           ... on Repository {
+            repoid
             private
             uploadToken
             defaultBranch
@@ -73,6 +75,7 @@ type UseRepoArgs = {
   repo: string
   opts?: {
     refetchOnWindowFocus?: boolean
+    enabled?: boolean
   }
 }
 
diff --git a/src/services/user/useOwner.ts b/src/services/user/useOwner.ts
@@ -73,7 +73,7 @@ export function useOwner({
           })
         }
 
-        return res?.data?.owner
+        return parsedRes?.data?.owner
       }),
     ...opts,
   })
diff --git a/src/services/user/useUser.ts b/src/services/user/useUser.ts
diff --git a/src/shared/ListRepo/InactiveRepo/InactiveRepo.tsx b/src/shared/ListRepo/InactiveRepo/InactiveRepo.tsx
diff --git a/src/ui/ContextSwitcher/ContextSwitcher.tsx b/src/ui/ContextSwitcher/ContextSwitcher.tsx

Original file line number	Diff line number	Diff line change
`@@ -73,7 +73,7 @@ export function useOwner({`
`73`	`73`	`})`
`74`	`74`	`}`
`75`	`75`
`76`		`- return res?.data?.owner`
	`76`	`+ return parsedRes?.data?.owner`
`77`	`77`	`}),`
`78`	`78`	`...opts,`
`79`	`79`	`})`