-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix: dev server commands emit events #410
Conversation
cmd/root.go
Outdated
viper.GetString(cliflags.BaseURIFlag), | ||
viper.GetBool(cliflags.AnalyticsOptOut), | ||
) | ||
tracker.SendCommandRunEvent(cmdAnalytics.CmdRunEventProperties(cmd)) |
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.
I don't think this gets called for every subcommand, so running something like flags show test-flag
doesn't send a "CLI Command Run" event, only a "CLI Command Completed."
It looks like the root's PersistentPreRun
method isn't run for all subcommands.
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.
The docs indicate that PersistentPreRun
does get run by sub commands (PreRun
doesn't get run by sub commands). Were you observing different behavior? Could I achieve do the same thing by putting the run event command in the Execute
method like we did for completed?
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 print out the data sent to gonfalon in the analytics Client#sendEvent()
method, you can see what gets sent. Or you could dump out the body gonfalon's POST /internal/tracking
if that's easier. When I printed out the request body in main vs. the branch, I didn't see the first request show up in the latter.
You can try to get it working in Execute
, but I think we tried that and it didn't have all the info or wasn't called all the time when it was expected. I can't remember exactly the issue, but you may find another solution if you want to spend more time with it.
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.
Turns out that this sorta works. Issue is that this doesn't get run for help commands. Other issue is that my annotation-based-event-properties idea has a lot of issues since the annotations are used for other reasons and will make it really hard to understand what event properties are significant.
I'm gonna abandon this approach and implement the event tracking more directly. Also, I'll add a note in CONTRIBUTING.md
about the stuff you need to do when adding a new command since there are a few things you need to do for it to be wired up correctly.
DO NOT MERGE use log tracker
e73a3b9
to
8080bd2
Compare
This adds events for the dev server commands so that we can track adoption. Along the way, I also did the following
LogClient
implementation for testing event trackingTrackerFn
so that it was easier to swap clientsTested the event sending by swapping in LogClient and I produced the following (404 was expected)