Skip to content

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.

@loreto loreto requested review from gcurtis and mikeland73 August 29, 2022 23:57
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.

2 participants