Exact metric values, stricter fail-fast, and an opt-in client derivation

[aaylward]

What

Eleven findings from a critical post-merge review of #199, in two commits. Four are real correctness defects in the application-metrics path, and all four produce the failure mode that code's own comments name as the worst case: a scrape Prometheus rejects in full, with no in-process consumer to notice.

Every rendered value is exact now

FormatNumber had two related holes:

• It built a string from snprintf's would-have-written length, unclamped against its 64-byte buffer — an out-of-bounds read on every scrape for any value whose %.6f form is longer than that. gauge.Set(1e60) reaches it; so does a bucket bound of 1e60, which the ladder validation happily admits.
• %.6f is lossy below its granularity. NewHistogram("s","S",{1e-8, 1e-7}) passes the strictly-ascending check while both bounds render "0" — two identical le labels, a duplicate series, the whole scrape refused. Same trap one decimal further in: 1.1e-6 and 1.2e-6 both rendered "0.000001".

One rule fixes both: the fixed six-decimal spelling is kept only when it parses back to exactly the value it claims, and everything else gets a shortest round-trip form (%.15g/%.16g/%.17g, verified by parse-back). The stability half matters as much as the exactness half — rendered strings are series identity, so le="2500000" must never become le="2.5e+06". A test pins the historical spellings (0.005, 2500000, -0.25) untouched; the ASan job is what makes the out-of-bounds test a proof rather than a hope.

Misuse aborts instead of corrupting the scrape

Three gaps where the file's own fail-fast posture (ADR-0009) didn't reach:

• Duplicate label names rendered {a="1",a="2"} — a parse error that fails the entire scrape — while the invalid-name check beside them aborted.
• A user label named le on a histogram doubled the label the bucket lines append themselves. Reserved only where it collides: on a counter or gauge, le is legal Prometheus and a test pins that the reservation stays scoped.
• Re-registering a histogram compared kind and help but not buckets, beneath a comment promising "a mismatch aborts rather than picking one". A second caller's observations landed silently in the first caller's bins; same-ladder re-registration stays idempotent.

And one delegated design call: Counter::Increment now aborts on a negative amount. The decrease fails nowhere visible — rate() reads it as a counter reset and extrapolates from zero, inflating exactly the panel someone is staring at. prometheus/client_golang panics here for the same reason. The check runs before the disabled-registry early-return, so enabling metrics in production is never the first time it fires.

The client derivation is opt-in (the other delegated call)

Observe derived the ADR-0012 client on every request — header walk, address parse, string building — even in chains whose sinks never read it, which is every metrics-only composition. The trust boundary is now std::optional, defaulting to skip: client stays value-initialized at Source::kUnknown and the request pays nothing. Any supplied boundary derives, including TrustedProxies::None(), which keeps its meaning as the explicit direct-connect statement rather than doubling as "I didn't think about it". Both of the access-log formatter's call sites (#206) already pass a boundary explicitly, so nothing out of tree changes behavior.

Cleanup from the same review

• The scrape-does-not-count-itself transport assertion searched for operation=""/status= spellings that cannot appear in this exposition at all, so it passed under any composition. It now asserts route="/metrics" absence, which the endpoint's self-label makes falsifiable.
• The guide's application-metrics example called NewHistogram without the buckets argument the API deliberately requires — the documented snippet didn't compile, and named its metric _seconds in a microseconds dialect.
• The CHANGELOG introduced the endpoint as "three families" and enumerated five in the same sentence; max_series was documented against the removed {method,route,status} keying; BuiltInLabels promised an empty-name drop its rewrite no longer performs.

Testing

Fifteen new tests: four value-formatting (huge exact, tiny buckets distinct, past-six-decimals distinct, historical spellings byte-stable), six fail-fast death tests plus the two scoping/idempotence positives, the derivation-skip pin (peer and header present, client stays kUnknown), and the two updated tests now stating their boundary explicitly.

Locally: make verify (129 tests under --config=werror, lockfiles, codegen, goldens, clang-format), make noexcept, gcc ASan and TSan on the touched targets, clang-tidy clean on the touched files, buildifier via npx, and the 15-target consumer suite. The only verify failure is the sandbox's missing buildifier binary, verified via npx @bazel/buildifier@8.2.1 instead.

Checklist

• Tests added/updated for the change
• bazel test //... and (cd codegen && gradle build spotlessCheck) pass locally
• Formatting clean (clang-format, buildifier via npx, spotless)
• Architectural decisions recorded as an ADR (not applicable — hardening and a defaulted-parameter change inside the existing design)

🤖 Generated with Claude Code

https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU

---

Generated by Claude Code