fix(telemetry): five review findings on the merged Go helper (backend#1897) - #507
Conversation
…#1897)
cli#503 merged before its review was resolved. All five findings are real; each
was reproduced against the merged code before anything changed, and each is
mutation-proved. Nothing imports this package yet (#1907 is the consumer), so
there is no production impact — but three of these would have shaped the first
call sites, which is exactly when they would have been expensive.
1. RESOURCE-SCOPE KEYS WERE ACCEPTED AS RECORD ATTRIBUTES. One flat `otelAttrs`
allowlist mixed the layers, so a call site could smuggle `service.name` into
the record and contradict its own process identity — the cloud_RoleName
cross-layer confusion, at the call site of the package meant to close it.
`TestTheSinkReceivesResourceAndRecordSeparately` passed only because the
caller happened not to pass one.
Now two sets. `tracebloc.component`/`tracebloc.tenant.id` are included even
though correctly prefixed — the namespace rule alone waved them past. And
`event.name` is refused: it is Emit's first ARGUMENT, so accepting it let a
caller replace the name after the grammar and failure-set checks had run.
2. THE PRIMITIVE SWITCH WAS NARROWER THAN GO'S SCALARS. A type switch matches
the DYNAMIC type, so `case int64` never matched `time.Duration` — an
idiomatic caller writing `Attrs{"tracebloc.elapsed": elapsed}` was told their
duration was the retired extraData defect. Now switches on reflect.Kind, so
int8/32, uint*, float32 and named types over them all pass.
Added `Duration(d) int64` returning MILLISECONDS, and the reason is that
time.Duration is an int64 kind and would otherwise pass as a raw nanosecond
count — a number nobody reading a dashboard can interpret.
3. AN EMPTY service.instance.id WAS STAMPED. os.Hostname() returns "" on error.
Omitted now: the "sent as empty rather than omitted" defect the record layer
already refused, which the resource layer did not.
4. DELIVERY IGNORED Exports(). An emitter for an unrecognised env delivered
anyway if a sink was installed, so "unknown never exports" lived in caller
discipline at every #1907 call site. Gated now — validation still always
runs, so a bad event fails in CI wherever the binary is built.
5. A BAD KEY WITH AN EMPTY VALUE PASSED SILENTLY. `normalise` continued on
nil/empty BEFORE validating the key, so `Attrs{"experimentKey": nil}` raised
nothing — contradicting this package's own "a malformed event must not pass
silently". The key is validated first now; a GOOD key with an empty value is
still dropped rather than rejected.
Finding 1 is the same defect Bugbot found independently in the Python sibling
(backend#1996); both are fixed the same way.
31 tests, 100% statement coverage, `make check` green. Six mutations, all caught.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
bugbot run |
… value check
Bugbot, and it is fallout from finding 2 in this same PR — the honest kind. I
widened `checkAttrValue` to accept every string KIND so `type Reason string`
would pass, and left the omit rule type-asserting to builtin `string`. So an
empty named string was accepted as a value and never recognised as absent:
`Attrs{"tracebloc.reason": Reason(" ")}` landed on the record, reopening §1.2
for exactly the callers the widening was for.
The consequence is worse one line down. On a failure, an empty named
`error.type` satisfies `checkFailureSet` by key presence alone — a failure that
cannot be grouped, reported as one that can. That is the whole point of the
required-error.type rule, defeated by a type assertion.
`absentValue` now asks by reflect.Kind, the same question `checkAttrValue`
asks, because the two have to agree on what a string is. Zero numbers and
false bools stay data: they are measurements that happen to be falsey.
Three tests — the drop, the failure-set consequence, and the other half (a
named string with content arrives, 0 and false are kept). Mutation-proved:
restoring the `value.(string)` assertion reddens two of them.
The Python sibling had the same class in a different container (arrays bypassed
both the omit and size rules); fixed there in backend#1996.
Coverage stays 100%. `make check` green.
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 c3a9210. Configure here.
shujaatTracebloc
left a comment
There was a problem hiding this comment.
Approve (backend#1897) — CI green, no open Bugbot. Five review findings on the merged Go telemetry helper (cli#503), each a real hardening: the flat allowlist splits into resourceScope vs recordScope (the cross-layer confusion that made cloud_RoleName report a process name, reintroduced at the call site of the package meant to close it); the attribute key is validated BEFORE the empty-value drop (so a retired/malformed key carrying an empty value no longer passes silently); absentValue tests by KIND not concrete type (a named string type type Reason string could otherwise smuggle an empty error.type past checkFailureSet); validation always runs while delivery is gated on Exports() (the "unknown env never exports" guarantee no longer depends on every call site remembering not to install a sink); and service.instance.id is omitted rather than stamped empty when os.Hostname() fails. A zero/false value is correctly NOT treated as absent. Mirrors the same fixes landing on the Python sibling (backend#1996). Clean.
cli#503 merged before its review was resolved. This addresses all five of @saqlainsyed007's findings.
All five are real. Each was reproduced against the merged code before anything changed:
Nothing imports this package yet (#1907 is the consumer), so there is no production impact — but three of these would have shaped the first call sites, which is precisely when they'd have become expensive.
The five
1 · Resource-scope keys accepted as record attributes. One flat allowlist mixed the layers, so a call site could contradict its own process identity — the
cloud_RoleNameconfusion, at the call site of the package meant to close it.TestTheSinkReceivesResourceAndRecordSeparatelypassed only because the caller happened not to pass one.Now two sets.
tracebloc.component/tracebloc.tenant.idare included even though correctly prefixed — the namespace rule alone waved them past. Andevent.nameis refused: it isEmit's first argument, so accepting it let a caller replace the name after the grammar and failure-set checks had run.2 · The primitive switch was narrower than Go's scalars. A type switch matches the dynamic type, so
case int64never matchedtime.Duration. Now switches onreflect.Kind. AddedDuration(d) int64returning milliseconds — becausetime.Durationis an int64 kind and would otherwise pass as a raw nanosecond count, a number nobody reading a dashboard can interpret.3 · An empty
service.instance.idwas stamped.os.Hostname()returns""on error. Omitted now — the record layer already refused empty values; the resource layer did not.4 · Delivery ignored
Exports(). "Unknown never exports" lived in caller discipline at every #1907 call site. Gated now; validation still always runs, so a bad event fails in CI wherever the binary is built.5 · A bad key with an empty value passed silently.
normalisecontinued before validating the key. Validated first now — and a good key with an empty value is still dropped rather than rejected.Note
Finding 1 is the same defect Bugbot found independently in the Python sibling (backend#1996). Both fixed the same way, which is some evidence the layer split is the right shape rather than a local patch.
31 tests, 100% statement coverage,
make checkgreen. Six mutations, all caught.Refs #1897.
🤖 Generated with Claude Code
Note
Low Risk
Changes are confined to an unused internal telemetry package with expanded tests; no production callers yet, but the rules will shape future CLI instrumentation.
Overview
Hardens the CLI telemetry emitter before first call sites land (#1907): validation behavior stays strict, but several contract gaps from the merged helper are closed.
Resource vs record attributes — The single OTel allowlist is split into
resourceScope(process-only, set inNew) andrecordScope(call-site OTel keys). Call sites can no longer pass resource keys orevent.nameas attributes.Values and omission — Primitive checks use
reflect.Kindso named scalars (e.g.time.Duration) are accepted;absentValueuses the same kind-based rule so empty named strings are dropped and cannot satisfy failureerror.type.Duration()exposes milliseconds for duration attributes.service.instance.idis omitted when blank/whitespace.Emit delivery — The sink runs only when
Exports()is true; validation always runs. Attribute keys are validated before empty values are dropped, so retired/malformed keys cannot slip through on nil/empty values.Tests cover all five findings plus the named-string edge cases.
Reviewed by Cursor Bugbot for commit c3a9210. Bugbot is set up for automated code reviews on this repo. Configure here.