Add minimal, anonymous usage telemetry#292
Conversation
Default-on telemetry with a Homebrew-style consent model (issue #283): nothing is ever sent before the first-run notice has been shown. - State/marker: ~/.clickhouse/telemetry.json ({"disabled": false}); first run writes the file, prints a notice to stderr, and sends nothing. The notice is gated on the write succeeding, so an unwritable config dir fails open to disabled with no repeated notice. - DO_NOT_TRACK (donottrack.sh) overrides everything; CHCTL_TELEMETRY_DEBUG=1 prints the exact payload to stderr and sends nothing. - Payload is command path + flag NAMES only, built by walking the clap Command/ArgMatches metadata (never get_one/get_raw), so leaking values or positionals is structurally impossible. Plus success bool, agent/CI detection, version, os/arch. - Transport is a detached `telemetry send` child (hidden subcommand, all stdio nulled, 2s timeout, parent never waits): zero latency impact even with the endpoint blackholed. - `clickhousectl telemetry enable|disable|status` manages consent; the whole feature (including the subcommand) compiles out with --no-default-features for distro packagers, enforced in CI. - Existing subprocess tests now set DO_NOT_TRACK=1 so test runs never write real state or post to the production endpoint. Verified end-to-end against the telemetry worker running locally via wrangler dev: first run silent, subsequent runs deliver events that the worker accepts and inserts (200 OK). Closes #283 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| TelemetryCommands::Disable => { | ||
| set_disabled(true)?; | ||
| println!("Telemetry disabled."); | ||
| Ok(()) |
There was a problem hiding this comment.
Enable sends before first-run notice
High Severity
The telemetry enable and telemetry disable commands write the telemetry state file (telemetry.json) before the finalize hook. This bypasses the State::Missing check, causing the first-run notice to be skipped. As a result, telemetry can be enabled and potentially sent, or disabled, without the user ever seeing the initial consent notice.
Reviewed by Cursor Bugbot for commit 7fddcf3. Configure here.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| /// the real environment). | ||
| type EnvLookup<'a> = &'a dyn Fn(&str) -> Option<String>; | ||
|
|
||
| fn real_env_lookup(key: &str) -> Option<String> { |
There was a problem hiding this comment.
🟡 Medium src/telemetry.rs:104
real_env_lookup uses std::env::var(key).ok(), which returns None when the environment variable is set but its value contains non-UTF-8 bytes. On Unix, if DO_NOT_TRACK is set to non-UTF-8 bytes, decide treats the opt-out variable as absent and telemetry proceeds despite the user setting it. Use std::env::var_os so non-Unicode values are still detected.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/telemetry.rs around line 104:
`real_env_lookup` uses `std::env::var(key).ok()`, which returns `None` when the environment variable is set but its value contains non-UTF-8 bytes. On Unix, if `DO_NOT_TRACK` is set to non-UTF-8 bytes, `decide` treats the opt-out variable as absent and telemetry proceeds despite the user setting it. Use `std::env::var_os` so non-Unicode values are still detected.
| if env_truthy(env(DNT_ENV)) { | ||
| return Action::Silent; | ||
| } | ||
| match load_state_from(path) { |
There was a problem hiding this comment.
🟡 Medium src/telemetry.rs:242
On a fresh machine, concurrent clickhousectl invocations can send telemetry before the first-run consent notice is ever shown. In decide(), the State::Missing arm writes telemetry.json (marking the notice as "shown") before finalize() actually prints the notice. A second process started concurrently sees the marker file, reads State::Enabled, and returns Action::Send(...), transmitting telemetry even though no user has been notified yet. Consider deferring the state write until after the notice has been printed, or using a distinct intermediate state so that Send is not reachable until the notice has actually been displayed.
Also found in 1 other location(s)
README.md:852
The claim at
README.mdline 852 that "Nothing is ever sent before you have seen the notice" is false forclickhousectl telemetry enableon a fresh machine.maincallstelemetry::finalizeafter command execution,telemetry enablewrites~/.clickhouse/telemetry.jsoninset_disabled(false), and thendecide()seesState::Enabledand returnsAction::Sendwithout ever printing the first-run notice. A first-timetelemetry enableinvocation therefore sends an event even though the README says first run always sends nothing.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/telemetry.rs around line 242:
On a fresh machine, concurrent `clickhousectl` invocations can send telemetry before the first-run consent notice is ever shown. In `decide()`, the `State::Missing` arm writes `telemetry.json` (marking the notice as "shown") before `finalize()` actually prints the notice. A second process started concurrently sees the marker file, reads `State::Enabled`, and returns `Action::Send(...)`, transmitting telemetry even though no user has been notified yet. Consider deferring the state write until after the notice has been printed, or using a distinct intermediate state so that `Send` is not reachable until the notice has actually been displayed.
Also found in 1 other location(s):
- README.md:852 -- The claim at `README.md` line 852 that "Nothing is ever sent before you have seen the notice" is false for `clickhousectl telemetry enable` on a fresh machine. `main` calls `telemetry::finalize` after command execution, `telemetry enable` writes `~/.clickhouse/telemetry.json` in `set_disabled(false)`, and then `decide()` sees `State::Enabled` and returns `Action::Send` without ever printing the first-run notice. A first-time `telemetry enable` invocation therefore sends an event even though the README says first run always sends nothing.
… prefix The send child already goes through http::client_builder(), so every telemetry POST carries the same User-Agent as all other outbound HTTP: `clickhousectl/<version>`, optionally followed by an ` (agent=<id>)` comment. Assert it wire-level so the ingest worker can safely reject traffic without that prefix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The CLI already holds the detected agent as a structured value via is-ai-agent, so set both facts client-side — is_agent (bool) and agent (canonical id, e.g. "claude-code"; null for humans) — rather than having the ingest worker string-parse them out of the User-Agent header. Same for version: the payload value from CARGO_PKG_VERSION stays canonical. The User-Agent still carries the same facts for prefix-based request filtering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| - whether it ran in CI (`CI` env var) | ||
| - whether it ran under a detected coding agent, and if so which one (e.g. `claude-code`) | ||
|
|
||
| There is no install ID, no device ID, and no fingerprinting of any kind. The payload is built from the clap command definitions rather than the raw command line, so leaking an argument value is structurally impossible — the code that builds the event has no access to values at all. |
There was a problem hiding this comment.
🟡 Medium README.md:851
Line 851 states there is no install ID, device ID, or fingerprinting of any kind. That claim is false: telemetry requests built by run_child_send() go through http::client_builder(), which unconditionally adds agent-session-id and traceparent headers from is_ai_agent::detect(). These headers expose stable session/conversation identifiers that let the backend correlate multiple telemetry posts, contradicting the documented promise that events carry no IDs or fingerprinting.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @README.md around line 851:
Line 851 states there is no install ID, device ID, or fingerprinting of any kind. That claim is false: telemetry requests built by `run_child_send()` go through `http::client_builder()`, which unconditionally adds `agent-session-id` and `traceparent` headers from `is_ai_agent::detect()`. These headers expose stable session/conversation identifiers that let the backend correlate multiple telemetry posts, contradicting the documented promise that events carry no IDs or fingerprinting.
The finalize hook already receives the exact exit code the process is about to exit with (Error::exit_code: 0 success, 1 error, 2 cancelled, 4 auth required), so send that: success is derivable (exit_code = 0) but the code also distinguishes cancellations and auth failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 37d248d. Configure here.
| "Could not determine home directory", | ||
| )) | ||
| })?; | ||
| save_state_to(&path, disabled).map_err(Error::Io) |
There was a problem hiding this comment.
DNT still writes telemetry file
Medium Severity
DO_NOT_TRACK is honored in decide()/finalize() but not in telemetry enable or telemetry disable. Those subcommands always call set_disabled() and write ~/.clickhouse/telemetry.json, contradicting the module comment and PR text that DNT overrides everything with no file write.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 37d248d. Configure here.
| } else { | ||
| Action::Send(json) | ||
| } | ||
| } |
There was a problem hiding this comment.
Parallel first runs may send
Medium Severity
decide reads and writes telemetry.json without synchronization. Two concurrent processes that both start with a missing marker can each pass the first-run path; after one writes the enabled marker, the other may still reach State::Enabled and send on what is effectively still the user’s first wave of runs, before a separate “following run.”
Reviewed by Cursor Bugbot for commit 37d248d. Configure here.
1 participant
Closes #283
Default-on anonymous usage telemetry with a Homebrew-style consent model: nothing is ever sent before the first-run notice has been shown.
Consent & state
~/.clickhouse/telemetry.json({"disabled": false}) doubles as the first-run marker. First run: write file → print notice to stderr (regardless of TTY) → send nothing. The notice is gated on the write succeeding, so an unwritable config dir fails open to disabled — no send, no error, no repeated notice.DO_NOT_TRACK(donottrack.sh spec: set and not""/"0"/"false") is checked first and overrides everything.clickhousectl telemetry enable|disable|statusmanages the file. Consent is evaluated after the command runs, sotelemetry disablesilences its own event andtelemetry enablesends one.Payload
{"command":"local list","flags":["json"],"exit_code":0,"is_agent":true,"agent":"claude-code","ci":false,"version":"0.3.1","os":"macos","arch":"aarch64"}command(path),flags(long names only),exit_code(gh-style: 0 success, 1 error, 2 cancelled, 4 auth required),is_agent+agent(canonical id from is-ai-agent,nullfor humans),ci(CIenv),version,os,arch. No install ID, no fingerprinting. Agent, version, and exit code are set client-side from structured values rather than extracted worker-side from headers.Values are structurally unreachable:
mainnow parses viaArgMatches, andtelemetry::capturewalks only arg ids andArgmetadata (neverget_one/get_raw), skipping positionals and non-command-line sources. A unit fixture asserts a payload built fromcloud service get SECRET-ID --org-id SECRET-ORGcontains noSECRET, and an integration test asserts the same over the wire.Transport
Detached child process (
telemetry send, hidden subcommand): payload via child env var, all stdio nulled, parent never waits; child fires one POST with a 2s timeout and dies silently on any failure. Measured ~0.3s command wall-clock with the endpoint blackholed. The child is short-circuited inmainbefore dispatch, so a send can never trigger another send.Every POST carries the canonical User-Agent (
clickhousectl/<version>, optionally(agent=<id>)) via the sharedhttp::client_builder(), pinned by a wire-level test — the ingest worker can reject anything without theclickhousectl/prefix.Transparency & opt-outs
CHCTL_TELEMETRY_DEBUG=1prints the exact payload to stderr and sends nothing.CHCTL_TELEMETRY_URLoverrides the endpoint (tests / local worker dev).telemetry(default-on):--no-default-featurescompiles the whole thing out including the subcommand — enforced by new CI steps.Tests
tests/telemetry_test.rs, subprocess + wiremock + sandboxedHOME): first-run notice/marker/no-send, payload shape + User-Agent prefix, failure exit code + positional non-leak, DNT silence, disable/enable persistence, debug mode, unwritable-HOME fail-open, parent-never-waits latency.DO_NOT_TRACK=1so test runs never write real state or POST to production.Verified end-to-end against the telemetry worker running locally (
wrangler dev): first run silent, subsequent runs delivered events the worker accepted and inserted (200 OK).Worker-side follow-ups (separate repo, in dev)
exit_code(number) replacessuccess(bool) — needs a numeric coercer + column (successis derivable asexit_code = 0).is_agentis now a payload bool (wasagent), andagentis the agent id string (nullfor humans) — needs a column + coercion update intoRow.User-Agentdoesn't start withclickhousectl/.Pre-release TODO (not this PR)
🤖 Generated with Claude Code