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

[devbox] Add anonymized telemetry #40

Merged
merged 4 commits into from
Aug 30, 2022
Merged

[devbox] Add anonymized telemetry #40

merged 4 commits into from
Aug 30, 2022

Conversation

loreto
Copy link
Contributor

@loreto loreto commented Aug 29, 2022

Summary

Add anonymized telemetry to understand usage of devbox and further improve it. We allow users to opt-out via an environment variable.

To add telemetry I implemented a small "midcobra" framework, that make it possible to add "middlware" to cobra CLIs. The plan is to re-use the midcobra functionality in other binaries like envsec.

How was it tested?

Built locally, ran, and checked telemetry logs.

Copy link
Collaborator

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to make sure we're not prematurely architecting things with the middleware. It's a little tricky to follow as opposed to something like:

t := tracker.Track(...)
defer t.done()

// run the cobra stuff


return &telemetryMiddleware{
opts: *opts,
disabled: noTrackEnvVar == "1" || noTrackEnvVar == "true" || opts.TelemetryKey == "",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strconv.ParseBool will catch all the usual values.

@@ -10,6 +10,7 @@ builds:
- -s -w -X go.jetpack.io/devbox/build.Version={{.Version}}
- -s -w -X go.jetpack.io/devbox/build.Commit={{.Commit}}
- -s -w -X go.jetpack.io/devbox/build.CommitDate={{.CommitDate}}
- -s -w -X go.jetpack.io/devbox/build.TelemetryKey={{ .Env.TELEMETRY_KEY }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually need to be kept secret? It'll still be visible in the binary. Can we just put it in a const instead of a linker flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna leave as is for this PR since it's already working, but we can hard-code it in a follow up clean up.

I'm open either way, last time I asked on Slack, folks leaned towards hiding this, so that's why I implemented this way.

@loreto loreto merged commit 650e8fe into main Aug 30, 2022
@loreto loreto deleted the daniel/telemetry branch September 1, 2022 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants