fix: dev server commands emit events#410
Conversation
| viper.GetString(cliflags.BaseURIFlag), | ||
| viper.GetBool(cliflags.AnalyticsOptOut), | ||
| ) | ||
| tracker.SendCommandRunEvent(cmdAnalytics.CmdRunEventProperties(cmd)) |
There was a problem hiding this comment.
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.
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.
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.
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
LogClientimplementation for testing event trackingTrackerFnso that it was easier to swap clientsTested the event sending by swapping in LogClient and I produced the following (404 was expected)