Skip to content

feat(telemetry): add basic telemetry system#34

Open
miguelsanchez-upsun wants to merge 8 commits intomainfrom
telemetry-integration-draft
Open

feat(telemetry): add basic telemetry system#34
miguelsanchez-upsun wants to merge 8 commits intomainfrom
telemetry-integration-draft

Conversation

@miguelsanchez-upsun
Copy link
Collaborator

Add telemetry infrastructure to track command usage with privacy controls.

Includes unit tests and integration test framework.

Copilot AI review requested due to automatic review settings March 24, 2026 14:17
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a basic telemetry subsystem to track selected CLI command usage, with controls to disable telemetry via configuration and DO_NOT_TRACK, plus associated unit and integration tests.

Changes:

  • Add internal/telemetry package for asynchronously building/sending whitelisted telemetry events.
  • Extend configuration schema with Telemetry.Enabled and Telemetry.Endpoint.
  • Wire telemetry emission into the CLI root command flow and add unit + integration tests.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
internal/telemetry/telemetry.go Implements event model and async HTTP sender with endpoint resolution and legacy-based identity/token lookup.
internal/telemetry/commands.go Defines the telemetry command whitelist and command extraction helper.
internal/config/schema.go Adds telemetry config fields and default enablement.
commands/root.go Hooks telemetry into legacy and Go-native command execution paths.
internal/telemetry/telemetry_test.go Unit tests for DNT/config/endpoint/whitelist behavior and endpoint resolution.
integration-tests/telemetry_test.go End-to-end tests with a mock telemetry server validating payload and auth header behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// Legacy commands send telemetry in Run before exitWithError
// cmd.Parent() != nil means this is a subcommand, not the root
if cmd.Parent() != nil {
telemetry.SendTelemetryEvent(cmd.Context(), cnf, cmd.Use)
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

In Cobra commands, cmd.Use can include placeholders like "init [flags]" (e.g. init is defined as Use: "init [flags]"), which won't match the exact strings in IsTracked(). As a result, telemetry won't be sent for tracked Go-native commands. Use cmd.Name() (or cmd.CommandPath() if you want full path) instead of cmd.Use when reporting the command identifier.

Suggested change
telemetry.SendTelemetryEvent(cmd.Context(), cnf, cmd.Use)
telemetry.SendTelemetryEvent(cmd.Context(), cnf, cmd.CommandPath())

Copilot uses AI. Check for mistakes.
Comment on lines +298 to +307
func waitForTelemetry(ctx context.Context, cnf *config.Config, args []string) {
command := telemetry.ExtractCommand(args)
done := telemetry.SendTelemetryEvent(ctx, cnf, command)

select {
case <-done:
// Telemetry completed
case <-time.After(2 * time.Second):
// Timeout - proceed anyway to avoid blocking user
}
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

waitForTelemetry() derives the command from raw os.Args[1:], but ExtractCommand() currently returns args[0] even if it's a flag (e.g. --no-interaction project:list). That will cause telemetry to be skipped for tracked legacy commands invoked with global flags. Consider passing Cobra-parsed args, or updating ExtractCommand() to skip leading flags and return the first non-flag token.

Copilot uses AI. Check for mistakes.
Comment on lines +26 to +33
// ExtractCommand extracts the command name from arguments.
func ExtractCommand(args []string) string {
if len(args) == 0 {
return "unknown"
}
// Return first arg (command name, no flags)
return args[0]
}
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

ExtractCommand() currently returns args[0] verbatim. For legacy invocations that include global flags before the command (e.g. --quiet project:list) this will return a flag and break tracking. Update this helper to skip leading flag-like args (prefix "-") and return the first non-flag token (and handle -- appropriately), falling back to "unknown" if none.

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +18

// Testing commands, to be removed
"project:list": true,
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

trackedCommands includes project:list with a note "Testing commands, to be removed". Shipping test-only telemetry behavior in production code is brittle and easy to forget. Consider removing it from the default whitelist and enabling it only in tests (e.g. via a test-only override, build tag, or env-controlled additional whitelist).

Suggested change
// Testing commands, to be removed
"project:list": true,

Copilot uses AI. Check for mistakes.
Comment on lines +43 to +44
Enabled bool `validate:"omitempty"` // defaults to true
Endpoint string `validate:"omitempty,url" yaml:"endpoint"` // telemetry endpoint
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

Telemetry.Endpoint is optional but its YAML tag is yaml:"endpoint" (no omitempty). This will cause marshaled config (used by the legacy wrapper) to always include endpoint: "" when unset, unlike most other optional string fields in this schema. Consider yaml:"endpoint,omitempty" for consistency and to avoid writing empty values into generated config.yaml.

Suggested change
Enabled bool `validate:"omitempty"` // defaults to true
Endpoint string `validate:"omitempty,url" yaml:"endpoint"` // telemetry endpoint
Enabled bool `validate:"omitempty"` // defaults to true
Endpoint string `validate:"omitempty,url" yaml:"endpoint,omitempty"` // telemetry endpoint

Copilot uses AI. Check for mistakes.
Comment on lines +49 to +56
func TestSendTelemetryEvent_RequiresEndpoint(t *testing.T) {
cnf := &config.Config{}
cnf.Telemetry.Enabled = true
cnf.Telemetry.Endpoint = "" // Empty = disabled

done := SendTelemetryEvent(context.Background(), cnf, "init")
<-done // Should skip telemetry
}
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

These tests don't clear the environment variable that getEndpoint() consults first (cnf.Application.EnvPrefix + "TELEMETRY_ENDPOINT"). Since EnvPrefix is empty in these test configs, a real TELEMETRY_ENDPOINT in the developer/CI environment could make SendTelemetryEvent() attempt a network call and slow/flakily block on the timeout. Set cnf.Application.EnvPrefix to a test-only prefix and/or explicitly Unsetenv("TELEMETRY_ENDPOINT") in tests that expect endpoint to be empty/ignored.

Copilot uses AI. Check for mistakes.
- Add getCLIVersion() helper to extract version from CLI binary
- Replace hardcoded '1.0.0' with dynamic version from --version output
- Fix TestTelemetry_TrackedCommand to use actual CLI version
- Remove TestTelemetry_UnauthenticatedUser (needs reimplementation)
- Update TestTelemetry_ServerError with proper org/project setup

Note: 2 tests still failing and need additional fixes
- Fix unchecked Write() errors in test mock servers
- Remove unused cnf parameter from sendEvent()
- Remove unused request parameter in error handler
- Fix test function signatures (remove unused t parameter)
- Clean up trailing newlines
- Fix import grouping in telemetry_test.go
- Remove redundant comment
- Add trailing newline
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