feat: Implement EventTracker abstraction and Amplitude implementation #3650
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3650 +/- ##
=======================================
Coverage 98.80% 98.81%
=======================================
Files 820 825 +5
Lines 14784 14876 +92
Branches 4214 4234 +20
=======================================
+ Hits 14608 14700 +92
Misses 167 167
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3650 +/- ##
=======================================
Coverage 98.80% 98.81%
=======================================
Files 820 825 +5
Lines 14784 14876 +92
Branches 4214 4234 +20
=======================================
+ Hits 14608 14700 +92
Misses 167 167
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3650 +/- ##
=======================================
Coverage 98.91% 98.92%
=======================================
Files 817 822 +5
Lines 14716 14806 +90
Branches 4165 4184 +19
=======================================
+ Hits 14557 14647 +90
Misses 152 152
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3650 +/- ##
=======================================
Coverage 98.80% 98.81%
=======================================
Files 820 825 +5
Lines 14784 14876 +92
Branches 4206 4226 +20
=======================================
+ Hits 14608 14700 +92
Misses 167 167
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
src/services/events/events.ts
Outdated
|
|
||
| const AMPLITUDE_API_KEY = process.env.REACT_APP_AMPLITUDE_API_KEY | ||
|
|
||
| export interface EventTracker { |
There was a problem hiding this comment.
Using an abstract class instead of an interface is a little more explicit of this Class's purpose
There was a problem hiding this comment.
Tried that initially, but went with interface because it handled method overloads a little more cleanly. I agree though - particularly if we go with the Event type instead of overloads
src/services/events/events.ts
Outdated
| ): void | ||
| } | ||
|
|
||
| class StubbedEventTracker implements EventTracker { |
There was a problem hiding this comment.
I love the idea of having a stubbed / generic event tracker instance; maybe we can throw some errors in the functions instead for some higher visibility
Additionally, pulling this out into a separate file would help reduce some cognitive load when parsing this file
There was a problem hiding this comment.
My current plan is for the StubbedEventTracker to be used in development/testing/staging environments, so I'm hesitant to make it noisy. We can chat more though
Agree on pulling out to its own file.
src/services/events/events.ts
Outdated
| } | ||
|
|
||
| class StubbedEventTracker implements EventTracker { | ||
| identify(_userOwnerId: string | number, _username: string): void {} |
There was a problem hiding this comment.
Having this be a string | number seems a little smelly, is there a way around this?
There was a problem hiding this comment.
The goal here was to make the API as easy as possible to use. Give us the id in whatever type and we'll handle it. I will look around the codebase again though, I think it may be true that it's almost always a string or almost always a number. In that case we can get rid of the union type.
There was a problem hiding this comment.
Yea. it's pretty much always a number, will update.
src/services/events/events.ts
Outdated
|
|
||
| export interface EventTracker { | ||
| // Identifies the user this session belongs to. | ||
| identify(userOwnerId: number | string, username: string): void |
There was a problem hiding this comment.
Personally I really like having functions utilize an object syntax for input parameters for explicit defining instead of relying on proper placement from developers
so in this case we'd have identify({userOwnerId, username}: {userOwnerId: number | string, username: string})
which would prevent a situation where someone accidentally puts the username as the first param unknowingly (since it also accepts a string)
There was a problem hiding this comment.
Yep that's fair.
src/services/events/events.ts
Outdated
| // Adding event types this way provides type safety for event properties. | ||
| // E.g., every 'Button Clicked' event must have the buttonType property. | ||
| track( | ||
| eventType: 'Button Clicked', |
There was a problem hiding this comment.
Having a central const somewhere of "EventTypes" might help with readability
| @@ -26,6 +31,11 @@ function InactiveRepo({ | |||
| repo: repoName, | |||
| }, | |||
| }} | |||
| onClick={() => | |||
| eventTracker(provider, owner, repoName).track('Button Clicked', { | |||
There was a problem hiding this comment.
Is it possible to instantiate the event tracker somewhere as a singleton and then call track against that instance? Currently every time we want to call track we need to create a new instance of an AmplitudeEventTracker which seems a little expensive
There was a problem hiding this comment.
Yea I considered this. The con of doing a singleton is that we then need some system of keeping the provider/owner/repo the instance is storing up-to-date. This could simply be done by moving those three into the track() call. Would like to talk more on this for sure
spalmurray-codecov
force-pushed
the
spalmurray/event-tracker-amplitude
branch
2 times, most recently
from
2ba5fd3 to
6e309e5
Compare
spalmurray-codecov
force-pushed
the
spalmurray/event-tracker-amplitude
branch
from
3069c0f to
b0b22ee
Compare
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
Bundle ReportChanges will increase total bundle size by 267.24kB (2.2%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
spalmurray-codecov
force-pushed
the
spalmurray/event-tracker-amplitude
branch
2 times, most recently
from
fd42c85 to
6144625
Compare
spalmurray-codecov
force-pushed
the
spalmurray/event-tracker-amplitude
branch
from
6144625 to
093c894
Compare
Bundle ReportChanges will increase total bundle size by 267.15kB (2.2%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
spalmurray-codecov
force-pushed
the
spalmurray/event-tracker-amplitude
branch
from
093c894 to
16a63d2
Compare
Implements a new EventTracker abstraction and an Amplitude specific implementation of it. This provides a generic even tracking interface so we can swap out or change the implementation without major refactoring. This PR also adds some example 'Button Clicked' events that we're interested in tracking for our onboarding funnel.
Depends on codecov/codecov-api#1095
Depends on AMPLITUDE_API_KEY env var getting set (done)
Closes codecov/engineering-team#3178