Add telemetry to the CLI

[JulesFaucherre]

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have checked for similar issues and haven't found anything relevant.
• This is not a security issue (which should be reported here: https://circleci.com/security/)
• I have read Contribution Guidelines.

Internal Checklist

• I am requesting a review from my own team as well as the owning team
• I have a plan in place for the monitoring of the changes that I am making (this can include new monitors, logs to be aware of, etc...)

Changes

Ticket: https://circleci.atlassian.net/browse/DEVEX-889

• Added a telemetry client in the CLI
• Telemetry events added on different parts of the code. The implementation of such tests are best effort so some commands are not tested for telemetry

Rationale

Product was having trouble seeing how the CLI was used and asked us to add telemetry

Considerations

Know there is a PR on homebrew core to change the build in order to enable telemetry for homebrew users

Screenshots

On your first time using the CLI it will ask you for your approval like this:

[loderunner]

A lot of open discussions, mostly naming, structure, and semantics. No major conceptual objections. Let's discuss.

[ryanwohara]

This seems problematic in an air-gapped server environment. What is the UX like there?

EDIT: Took this to Slack https://circleci.slack.com/archives/C034BLFP326/p1689092028572929

[loderunner]

Great work here! Massive addition to our CLI product telemetry. The way the client is built and tested seems sound to me.

The only part I'm uncomfortable with is the client lifecycle patterns. Creating a new client in the PreRun, PersistentPreRun or Run depending on the case.

I have a feeling this could be done in a simpler fashion. As far as I understand, we want a single telemetry client per run, we want it throughout the command execution, we want to batch all the events, and then, when the execution is done, we want to send all of our events to analytics.

If this is the case, it sounds like our telemetry client would live well in the Command's Context. What do you think of doing something like:

cmd/root.go

const telemetryClientCtxKey = "telemetry_client"

func Execute() {
	header.SetCommandStr(CommandStr())
	command := MakeCommands()

	telemetry := CreateTelemetry(rootOptions)
	defer telemetry.Close()

	command.setContext(
		context.WithValue(
			command.Context(),
			telemetryClientCtxKey,
			telemetry,
		),
	)

	err := command.Execute()

	if err != nil {
		os.Exit(-1)
	}
}

cmd/config.go

	packCommand := &cobra.Command{
		Use:   "pack <path>",
		Short: "Pack up your CircleCI configuration into a single file.",
		RunE: func(cmd *cobra.Command, args []string) error {
			err := packConfig(args)

			telemetryClient, ok := cmd.Context().Value(create_telemetry.ContextKey{}).(telemetry.Client)
			if ok {
				telemetryClient.Track(telemetry.CreateConfigEvent(create_telemetry.GetCommandInformation(cmd, true), err))
			}
			return err
		},

---

Using a string constant as a context key is not Good Go™, but it seems acceptable in our CLI context. Available to discuss alternatives.

---

Does this proposal make sense to you?

[JulesFaucherre]

About your proposition, I completely understand it and I first thought about doing something like this but I decided to go the long way by implementing the creation / closing of telemetry for every command because of product reason.

First reason is pretty minor but I like the fact that you are asked telemetry only on commands that send telemetry events. For example is you only use the CLI to set your projects' secrets you may never know about this telemetry.
Second reason is a edge case: for the circleci setup it is more logical in terms of code and more coherent in terms of UX to get asked about telemetry after you have answer all the setup input. In a code point of view because, at that point you have the user id and the host so you can save the user's information before sending the telemetry event. In a UX point of view because it feels more natural to be asked about telemetry as the last step of setting up your CLI

That being said those are kinda minor points and we may consider that having a more standard way to use the telemetry in the code is more important than the reasons I mention. Open to discuss this in a call if you want

EDIT: As I was reading your comments I came up with a way to implement this in a good generic way in this comment