Skip to content
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

Merged
merged 8 commits into from
Aug 30, 2024
Merged

Conversation

mike-zorn
Copy link
Contributor

@mike-zorn mike-zorn commented Aug 19, 2024

This adds events for the dev server commands so that we can track adoption. Along the way, I also did the following

  • Added a LogClient implementation for testing event tracking
  • Simplified the TrackerFn so that it was easier to swap clients
  • Split up the event tracker implementations into multiple files
  • Added a note on what steps are needed to add a command because we missed 2/3 for the dev server.

Tested the event sending by swapping in LogClient and I produced the following (404 was expected)

❯ go run . dev-server add-project --project jimbo-jones --source staging
2024/08/28 15:48:12 SendCommandRunEvent, properties: map[action:add-project baseURI:https://app.ld.catamorphic.com flags:[access-token analytics-opt-out base-uri dev-stream-uri project source] name:dev-server]
unable to get SDK key from LD API: 404 Not Found (code: internal_server_error)
2024/08/28 15:48:12 SendCommandCompletedEvent, outcome: error

@mike-zorn mike-zorn requested a review from dbolson August 19, 2024 19:02
cmd/root.go Outdated
viper.GetString(cliflags.BaseURIFlag),
viper.GetBool(cliflags.AnalyticsOptOut),
)
tracker.SendCommandRunEvent(cmdAnalytics.CmdRunEventProperties(cmd))
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@mike-zorn mike-zorn force-pushed the add-analytics-for-l-devly branch from e73a3b9 to 8080bd2 Compare August 28, 2024 23:17
@mike-zorn mike-zorn marked this pull request as ready for review August 28, 2024 23:22
@mike-zorn mike-zorn requested a review from dbolson August 28, 2024 23:22
@mike-zorn mike-zorn changed the title fix: Standardize command run events WIP fix: Standardize command run events Aug 28, 2024
@mike-zorn mike-zorn changed the title fix: Standardize command run events fix: dev server commands emit events Aug 29, 2024
@mike-zorn mike-zorn requested a review from k3llymariee August 30, 2024 17:10
@mike-zorn mike-zorn merged commit 2019b77 into main Aug 30, 2024
12 checks passed
@mike-zorn mike-zorn deleted the add-analytics-for-l-devly branch August 30, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants