prometheus: land the review fixes #232 merged without - #233
Merged
Merged
Conversation
The buckets httpstats and netstats register have never resolved. HistogramBuckets.Set splits on the last ".", but these registrations are written "http.message:body.bytes" — the ":" form splitMeasureField used before b45dd38 ("fix typo in splitMeasureField()", Aug 2019) changed the separator, and the strings were never updated. The string form cannot express these keys at all: every field involved carries a "." of its own (body.bytes, header.size, rtt.seconds), so no argument to Set yields {Measure: "http.message", Field: "body.bytes"}. A second mismatch sat behind the first — the lookup uses the measure name with the engine prefix attached, and a package registering from init() cannot know that prefix. Left alone, these histograms take the seconds-scale DefaultBuckets the handler now falls back to: eleven boundaries no byte count can reach. SetKey takes the Measure and Field halves directly. SetUnprefixed registers a measure named without the prefix an engine will add, and Lookup resolves those by dropping leading segments after an exact lookup fails. Exact registrations always win, and only registrations made through SetUnprefixed are matched that way — matching every registration by suffix cannot tell a derived measure from an unrelated one ending the same way. Engine.SetBuckets keyed on Buckets.Set and inherited its split, so it could not reach a dotted field either; it now splits the name itself and registers through SetKey. An ancestor can register for a sub-engine by naming the path to it, so one init function covers a whole tree. procstats registers "go.memstats:gc_pause.seconds" for fields declared type:"gauge", so that entry stays inert and is left as is. Addresses #232 (comment) and #232 (comment) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The check ran on the field name as received, which misses the case its
own comment promises to handle. A field is joined to its scope by an
"_", so Incr("requests.total") arrives as the bare field "total" and
already publishes <scope>_requests_total; any byte invalid in a metric
name, "." among them, also renders as "_". Both went unnoticed and
yielded _total_total. hasTotalSuffix runs the test on the rendered name.
Suffixing can also land a counter on a name a sibling field already
occupies — a counter "hits" beside a field "hits_total" in the same
scope. That put two samples with identical labels under one # TYPE
line, of which a scraper keeps one. The suffix is now dropped rather
than the two being merged: losing a naming convention beats losing a
series.
exposedName decides that from the set of names the store has held,
which cleanup deliberately does not prune. Consulting the live entries
instead would let a counter rename itself once its colliding sibling
expired past MetricTimeout, which a scraper reads as one series going
stale and another appearing. Metric names come from the program, not
from the data, so the set is bounded by the program's vocabulary —
label cardinality lives in metricEntry.states, not here.
Addresses #232 (comment)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The v5.11.0 notes describe the exposition as it stands after these fixes, since the release has not been cut yet: the httpstats and netstats registrations that now resolve, the new HistogramBuckets methods, the _total suffix decided on the exposed name and the sibling collision it can hit. Two earlier claims are corrected — histograms whose registered boundaries already ended in math.Inf(+1) were always evaluable, and Engine.SetBuckets does not let a sub-engine inherit an ancestor's buckets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
yatharth-1504
approved these changes
Sep 23, 2026
Merged
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
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.
Follow-up to #232, which was merged without its review fixes.
The three [high] findings on that PR were each answered in their review thread, but the code implementing those answers was never committed — the branch's last commit (
83be4c5) predates the review, and the merge took the pre-review tree. Somaincurrently carries the discussion without the change. This PR carries the change.Nothing here is new scope: it is exactly what those threads say was done.
What #232 merged without
mainDefaultBucketsfallback puts the library's own byte histograms on seconds bucketshttpstats/metrics.go:17andnetstats/conn.go:17still register"http.message:body.bytes"— keys nothing looks up sinceb45dd38(Aug 2019) changed the separator. Those byte histograms fall through to the seconds-scaleprometheus.DefaultBuckets: eleven boundaries no byte count can reach.852585a_totalsuffixing double-suffixes dotted names and merges distinct fieldsprometheus/metric.go:166still tests the received field name, soIncr("requests.total")publishesapp_requests_total_total, and a counterhitsbeside a fieldhits_totalputs two samples with identical labels under one# TYPEline.496391fSetBucketscallengine.goandREADME.md:221claim aWithPrefixchild resolves an ancestor's registration. It does not.852585aThe self-correction on the second thread — that
exposedNameconsulted the live entries, so a counter renamed itself once its colliding sibling expired pastMetricTimeout— is included in496391f.Changes
852585a—stats: make histogram bucket keys expressible and resolvableHistogramBuckets.Setsplits on the last., so a field carrying a.of its own (body.bytes,header.size,rtt.seconds) is unreachable through it — no string argument yields{Measure: "http.message", Field: "body.bytes"}. A second mismatch sat behind the first: the lookup uses the measure name after the engine prefix is attached, and a package registering frominit()cannot know that prefix.SetKey(Key, ...)takes theMeasureandFieldhalves directly.SetUnprefixed(Key, ...)registers a measure named without the prefix an engine will add to it.Lookup(measure, field)resolves those by dropping leading segments after an exact lookup fails. Exact registrations always win, and onlySetUnprefixedregistrations match that way — suffix-matching everything cannot tell a derived measure from an unrelated one ending the same way.Engine.SetBucketsnow splits the name itself and registers throughSetKey, so an ancestor can register for a sub-engine by naming the path to it.Setis unchanged, and theotlphandler, which reads the registry map directly, is untouched.procstatsregisters"go.memstats:gc_pause.seconds"for fields declaredtype:"gauge", so that entry stays inert and is left alone.496391f—prometheus: decide the_totalsuffix on the name as exposedhasTotalSuffixruns the test on the rendered name: a field is joined to its scope by an_, and.renders as_. Where the suffix would collide with a sibling field, it is dropped rather than the two being merged — losing a naming convention beats losing a series.exposedNamedecides that from every name the store has held, whichcleanupdeliberately does not prune, so the choice cannot flip mid-process. That set is bounded by the program's metric vocabulary; label cardinality lives inmetricEntry.states, not here.8e9adf7—docs: record the follow-up fixes in the v5.11.0 entryv5.11.0 is not tagged yet, so the notes are amended in place rather than opening a v5.12.0 section. Two claims from #232 are corrected: histograms whose registered boundaries already ended in
math.Inf(+1)were always evaluable, andSetBucketsdoes not let a sub-engine inherit an ancestor's buckets.Testing
go build ./...,go vet ./...,go test ./...— all packages pass, includingotlp../,./prometheus/...,./httpstats/...,./netstats/...), so the branch stays bisectable.gofmt -l .clean.TestHistogramBucketsSetKeyDottedField,TestHistogramBucketsLookupUnprefixed,TestHistogramBucketsLookupExactWins,TestHistogramBucketsSuffixMatchingIsOptIn,TestEngineSetBucketsFromAncestor,TestHTTPStatsBucketsReachTheHandler,TestCounterTotalSuffixUsesRenderedName,TestCounterTotalSuffixDottedName,TestCounterTotalSuffixCollision,TestCounterTotalSuffixCollisionSurvivesCleanup.Release
v5.11.0 is unreleased, so this lands before the tag and no released API changes. Once merged: date the
HISTORY.mdheading, thenmake release(bump_version --tag-prefix=v minor version/version.go) and push with--follow-tags.🤖 Generated with Claude Code