-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add instrument package for Segment #226
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
Conversation
I think it might be worth including some docs pointing netlify folks to https://github.com/netlify/segment-events to make sure internal users follow the processes and conventions mentioned there? I like the visibility of having this package in netlify-commons, because this is probably the first place folks would look for it, but I wonder if this shouldn't be a package in the internal segment events repo? |
In general, I like the idea of pointing to the doc. I think we can also improve our README to explain how to use each package too, and pointing to that repo is something we can do in there. How does that sound? I'm honestly little hesitate to add it to the codebase as a comment, so feels better if it was in README.
Sooooo I'd disagree with having this package in segment-events repo. netlify-commons repo is providing the basic common features of our services and the instrumentation can be count as a basic feature, was my reasoning having this here. For example, we could implements more instrumentation client potentially in the future and swap out as the project needs. |
uhmm re: doc, actually maybe I should be adding more in the README but more in the code (as you can see it in https://pkg.go.dev/github.com/netlify/netlify-commons@v0.51.2/metriks). |
instrument/instrument.go
Outdated
type Client interface { | ||
identify(userID string, traits analytics.Traits) error | ||
track(userID string, event string, properties analytics.Properties) error | ||
page(userID string, name string, properties analytics.Properties) error | ||
group(userID string, groupID string, traits analytics.Traits) error | ||
alias(previousID string, userID string) error | ||
} |
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.
if you don't export these methods then you effectively have an interface you can't implement in a different client.
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.
🤦 yes
This PR adds the instrument package for Segment. You can enable this by having the following in the config file:
Then, you can use this like the following:
If enabled is
false
(defaultfalse
), you can still use this, but instead of the events sent out to Segment, it'll be logged to the logger.