Conversation
There was a problem hiding this comment.
Pull request overview
Introduces best-effort, pseudoanonymous client-side telemetry for gh command invocations (sampled), with support for logging payloads locally and a hidden subprocess-based sender.
Changes:
- Add a telemetry service (
internal/telemetry) with device ID persistence, common dimensions, sampling, and subprocess flushing. - Instrument Cobra commands to record command path + set flags, with opt-outs for sensitive/irrelevant commands.
- Add hidden
gh send-telemetrycommand and Twirp client/server bindings, plus config/env plumbing and acceptance coverage.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmdutil/telemetry.go | Adds Cobra RunE instrumentation to record command invocation telemetry. |
| pkg/cmdutil/telemetry_test.go | Unit tests for command instrumentation behavior (flags, sorting, disabled commands). |
| pkg/cmd/version/version.go | Switches version to RunE to work with telemetry instrumentation. |
| pkg/cmd/send-telemetry/send_telemetry.go | Adds hidden send-telemetry command that posts payload batches to the telemetry endpoint. |
| pkg/cmd/send-telemetry/send_telemetry_test.go | Unit tests for send-telemetry option parsing and request formation. |
| pkg/cmd/root/root.go | Wires telemetry recorder into root command subcommands; registers send-telemetry command. |
| pkg/cmd/root/help_topic.go | Documents telemetry-related environment variables in help topics. |
| pkg/cmd/root/help_test.go | Updates root construction in tests to pass a no-op telemetry service. |
| pkg/cmd/root/extension_registration_test.go | Updates root construction in tests to pass a no-op telemetry service. |
| pkg/cmd/root/extension.go | Disables telemetry for extensions and uses cmdutil.DisableAuthCheck. |
| pkg/cmd/config/list/list_test.go | Updates config list expectations to include telemetry setting. |
| pkg/cmd/completion/completion.go | Disables telemetry for completion generation command. |
| pkg/cmd/auth/auth.go | Disables telemetry for auth subcommands. |
| internal/telemetry/telemetry.go | Implements telemetry service, sampling, payload building, and detached subprocess flush. |
| internal/telemetry/telemetry_test.go | Comprehensive tests for device ID persistence, state parsing, flush behavior, and sampling. |
| internal/telemetry/fake.go | Test spy for recording telemetry events. |
| internal/telemetry/detach_unix.go | Unix process detachment implementation for telemetry subprocess. |
| internal/telemetry/detach_windows.go | Windows process detachment implementation for telemetry subprocess. |
| internal/ghcmd/cmd.go | Creates telemetry service based on env/config and injects it into root command creation. |
| internal/gh/ghtelemetry/telemetry.go | Defines shared telemetry interfaces and event types used across packages. |
| internal/gh/gh.go | Extends gh.Config interface with Telemetry() accessor. |
| internal/gh/mock/config.go | Extends config mock to support Telemetry() calls. |
| internal/config/config.go | Adds telemetry config key, accessor, and config option registration. |
| internal/config/config_test.go | Tests telemetry config defaulting and user-provided values. |
| internal/config/stub.go | Wires stub config mock to underlying telemetry config implementation. |
| internal/barista/observability/telemetry.pb.go | Adds generated protobuf types for telemetry ingestion API. |
| internal/barista/observability/telemetry.twirp.go | Adds generated Twirp client/server bindings for telemetry ingestion API. |
| cmd/gen-docs/main.go | Updates docs generator to pass a no-op telemetry service to root. |
| acceptance/acceptance_test.go | Adds telemetry acceptance test suite + disables telemetry by default in shared setup. |
| acceptance/testdata/telemetry/command-invocation.txtar | Acceptance coverage for log-mode command invocation event output. |
| acceptance/testdata/telemetry/no-telemetry-for-completion.txtar | Ensures completion command does not emit telemetry. |
| acceptance/testdata/telemetry/no-telemetry-for-extension.txtar | Ensures extensions do not emit telemetry. |
| acceptance/testdata/telemetry/no-telemetry-for-send-telemetry.txtar | Ensures the sender command does not emit telemetry. |
| acceptance/testdata/telemetry/telemetry-failure-does-not-break-command.txtar | Ensures unreachable endpoint does not break command execution. |
| go.mod | Adds direct dependencies for uuid + twirp used by telemetry implementation. |
| go.sum | Adds checksum entries for new dependencies. |
| Makefile | Adds lint phony target. |
| .gitignore | Ignores test coverage output artifacts. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 36/38 changed files
- Comments generated: 5
| cmd := exec.Command(executable, "send-telemetry") | ||
|
|
||
| cmd.Stdout = io.Discard | ||
| cmd.Stderr = io.Discard | ||
|
|
||
| // Set the working directory to a stable directory elsewhere so that the subprocess doesn't | ||
| // hold a reference to the parent's current working directory, avoiding any weirdness around | ||
| // deleting the parent process's current working directory while the child is still running. | ||
| cmd.Dir = os.TempDir() |
There was a problem hiding this comment.
SpawnSendTelemetry sets cmd.Dir to os.TempDir(). If the executable argument is a relative path (e.g. set via GH_PATH or other caller), the process lookup will be performed relative to cmd.Dir and can fail to start. Consider ensuring executable is absolute (e.g., filepath.Abs/exec.LookPath) before setting cmd.Dir, or avoid changing cmd.Dir when executable is not absolute.
There was a problem hiding this comment.
Good catch. Hardened in ef7f77f: we now filepath.Abs(executable) before starting the child, and only set cmd.Dir = os.TempDir() when the resulting path is absolute, so a relative GH_PATH can still spawn the subprocess.
|
|
||
| // Write the payload synchronously into the kernel pipe buffer, then close | ||
| // the pipe to signal EOF. The child reads the complete payload from stdin. | ||
| _, _ = stdin.Write(payloadBytes) |
There was a problem hiding this comment.
The payload write to the child stdin ignores the returned byte count and error. A short write can result in truncated JSON and a guaranteed parse failure in the child process. Consider using io.Copy/WriteAll semantics (loop until all bytes are written or an error occurs), even if errors are ultimately ignored.
| _, _ = stdin.Write(payloadBytes) | |
| _, _ = io.Copy(stdin, bytes.NewReader(payloadBytes)) |
There was a problem hiding this comment.
Switched to io.Copy(stdin, bytes.NewReader(payloadBytes)) in ef7f77f. In practice *os.File.Write already follows the full-write contract for pipes, but the io.Copy form is equally readable and robust by construction.
| %[1]sGH_TELEMETRY%[1]s: set to %[1]slog%[1]s to print telemetry data to standard error instead of sending it. | ||
| Set to %[1]sfalse%[1]s or %[1]s0%[1]s to disable telemetry that would have been printed when set to %[1]slog%[1]s. | ||
| %[1]sDO_NOT_TRACK%[1]s: set to %[1]strue%[1]s or %[1]s1%[1]s to disable telemetry that would have been printed | ||
| when %[1]sGH_TELEMETRY%[1]s is set to %[1]slog%[1]s. %[1]sGH_TELEMETRY%[1]s takes precedence if both are set. |
There was a problem hiding this comment.
The help text for GH_TELEMETRY/DO_NOT_TRACK contains trailing whitespace/blank lines and a double space (set to log) which will render oddly and tends to trip whitespace-sensitive tooling. Also, the DO_NOT_TRACK description is confusing: in the implementation, GH_TELEMETRY being set (including log) takes precedence and DO_NOT_TRACK is ignored, so wording implying it disables telemetry “when GH_TELEMETRY is set to log” is misleading.
There was a problem hiding this comment.
Cleaned up in ef7f77f: removed the trailing whitespace and double space, and rewrote the two blurbs so the precedence is accurate (GH_TELEMETRY always wins when set; DO_NOT_TRACK only applies when GH_TELEMETRY is unset).
| cfg, err := cmdFactory.Config() | ||
| if err != nil { | ||
| fmt.Fprintf(stderr, "failed to load config: %s\n", err) | ||
| return exitError | ||
| } |
There was a problem hiding this comment.
Main now treats any config load error as fatal and exits early. Previously, many commands (e.g. gh version) could still run even if the config file was unreadable/corrupt; this change makes the entire CLI unusable in that situation, which is a significant behavior change unrelated to telemetry collection. Consider falling back to a blank config (and disabling telemetry/migrations) when config loading fails, or gating the fatal exit behind cases where config is strictly required.
There was a problem hiding this comment.
Fixed in ef7f77f. A config load error is now a stderr warning rather than a fatal exit: we fall back to a no-op telemetry service, skip the migration, and continue building the root command. This restores the pre-PR behaviour where gh version, gh --help, etc. remain usable when the config file is unreadable or corrupt.
- Harden SpawnSendTelemetry against relative executable paths - Use io.Copy for telemetry subprocess stdin write - Clean up GH_TELEMETRY/DO_NOT_TRACK help text - Fall back to blank config (NoOp telemetry) on config load failure Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
As per upcoming docs update, changelog and release notes, we're going to start collecting pseudoanonymous client side telemetry. Agents love
gh, and we're experiencing massive growth in usage. We need more insight into usage to make better, faster decisions.Telemetry can be viewed on stderr without being sent by setting
GH_TELEMETRY=logorgh config set telemetry log. It can be disabled usingGH_TELEMETRY=false,DO_NOT_TRACK=trueorgh config set telemetry disabled.The telemetry is best-effort, spawning a detached subprocess at the end of an invocation. It sampled at
1%across all commands.This PR requires
GH_PRIVATE_ENABLE_TELEMETRYto be set for any telemetry to be sent. This is because we need to communicate through other channels before turning it on.