-
Notifications
You must be signed in to change notification settings - Fork 27
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(eventtracker): Initial implementation of basic eventtracker #779
Conversation
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.
Added image of what we discussed offline for reference
Basically
- create a generic posthog client package
- Have a telemetry package with a struct called trackCommand or similar. This telemetry package is the one that will do all the heavy lifting re: labels, env discovery and so on, the posthog package should just map TAgs into their own format and emit the event.
- we do not need a noop tracker, we can just check if the client is nil
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.
Some more additional feedback, looking good
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.
LGTM!
I left some comments on the tests, I have the feeling that we have probably used mocking too much and for use-cases where we could unit test the different parts instead.
For example, we could test the label generation/discovery by themselves, and then about calling Track
just test with or without tags.
But anyways, this is more about personal preference, nothing against this code, just double check that the use of _ is idiomatic, I do not think so.
re: tests failing, I think it's detecting GitHub, you can unset it as I did here
os.Unsetenv("CI") |
Thanks!
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.
LGTM! 🚀
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
07c000a
to
8c22412
Compare
Initial implementation of an Event Tracker interface that is able to send events to a third party backend.
It creates one interface
Client
that is the one being called from the implementation whenever we want to track something.Refs: #780