feat(telemetry): one outcome event per install, with nowhere to put a path (backend#1907) - #747
feat(telemetry): one outcome event per install, with nowhere to put a path (backend#1907)#747LukasWodka wants to merge 8 commits into
Conversation
… path (backend#1907) The installer is the highest-variance, least-observed step in the product: it runs on machines we have never seen, under package managers, proxies and shells we do not control, and it reports to nobody. Each of the backend#736 failures — the CLI landing in ~/.local/bin with PATH advice only printed, apt-get appearing hung because unattended-upgrades held the dpkg lock — was invisible until a customer happened to mention it. scripts/lib/telemetry.sh emits one contract-shaped event per run from install_cleanup, the EXIT trap, so it fires on every path including the interrupted and the fatal one. It carries the phase reached, per-phase durations, the exit code, the client state, OS/arch, the version, an error class, and — for the #736 PATH case specifically — TB_CLI_ON_FRESH_PATH, which install-cli.sh has always computed and only ever printed advice about. "NO ARGUMENTS, NO PATHS, NO DATA" IS A SHAPE, NOT A RULE. Every value goes through _telemetry_attr, which admits a string only if it matches ^[A-Za-z0-9._-]{1,64}$ and an integer only if it is one. A path contains '/', a proxy credential contains ':' and '@', a token is longer than 64 characters, a name contains a space. Values that fail are dropped, never trimmed: a redactor has to imagine what it is stripping, and a shape only admits what it was told to. The phase and the client state are additionally checked against their closed sets at the render boundary — a canary assigned straight to TB_TELEMETRY_PHASE reached the record before that line existed, and it was shaped exactly like a legal value, so the token regex waved it through. The vocabularies are DERIVED, and a new guard proves it. scripts/tests/telemetry-vocabulary-agreement.sh parses install-k8s.sh's step_header calls, summary.sh's CLIENT_STATE writers, gen-manifest.sh's FILES array and install.sh's release-tag regex, and compares each to telemetry.sh's declaration; the error classes have no second declaration, so it exercises the classifier over the full cross-product and checks both that every answer is registered and that every registered class is reachable. It runs in drift-checks' `Source-of-truth drift` job, which is required — a guard in a job nobody must wait for is advice. It found one thing on its first run: summary.sh's own CLIENT_STATE docstring had been missing image_pull_ca since #424. TWO REAL BUGS THE TESTS CAUGHT, both of the same shape and both fatal: `printf | grep -q` returns 141 on a match under `set -o pipefail`, and so does `tr -dc < /dev/urandom | head -c 16` — the latter at SOURCE time, which killed the whole installer before it printed a line. Every unit-level test passed throughout; only the test that runs install_cleanup for real under the installer's own shell options went red. That test stays. install-bootstrap.bats held a hand-written second copy of install.sh's FILES array, in two places, so adding a lib turned ten unrelated supply-chain tests red. It now derives the list, and fails closed on an inert parse. WHAT IS NOT CONNECTED: the transport. The 17 Aug decision (rfcs#28) replaced the Collector gateway with an ingest endpoint on the backend — backend#1905, which does not exist yet — so _telemetry_deliver writes the install log and a bounded 0600 local spool that #1906's forwarder can drain, and posts nothing. Opt-out (default on) via TRACEBLOC_NO_TELEMETRY or DO_NOT_TRACK, documented in --help — and that promise is itself checked, because a user who exports a stale name believes they have opted out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
bugbot run |
|
Sibling PR for the CLI half of backend#1907: tracebloc/cli#527. Independent — neither depends on the other, and the transport for both is tracebloc/backend#1905. |
…ixture (backend#1907) gitleaks' curl-auth-user rule fired on the TB_ERR_CMD fixture, and it was right to: a source file containing that spelling is a finding whatever the surrounding test claims, and a reviewer scanning the diff has no way to tell a canary from the real thing at a glance. The fixture's purpose is unchanged — TB_ERR_CMD holds the failing command UNEXPANDED, which is free text carrying a path, and must not be emitted. It now carries a path instead of a credential. The credential half of the same test is already covered by HTTPS_PROXY, which encodes user:pass in a proxy URL and is what a hospital network actually configures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
bugbot run |
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
bugbot run |
Bugbot round 1 — one fixed, one demoted with evidence✅ "Non-install exits report success" (High) — real, fixedReproduced exactly as described: This is the worst possible bug in this PR and the severity is right: Fix: a run-started latch, not a phase test. Second, related thing found while fixing it: the assess handoff exits 0 Not covered, deliberately:
|
| mutation | reddened |
|---|---|
| the run-started latch removed | 2 tests |
| the latch set at source time (always on) | 1 test |
main stops setting the latch (feature dead) |
1 test |
| the skipped handoff reported as succeeded | 1 test |
assess.sh stops marking the handoff skipped |
1 test |
| a bootstrap failure classified as something else | 1 test |
Two of those came back inert on the first pass — "main stops setting the
latch" and "assess.sh stops marking the handoff" — because the tests set the
flags themselves and so could not see the wiring disappear. That is the same
class as the bug you found: a unit test of the emitter cannot observe which exits
reach the trap. Both are now driven end to end through the real entrypoint (a
validate_config rejection, which lands after the latch and before step a) and
through the real _assess_handoff with tracebloc mocked. Both mutations redden
now.
26 tests in telemetry.bats, make check green.
install_cleanup is the EXIT trap, so it fires for every exit of install-k8s.sh —
including the terminal commands that touch no machine. `--help` exits 0 and was
emitting a full install.run.succeeded with phase `bootstrap`. Reproduced:
$ HOST_DATA_DIR=$T bash scripts/install-k8s.sh --help
{"attributes":{"event.name":"install.run.succeeded",
"tracebloc.install.phase":"bootstrap","tracebloc.install.exit_code":0,…}}
That is the worst bug this feature could have. `--help` is the command people run
MOST while a real install is broken, so a free success lands in the denominator
of the failure RATE — the one number the ticket exists to produce — and moves it
in the direction that hides the problem. Found by Bugbot on client#747.
The fix is a latch, not a phase test: main() calls telemetry_run_started once
--help / --diagnose / prepare-host have had their chance to dispatch, and
telemetry_emit_outcome returns early without it. A phase test would have been
wrong — a genuine failure IN the bootstrap phase (the leftover-data guard,
validate_config) is an install attempt and must still be reported, which is now
pinned by a test that drives the real entrypoint into a validate_config
rejection.
Found while fixing it: the assess handoff exits 0 having run no step, so
counting it as succeeded would grow the success count with re-runs on machines
nothing happened to. It now emits install.run.skipped — a registered outcome
verb (contract §6.4), so no new vocabulary — which also makes "how often is the
installer re-run on a machine that was already done" answerable.
prepare-host is deliberately still not reported: it is a different command with
its own registry component (§10.1), and filing it under
tracebloc.component=install would be mislabelling it rather than measuring it.
TWO OF THE SIX NEW MUTATIONS CAME BACK INERT on the first pass — "main stops
setting the latch" and "assess.sh stops marking the handoff" — because the tests
set those flags themselves and so could not see the WIRING disappear. That is
the same class as the bug Bugbot found: a unit test of the emitter cannot observe
which exits reach the trap. Both are now driven end to end, through the real
install-k8s.sh and the real _assess_handoff, and both mutations redden.
Bugbot's second finding (the source lookup aborting the whole event under set -e)
is reported as unreachable with evidence rather than fixed: a command
substitution in an ARGUMENT position does not propagate its status to the
enclosing command, measured on bash 3.2.57 and 5.x. The invariant is pinned
anyway — an unrecognised source location drops the field, never the event.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 29495f7. Configure here.
…kend#1907)
telemetry_phase_begin only closes a phase when the NEXT one starts, and nothing
closed the active phase before the event rendered. That lost the most important
number in the file, and it lost it in exactly the case the ticket names:
* on every SUCCESSFUL install, phase_connect_ms was absent — the readiness
wait, up to READY_TIMEOUT (600s), the single longest phase;
* on every failure and every cancel, the phase named by
tracebloc.install.phase had no duration. The dpkg-lock case in its likeliest
real form is stuck twenty minutes in `prerequisites` and then killed or given
up on, never reaching step c. Reproduced before fixing:
"tracebloc.install.phase":"prerequisites"
"tracebloc.install.phase_preflight_ms":0
<no prerequisites duration at all>
* `bootstrap` had no key at all, because the loop iterated the letter map and
bootstrap has no step letter. So the download + verify + leftover-guard +
assess time was an unnamed remainder — which is also why subtracting the
other keys from duration_ms could not recover the missing active phase.
Found by Bugbot on client#747. My own "a slow phase is visible" test passed
throughout, because it only ever measured a phase a later step_header had
closed — the exact shape of a test that proves the easy half.
The live delta is added at READ time rather than by a "close the phase" call in
the emit path, so render stays idempotent: the tests call it repeatedly, and a
render that mutated the accumulators would report different numbers each time.
The clock is now read once per event, so the per-phase numbers and the total are
exactly consistent — which is an invariant a test asserts.
That test needed a FAKE CLOCK. The obvious fixture is wrong: winding
_TB_TELEMETRY_PHASE_STARTED_MS backwards after the step_headers have already
attributed that time invents milliseconds that never elapsed, and the first
version failed for precisely that reason (sum 1200000 vs total 900000).
Four mutations run. Three redden — dropping the live delta, going back to the
letter map, and counting the delta for every phase. The fourth (reading the clock
per attribute instead of once) is INERT and is reported as inert rather than
counted: _telemetry_now_ms has second resolution, so two reads inside the same
second are identical, and the guard only matters across a second boundary.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bugbot round 2 — the third finding was real too, and it hit the DoD claimI missed this one in my first reply: it came from Bugbot's earlier review pass ✅ "Final phase duration never recorded" (Medium) — real, fixed
This undercut the DoD claim in the PR body. I wrote that per-phase durations And as you noted, the remainder trick could not recover it, because My own test passed throughout. "A slow phase is visible on a run that Fix: the live delta is added at read time, not by a "close the phase" call New invariant, asserted: the phase durations sum EXACTLY to That test needed a fake clock, and the reason is worth recording: the obvious Mutation proofs
The fourth is reported as inert rather than counted: Running total across both rounds3 findings, 2 fixed, 1 demoted with measured evidence. Two of my own 29 tests in
|
|
bugbot run |
…refused (backend#1907)
_telemetry_deliver ran from the EXIT trap and did `mkdir -p
"$HOST_DATA_DIR/telemetry"` unconditionally — including on the path where
early_data_dir_guard had just REFUSED that directory for being on a network
filesystem and called `error`.
That guard deliberately skips an existing directory ("an EXISTING data dir has no
at-risk mkdir here", client#441), so anything creating the directory behind its
back disarms it for every later run:
run 1: guard refuses (dir absent) -> error -> EXIT trap -> telemetry creates it
run 2: guard sees the dir, returns 0 -> MySQL installs onto NFS
which is exactly the InnoDB corruption client#432 exists to prevent, reintroduced
by the telemetry that was only supposed to watch. Reproduced before fixing:
guard exit=1
HOST_DATA_DIR created on the REJECTED volume? YES
./nfs-volume/.tracebloc/telemetry
An observer that changes the install's own preconditions is not an observer. The
spool now only writes INTO a data dir that already exists; a run that dies before
that still reports through the install log, which is what a support bundle
collects and which _choose_log_file has already placed somewhere safe (falling
back to $TMPDIR).
Found by Bugbot on client#747 — the third real finding of three rounds, and the
most serious: the other two corrupted the metric, this one corrupted a customer's
database.
Both directions mutation-proven: removing the existence check reddens the new
test, and disabling delivery outright reddens four others, so the fix cannot pass
by simply turning the feature off.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bugbot round 3 — the most serious one yet, fixed✅ "Spool mkdir undoes NFS guard" (High) — real, and worse than the first twoThe first two findings corrupted a metric. This one corrupted a customer's
which is the InnoDB corruption client#432 exists to prevent, reintroduced by the The principle I got wrong: an observer that changes the install's own Fix: the spool only writes into a data dir that already exists. A run that Both directions mutation-proven, because "don't write" is trivially satisfied
The regression test carries an anchor asserting the guard actually refused Final tally across three rounds4 findings, 3 fixed, 1 demoted with measured evidence. In order of what they
Three of those were invisible to my own tests, and the pattern is consistent: all 30 tests in
|
|
bugbot run |
…re (backend#1907) Fallout from the previous commit, and the worst kind: the fix that stopped telemetry disarming the NFS guard also made the NFS refusal itself invisible. _telemetry_deliver's comment claimed "the install log always gets it". It does not. `log` is a no-op until setup_log_file sets LOG_FILE, and setup_log_file runs AFTER validate_config and early_data_dir_guard — deliberately, because #432 refuses a network data dir BEFORE logging starts. So on exactly those paths there was no log AND (correctly, since the previous commit) no data dir, and the rendered event was discarded: $ early_data_dir_guard # target reads as nfs guard exit=1 any telemetry written anywhere? 0 A run refused for being on NFS is a real, actionable field failure, and it was the single case producing no record at all — invisible to the very failure rate this feature exists to produce. Those pre-log failures are also precisely the class the run-started latch was built to preserve, so losing them undid that too. Found by Bugbot on client#747, which also spotted that this file's own NFS test masked the bug by setting LOG_FILE=/dev/null. That test now leaves LOG_FILE unset, as the real path does, and asserts the refusal IS reported. Fix: when there is no data dir, spool to a mktemp'd file in $TMPDIR. mktemp and not a fixed name — /tmp is world-writable on Linux and the installer runs privileged steps, so a predictable path is a symlink target for an append that may be running under sudo; mktemp creates with O_EXCL. This mirrors _choose_log_file's own fallback, so an early-failure run leaves one small file beside the install log it already leaves there, rather than a new class of litter. #1906's forwarder reads both locations. A comment that claims something untrue is itself the defect (workspace CLAUDE.md rule 7), so the false claim is replaced with what actually holds and why. Four mutations, all reddening: removing the fallback, giving it a predictable shared path, dropping its 0600 mode, and dropping the data-dir existence test (which would disarm the NFS guard again). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bugbot round 4 — right again, and it was fallout from my own round-3 fix✅ "Early failures drop outcome events" (High) — real, fixedThis is the sharpest of the four, because it is a consequence of the previous Reproduced:
Those pre-log failures are also precisely the class the run-started latch was And you were right about the test. My NFS test set Fix: when there is no data dir, spool to a The false comment is replaced with what actually holds and why — a comment that
Final tally — 5 findings across 4 rounds4 fixed, 1 demoted with measured evidence. Every single real one was about
No unit test of 31 tests in
|
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 46a33de. Configure here.
saadqbal
left a comment
saadqbal
left a comment
There was a problem hiding this comment.
Four findings, all about which runs reach the emitter or what gets past the shape guard — nothing wrong with the render logic itself. The $TMPDIR one is the significant one: it quietly undoes the round-4 fix on the primary macOS path.
Two things that don't fit on a line in the diff:
install.run.skippedwill almost never fire on thecurl | bashpath.install.sh:144exec traceblocs on a healthy machine before install-k8s.sh is fetched at all, so assess.sh's gate only sees direct./install-k8s.shre-runs. Fine, just much narrower than "how often do people re-run an installer that was already done".quality / gitleaksis still red. Your comment explains it and what it needs; noting it's the one thing blocking merge.
Vocabulary guard and the 31 bats tests are green here on 46a33de.
…tes (backend#1907) Four findings from @saadqbal's review, each reproduced before it was fixed, plus the nit. The first one is the significant one: it silently undid round 4 on the primary macOS path. 1. The fallback spool landed inside the bootstrap's own scratch dir. install.sh:238 does `TMPDIR="$(mktemp -d)"` and :239 traps `rm -rf "$TMPDIR"`. A plain assignment to a name that is ALREADY EXPORTED keeps the export attribute — and TMPDIR is always exported on macOS — so install-k8s.sh inherited the doomed directory and `mktemp "${TMPDIR:-/tmp}/…"` wrote the record into it. Reproduced end-to-end: the spooled file was gone the moment the bootstrap returned. So the NFS refusal, and every other pre-setup_log_file failure, still produced no record anywhere on macOS — the exact hole the fallback closed. _telemetry_fallback_dir now disqualifies TMPDIR when the running installer is inside it, which is true precisely when TMPDIR is the bootstrap's scratch dir, and falls back to $HOME (never $HOME/.tracebloc — telemetry must not create HOST_DATA_DIR) and then /tmp. DERIVED rather than agreed: asking install.sh to export its original TMPDIR under another name would work only when the bootstrap is new, and install.sh is served from a URL a user may have curl'd months ago. Both sides of that comparison are resolved with `pwd -P`. The first cut compared them as written and missed every Mac, because /var is a symlink to /private/var — caught by re-running the reproduction against the fix, not by reading it. 2. grep is line-based, so a value with a newline passed the shape check. The one input shape "nowhere for a path to go" does not cover, because what lands is not a path — it is a second line: TB_VERSION=$'v1.9.3\n","tracebloc.install.injected":"yes' → "service.version":"v1.9.3 ","tracebloc.install.injected":"yes",… A forged attribute AND one record split across two lines of a .jsonl spool, so #1906's forwarder reads two malformed events. Reproduced on all four checks — the key, str and int shapes in _telemetry_attr, _telemetry_version, and _telemetry_source_line's inline regex. All now `[[ =~ ]]`, which anchors at end of string. As a bonus it removes the external process, so the backend#1778 SIGPIPE class the here-strings were working around cannot recur here at all. The agreement check proved the two version regexes were byte-identical while they behaved differently, and reported that as "the service.version shape is install.sh's own release-tag gate" — a claim about behaviour that byte-identity does not support, because each side was matched with a different operator. It now checks both: byte-identity, then verdict agreement over a corpus, each side evaluated the way the file that owns it evaluates it. The corpus is written down independently of either matcher and the check fails closed if it contains no embedded-newline input, since without one it degenerates into the byte check. 3. exit 2 is the "complete this step and re-run" handoff, not a failure. gpu-nvidia.sh:55 exits 2 after install_nvidia_drivers SUCCEEDED, to ask for a reboot. That call sits under step_header b, so every unattended GPU host's first install booked an `install.run.failed` with error.type=prerequisites_failed — a fabricated prerequisite failure in the rate this ticket exists to produce. Same shape as the --help bug, opposite direction; install_cleanup has treated 2 as its own outcome ("Re-run required") since client#681. It now renders install.run.cancelled and carries no error.type. It rides an existing verb rather than a new one because §6.4's outcome list is closed and adding to it is a PR against the contract, not an emitter's unilateral call; of the registered verbs, `cancelled` is the only terminal one that is true here. exit_code stays on the record, so 2 (handoff) and 130/143 (Ctrl-C) remain separable — which is why the exit code is an attribute in the first place. Event names are now a declared closed set with a guard. The guard derives the emitted names two ways — the literals in the case statement, and what the function actually renders over the installer's exit codes — rather than reading the declaration twice, and checks §6.1's grammar. It cannot check the §6.4 half from this repo: the verb registry is in rfcs, and a hand-copied second list of verbs would be the defect rather than the fix. 4. The chmod ran before the trim replaced the file. `tail > "${spool}.tmp"` creates under the process umask and `mv` keeps the tmp file's mode, so 0600 did not survive. common.sh's `umask 077` normally covers it, but _install_userspace_tools (setup-linux.sh:893) and its macOS twin set `umask 022` and restore it only afterwards. Reproduced: spool 644. The chmod now runs on the inode that survives, before the mv — one chmod, not two, because a second one on the spool afterwards is unreachable belt and braces that no test can redden. The test pinning 600 could not see any of this, because load_lib sources common.sh first and every test therefore ran under 077. There is now one that sets umask 022 and asserts the umask actually took. 5. nit: the comments claimed coverage the file does not have. `bootstrap` means "install-k8s.sh before step a", not "everything before step a" — download and verify happen in install.sh, which never sources this file and whose EXIT trap is `rm -rf "$TMPDIR"`. install.sh in TB_TELEMETRY_SOURCES is unreachable for the same reason: TB_ERR_LOC has exactly one writer, common.sh's _record_err. Both comments now say so. No bootstrap telemetry added. Also noted on install.run.skipped, which reads wider than it is: install.sh:132 reaches a healthy machine and `exec tracebloc`s at :144 before install-k8s.sh is fetched, so on curl|bash the assess gate is never reached at all. Tests: 4 new bats tests (36 in telemetry.bats, 1161 across the suite, all green), 3 new checks in the agreement guard. Every fix mutation-proved: 11 mutations, 11 reddened, each with its anchor asserted. Two first-pass mutations came back inert and are fixed rather than counted — one removed a redundant chmod nothing could observe (the redundancy is now gone), the other rewrote the guard's own detector alongside its corpus so the detector matched its mutated needle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit eaa848a. Configure here.
saadqbal
left a comment
saadqbal
left a comment
There was a problem hiding this comment.
All five addressed, and the fixes are better than what I suggested in two places — deriving "is the installer running inside this directory?" instead of asking install.sh to export a second name is the right call for a bootstrap users may have curl'd months ago, and the pwd -P catch on both sides is the kind of thing only re-running the reproduction finds.
Verified on this head rather than taken: the agreement check is green including the new behavioural version corpus and the exit-2 branch; bats scripts/tests/telemetry.bats is 36/36; and I re-checked the four shape guards under /bin/bash 3.2.57 — [[ =~ ]] refuses every embedded-newline input and still admits the legal spellings, and the {1,15} interval works there.
One non-blocking note below on exit 2. quality / gitleaks is still the only red check and still needs the squash from a human.
…ange scripts/gen-manifest.sh output, required by the Static analysis R8 gate after any installer script changes (backend#1907). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a00b1c9. Configure here.
| rm -f "${spool}.tmp" 2>/dev/null || true | ||
| fi | ||
| fi | ||
| return 0 |
There was a problem hiding this comment.
Spool failure drops outcome event
High Severity
When HOST_DATA_DIR exists but creating or appending the data-dir spool fails, _telemetry_deliver returns immediately and never tries the fallback spool. That path is exactly when _choose_log_file already fell back because the data dir was not writable, so the outcome event is lost — the same invisibility hole the fallback was added to close.
Reviewed by Cursor Bugbot for commit a00b1c9. Configure here.
| if [ -n "${TB_ERR_LOC:-}" ]; then | ||
| _telemetry_attr "tracebloc.install.source" "$(_telemetry_source_basename "$TB_ERR_LOC")" | ||
| _telemetry_attr "tracebloc.install.source_line" "$(_telemetry_source_line "$TB_ERR_LOC")" int | ||
| fi |
There was a problem hiding this comment.
Orphan source line without file
Low Severity
tracebloc.install.source and tracebloc.install.source_line are gated independently. When TB_ERR_LOC has a numeric line but a basename outside TB_TELEMETRY_SOURCES, the line attribute is still emitted with no matching source, so dashboards get a meaningless orphan line number.
Reviewed by Cursor Bugbot for commit a00b1c9. Configure here.
saadqbal
left a comment
saadqbal
left a comment
There was a problem hiding this comment.
Re-approving — the marker is the right shape and goes further than the note asked for. Declaring at the exit 2 site, clearing at source time so an inherited _TB_TELEMETRY_RERUN_HANDOFF=1 can't pose as a handoff, and failing closed toward failed for an undeclared 2 all land the way round I'd want. unexpected_exit_2 as its own row rather than the phase bucket is better than what I suggested: a stray 2 stays distinguishable from a real prerequisite failure instead of just being counted.
(d4) deriving the site list from grep -n 'exit 2' over the runtime, and failing closed on zero sites with the TRACEBLOC_DOCKER_FIRST_RUN_EXIT precedent named, is the part that will still be true in a year.
Verified on a00b1c9: agreement check green (13 checks, including both exit-2 directions and the site sweep), bats scripts/tests/telemetry.bats 40/40, gpu-nvidia.bats green, gen-manifest.sh --check current, shellcheck -S warning -x clean on the three touched files apart from the pre-existing SC2034 at gpu-nvidia.sh:253. dispatch_gpu_setup is a plain call chain from install_linux, no subshell, so the marker really does survive to the trap — test 30 pins it.
Two follow-ups, neither for this PR: install_cleanup still prints "Re-run required. Complete the step above" for an undeclared 2, so the user gets the message telemetry now correctly refuses to believe — it can key on the same marker. And the dead TRACEBLOC_DOCKER_FIRST_RUN_EXIT read at common.sh:1022 deserves its own ticket now that you've traced its producer to 8c3a3d4.
quality / gitleaks is still the only red check, still needs the squash.
2 participants
Parent epic: tracebloc/backend#1872 · Ticket: tracebloc/backend#1907 · Contract: the telemetry contract · Sibling: tracebloc/cli#527
The installer is the highest-variance, least-observed step in the product: it
runs on machines we have never seen, under package managers, proxies and shells
we do not control, and it reports to nobody. Each of the backend#736 failures
was invisible until a customer happened to mention it.
scripts/lib/telemetry.shemits one contract-shaped event per install, frominstall_cleanup— the EXIT trap — so it fires on every path including theinterrupted one and the one that dies under
errexit. That is where §6.5's"terminal event on every path" is actually honoured; anywhere else and the
failure rate is uncomputable.
What is emitted
install.run.succeeded/.failed/.cancelled, with:tracebloc.install.phasepreflight…connect, from the closed a–f settracebloc.install.phase_<name>_mstracebloc.install.duration_ms/.exit_codetracebloc.install.client_statetracebloc.install.cli_on_patherror.typetracebloc.install.source/.source_lineservice.name/tracebloc.componentinstaller/install(§10.1)service.version,os.type,host.arch,deployment.environment,service.instance.idCtrl-C (130/143) is
cancelledand carries noerror.type."No arguments, no paths, no data" is a shape, not a rule
Every value goes through one writer,
_telemetry_attr, which admits a stringonly if it matches
^[A-Za-z0-9._-]{1,64}$and an integer only if it is one. Afilesystem path contains
/. A proxy credential contains:and@. A tokenis longer than 64 characters. A person's name contains a space. Values that
fail are dropped, never trimmed or escaped — a value that had to be repaired
to be safe is a value we did not understand, and shipping our guess about it is
how a redactor leaks.
The phase and the client state are additionally checked against their closed
sets at the render boundary. That line exists because of a test failure: a
canary assigned straight to
TB_TELEMETRY_PHASEreached the record, and it wasshaped exactly like a legal value, so the token regex waved it through.
telemetry_phase_beginis the only writer and already closes the set — but"the only writer is careful" is a property that stops being true silently.
TB_ERR_CMDis deliberately never emitted (unexpanded command text is stillfree text), and
service.instance.idis a fresh per-run value rather than thehostname — field hostnames here are overwhelmingly
<firstname>-macbook, which§7.3 forbids outright.
The vocabularies are derived, and a required check proves it
scripts/tests/telemetry-vocabulary-agreement.shholds no copy of anyvocabulary. It parses:
install-k8s.sh'sstep_headercalls → the phase letterssummary.sh'sCLIENT_STATEwriters → the client statesgen-manifest.sh'sFILESarray → the source basenamesinstall.sh's immutable-release-tag regex → theservice.versionshapecommon.sh'sprint_help→ the documented opt-out variablesand for the error classes — which have no second declaration to parse — it
exercises
telemetry_error_classover the full cross-product of its twoclosed input sets, checking both that every answer is registered and that
every registered class is reachable. Comparing a declaration to itself would be
self-consistent and therefore blind.
It runs in
drift-checks.yaml'sSource-of-truth driftjob, which is arequired check — a guard in a job nobody must wait for is advice, not a gate.
It found something on its first run:
summary.sh's ownCLIENT_STATEdocstring listed five states while
_diagnose_not_readyproduces six(
image_pull_ca, added by #424, never made it into the comment). Fixed here.Two real bugs the tests caught — both fatal, both invisible to unit tests
Same shape twice:
grep -qcloses the pipe at its first hit, so under theinstaller's
set -o pipefaila producer takes SIGPIPE and the pipeline returns141 — on a successful match. This is the backend#1778 defect, already
documented in
summary.sh's own comments.printf '%s' "$key" | grep -qE …in the shape checks.tr -dc 'a-f0-9' < /dev/urandom | head -c 16for the instance id — this oneat source time, so it killed the entire installer under
set -ebeforeit printed a line.
Every unit-level test passed throughout both. The only thing that went red was
the test that drives
install_cleanupfor real, under the installer's own shelloptions. That test stays, and a second one now exercises the whole surface under
set -euo pipefailexplicitly.Mutation proofs
Every guard broken, watched go red, restored — and every anchor asserted to
have applied, because an inert mutation and real coverage are identical in a
log.
CLIENT_STATEpassed through unchecked_telemetry_attrsanitises instead of droppingurandom | headSIGPIPE pipeline restoredCLIENT_ENVexported under a guessstep_headerstops driving the phase clocksummary.shgains an unknown client stateinstall.shTwo mutations were rerun after strengthening a test that had stayed green:
CLIENT_STATEand the phase were being tested with a value containing/,which
_telemetry_attr's shape refuses regardless — so the test proved theshape guard and said nothing about the vocabulary guard. It now uses
degraded/verifying, which are shape-legal and exactly what a future editto
summary.shwould produce.One mutation came back inert and is reported as such rather than counted:
putting
_telemetry_attr's key check back through a pipe reddens nothing,because for a short key
printfcompletes beforegrep -qexits and no SIGPIPEoccurs. The here-string is still the correct idiom (and is what fixes the value
checks, where the input can be long), but this PR does not claim coverage it
does not have.
What remains to be connected
The transport, and only the transport. The ticket said "rides the gateway
and token"; the 17 Aug decision replaced the Collector gateway with an ingest
endpoint on the backend (rfcs#28),
which is tracebloc/backend#1905, in flight and not yet available.
_telemetry_delivertherefore writes the install log and a bounded, 0600local spool at
$HOST_DATA_DIR/telemetry/pending.jsonl(last 50 events, in a0700 directory) — which tracebloc/backend#1906's forwarder can drain — and posts
nothing. That one function is the whole change when #1905 lands.
The ticket body has been corrected so it no longer says "blocked by the gateway".
Would the three named field failures now be visible?
~/.local/bin, PATH advice only printedtracebloc.install.cli_on_path=0rides every event.install-cli.shhas always computed this — it asks whether a fresh login shell resolvestracebloc— and only ever printed advice about it. It becomes a rate.apt-gethung on the dpkg locktracebloc.install.phase_prerequisites_msrides the success event too — that failure produces no non-zero exit at all, it just takes twenty minutes. It also rides the cancel and failure events, for the phase that was still running when the process ended, which is the likelier real shape: stuck twenty minutes and then killed. (That second half was broken in the first two commits and is Bugbot finding 3 — see the comments; the phase durations now sum exactly toduration_ms, which is asserted.)cluster infoon the wrong kubeconfig contextTwo of the three are this PR's; the third is the sibling's.
Also here
install-bootstrap.batsheld a hand-written second copy ofinstall.sh'sFILESarray, in two places — so adding one lib turned ten unrelatedsupply-chain tests red. It now derives the list from the array (the same parse
gen-manifest.shuses) and fails closed on an inert parse. That is a landmineremoved, not a workaround for this PR.
scripts/manifest.sha256regenerated;telemetry.shadded to bothFILESarrays, which
gen-manifest.shcross-checks.--helpgains a "Usage reporting" section; the copy golden is regenerated.Review history
Bugbot found 5 issues across 4 rounds: 4 fixed, 1 demoted with measured
evidence (the mechanism does not exist on bash 3.2 or 5.x). In order of what they
would have cost:
mkdirdisarmed the NFS guard —_telemetry_delivercreatedHOST_DATA_DIRfrom the EXIT trap on the very path whereearly_data_dir_guardhad just refused it, and that guard skips an existingdirectory by design. Run 1 refused and created it; run 2 installed MySQL onto
NFS. Customer database corruption, reintroducing client#432.
--helpemittedinstall.run.succeeded— a free success in the denominatorof the failure rate this ticket exists to produce.
its likeliest form, and
phase_connect_msabsent on every success. Thisundercut the DoD claim in the table above.
logis ano-op until
setup_log_file, which runs after the guards, so the NFS refusalitself became permanently invisible. Events now fall back to a
mktemp'd filein
$TMPDIR. My own NFS test had been masking this by settingLOG_FILE=/dev/null.argument position does not propagate its status, measured on bash 3.2 and 5.x.
All four real ones were invisible to my own tests, for the same reason: they
were about which code paths reach the emitter, not about what the emitter does
with its inputs — and one of my tests was actively masking a bug. No unit test of
telemetry_render_eventcan see any of them.Every replacement test now drives the real
install-k8s.sh,install_cleanup,_assess_handoff, orearly_data_dir_guard. Full reproductions and mutationtables are in the PR comments.
Two of my own mutations came back inert and were fixed by moving the tests end
to end rather than by counting them; a third is reported as inert rather than
counted. Details in the comments.
Test plan
make checkgreen (lint, drift incl. the new guard, helm-lint, helm-vocab);bats scripts/tests/*.batsgreen (31 new tests);shellcheck -S warning -xclean on every new file;bash -nclean;gen-manifest.sh --checkcurrent; the mutations above plus 10 more from the review rounds (see comments).
🤖 Generated with Claude Code
Note
Medium Risk
Changes the installer’s universal EXIT trap and adds telemetry with strict privacy rules; mistakes could misreport failure rates or leak data, but behavior is heavily guarded, opt-outable, and covered by required drift checks and bats.
Overview
Adds one contract-shaped outcome event per install (
install.run.succeeded/.failed/.cancelled/.skipped) via newscripts/lib/telemetry.sh, emitted frominstall_cleanupon every exit path oncemain()commits past terminal commands (--help/--diagnose/prepare-hostdo not count).Wiring:
telemetry.shis fetched in bootstrapFILESand sourced frominstall-k8s.sh; phase timings hookstep_header; assess “already set up” callstelemetry_run_skipped; GPU reboot handoff callstelemetry_rerun_handoffbefore deliberateexit 2. Values are shape-filtered (no paths/credentials); transport is install log plus bounded0600spool (no network post until backend#1905).Guards & tests: Required
telemetry-vocabulary-agreement.shderives phases, client states, sources, and error classes from producers;telemetry.batsand CI/shellcheck/Makefile/drift-checksinclude the new script.install-bootstrap.batsnow parsesinstall.sh'sFILESinstead of duplicating the list.summary.shdocumentsimage_pull_ca;--helpdocuments usage reporting and opt-out.Reviewed by Cursor Bugbot for commit a00b1c9. Bugbot is set up for automated code reviews on this repo. Configure here.