release-train: develop -> staging - #508
Merged
Merged
Conversation
…#1897) (#507) * fix(telemetry): five review findings on the merged Go helper (backend#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> * fix(telemetry): the omit rule must know what a string is, same as the 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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
bugbot run |
tracebloc-release-train
Bot
added
the
gate-nudge
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 e7c7185. Configure here.
tracebloc-release-train
Bot
removed
the
gate-nudge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated promotion by the release train (RFC-0008 D14). Head is the train-managed
release-train/to-stagingbranch (a mirror ofdevelop), so it never collides with a human PR. Merged only when the fr-gate is green.Note
Medium Risk
Changes how events are validated and whether they are exported; callers that relied on old permissive behavior (resource keys on records, delivery in unknown envs, empty named strings) will see errors or silent non-delivery until fixed.
Overview
Tightens CLI telemetry contract enforcement so invalid or cross-layer attributes cannot slip through, and export behavior no longer depends on every call site remembering
Exports().Attribute keys are split into resource vs record scopes: call sites cannot set
service.*,tracebloc.component,event.name, etc. Emptyservice.instance.idis omitted atNew()instead of being sent blank.Emitonly delivers whenExports()is true, even if a sink is installed.Normalization now validates keys before dropping empty values, so retired or malformed keys cannot pass with
nil/empty payloads. Absence detection uses reflect string kinds (including named strings liketype Reason string), while 0 andfalseare kept as real measurements. Primitive checks accept all Go scalar kinds (e.g.time.Duration);Duration()converts durations to milliseconds for dashboard-friendly attributes.Tests cover the above (resource-scope rejection, export gating, named-string empty
error.type, and related cases).Reviewed by Cursor Bugbot for commit e7c7185. Bugbot is set up for automated code reviews on this repo. Configure here.