feat(telemetry): Go helper so the CLI can emit a conformant event (backend#1897) - #503
Conversation
…ckend#1897)
RFC-BACKEND-1872 D2 for the CLI, and D12's host-process path. The Go half of
what backend#1896 does for the Python services. Unblocks #1907.
The CLI emits nothing today — it is not a pod, so the edge Collector's filelog
receiver cannot reach it, and a field failure is only ever a support thread.
e := telemetry.New(cfg.CurrentEnv, version, hostname)
err := e.Emit("cli.command.failed", telemetry.Attrs{"error.type": "network"})
ENFORCED, NOT DOCUMENTED. The contract's mechanically-checkable rules run at the
call site and return an error: the <domain>.<object>.<outcome> grammar with its
closed vocabularies, the attribute-key namespace, retired names, value types,
and the error set a failure must carry — stacktrace included. It returns rather
than panics, because a CLI must never die of telemetry; but a malformed event
must not pass silently either, and the caller's tests are where it fails.
THE ENVIRONMENT IS DERIVED, NOT RESTATED. `New` classifies via
`api.IsKnownEnv`, the same function that rejects a `--env staging` typo at the
CLI's front door, and a test asserts the two agree across dev/stg/prod/staging/
prd/PROD/"". One saying yes while the other says no is precisely how records
acquire a guessed environment.
The domain vocabulary is narrower than the full registry — `cli` and `auth`
only. The CLI is not the installer and not the backend, so admitting domains it
cannot legitimately produce would make a typo look plausible. That is the
failure already visible in the browser leg, where 461 of 484 events are named
`not_specified`.
25 tests, 100% statement coverage, `make check` green. Twelve rules
mutation-proved.
ONE SURVIVED THE FIRST PASS, and it was a weak test rather than weak code —
the same one the Python side hit. Every key I had tried was caught by the
retired or namespace rule, so nothing exercised the key-SHAPE check; it needed a
key that passes every other rule and is still badly shaped
(`tracebloc.clientID`). Added, and the mutation now bites.
NOTHING IMPORTS THIS YET, deliberately: #1907 is the consumer ticket. One
consequence worth recording rather than discovering — `make deadcode` scans
reachability from ./cmd/tracebloc, so a package outside that import graph is
invisible to it. The gate is SILENT on this package, not passing it, and will
start covering it the moment #1907 wires the first call site.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
shujaatTracebloc
left a comment
There was a problem hiding this comment.
Approve (backend#1897, RFC-BACKEND-1872 D2). The Go half of the telemetry contract (the spec I approved as rfcs#25) — the CLI needs its own emitter because it is not a pod, so no edge Collector filelog reaches it. Enforces the mechanically-checkable rules at the call site and returns errors rather than panicking (a CLI must not die on bad telemetry, but a malformed event must not pass silently — malformed looks like coverage): §6.1 three-segment grammar, closed domain/outcome vocabularies (narrower than the full registry, on purpose), §1.1 key namespace (OTel allowlist or tracebloc.*), §8.5 retired names, §1.2 primitives-only (rejects the extraData map defect), §8.4 failure error.type + caught-exception stacktrace-required. Resource attrs set once at New() (never per call site — the layering bug that made cloud_RoleName the process name), service identity a constant not derived from os.Args[0], unknown env disables export rather than guessing. 20 tests, one per rule incl. sink-separation and resource-is-a-copy. CI green, Bugbot clean.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Reviewed for correctness. This is a clean, well-documented, and impressively well-tested package (25 tests, the mutation-driven key-shape test is a nice touch), and the layering discipline of resource-vs-record is the right shape. Nothing crashes and there are no call sites yet, so none of this is urgent — but a few things are worth fixing before #1907 wires the first caller, since they'd otherwise be discovered in production telemetry. Details inline.
|
@saqlainsyed007 — apologies, this merged before your review was resolved. All five findings are addressed in #507 (assigned to you for review). Each one was reproduced against the merged code first: All five were real. Nothing imports the package yet (#1907 is the consumer), so no production impact — but three of them would have shaped the first call sites. 31 tests, 100% statement coverage, six mutations all caught. |
Implements RFC-BACKEND-1872 D2 for the CLI, and D12's host-process path. The Go half of what backend#1896 does for the Python services. Unblocks #1907.
The CLI emits nothing today. It is not a pod, so the edge Collector's
filelogreceiver cannot reach it — which means a field failure is only ever a support thread.Enforced, not documented
The contract's mechanically-checkable rules run at the call site and return an error: the
<domain>.<object>.<outcome>grammar with its closed vocabularies, the attribute-key namespace, retired names, value types, and the error set a failure must carry — stacktrace included.It returns rather than panics: a CLI must never die because telemetry was malformed. But a malformed event must not pass silently either, and the caller's own tests are where it is meant to fail.
Two decisions worth reviewing
The environment is derived, not restated.
Newclassifies viaapi.IsKnownEnv— the same function that rejects a--env stagingtypo at the CLI's front door — and a test asserts the two agree acrossdev/stg/prod/staging/prd/PROD/"". One saying yes while the other says no is exactly how a record acquires a guessed environment.The domain vocabulary is narrower than the full registry —
cliandauthonly. The CLI is not the installer and not the backend, so admitting domains it cannot legitimately produce would make a typo look plausible. That failure is already visible in the browser leg, where 461 of 484 custom events are namednot_specified.Verification
25 tests, 100% statement coverage,
make checkgreen.Twelve rules mutation-proved. One survived the first pass, and it was a weak test rather than weak code — the same gap the Python side hit. Every key I had tried was caught by the retired or namespace rule, so nothing exercised the key-shape check. It needed a key that passes every other rule and is still badly shaped (
tracebloc.clientID). Added; the mutation now bites.Nothing imports this yet — deliberately
#1907 is the consumer ticket. One consequence worth recording rather than leaving to be discovered:
make deadcodescans reachability from./cmd/tracebloc, so a package outside that import graph is invisible to it. The gate is silent on this package, not passing it, and starts covering it the moment #1907 wires the first call site.The pre-existing stale-allowlist note (
internal/doctor/doctor.go: Status.String) reproduces ondevelopuntouched — not from this change.Closes #1897.
🤖 Generated with Claude Code
Note
Low Risk
Additive library with no production call sites yet; validation is local to telemetry and does not change CLI behavior until wired in #1907.
Overview
Adds
internal/telemetry, a new Go emitter for RFC-BACKEND-1872 so the CLI can send outcome events itself (not via podfilelog). Call sites useEmitter.Emit, which returns errors for invalid events instead of panicking or silently dropping them.Newsets fixed resource identity (service.name/tracebloc.component=cli), normalises empty/devversions to0.0.0-unknown, and only stampsdeployment.environmentwhenapi.IsKnownEnvagrees—unknown envs validate butExports()is false (no guessed env).SetSinkdelivers resource + record separately; nil sink still runs validation.Enforcement at emit time includes three-segment
domain.object.outcomenames (CLI domainscli/authonly), attribute namespace / retired keys / primitive values, omission of nil/empty strings, and failure outcomes requiringerror.typeplus a complete exception set when any exception field is present.~25 tests cover identity, env parity with
IsKnownEnv, grammar, attributes, failures, and sink layering. Nothing incmd/traceblocimports this yet (follow-up #1907).Reviewed by Cursor Bugbot for commit 70c0517. Bugbot is set up for automated code reviews on this repo. Configure here.