prometheus: fix the exposition so histograms, counters and types are valid - #232
Conversation
makeMetricBuckets allocated exactly len(buckets) entries and never appended an overflow bucket, so observations above the highest registered boundary were counted in _sum and _count but landed in no bucket at all. histogram_quantile() returns NaN unless the highest bucket has an upper bound of +Inf, so no histogram on this path could be evaluated. The rebuild check in metricState.update has to move with it. The stored bucket set is now one entry longer than the registry slice, so comparing against len(buckets) never matches: every observation would reallocate the bucket set and discard the counts, leaving _count climbing while every _bucket stayed at 0 or 1. That is worse than the defect being fixed, which is why both changes are in one commit. +Inf currently sorts ahead of every numeric boundary because label values compare as raw strings and '+' is ASCII 43 while digits start at 48. The golden tests record that ordering; a follow-up commit fixes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
label.less compared label values as raw strings, so "+Inf" sorted ahead
of every boundary ('+' is ASCII 43, digits start at 48) and "10" sorted
ahead of "2". byNameAndLabels.Less only delegates here, so histogram
buckets came out in the wrong order.
OpenMetrics requires buckets in increasing order; the text format is
indifferent, but the ordering is also what makes the exposition readable
and matches every other Prometheus client.
The comparison is shared by every label, so the numeric path is scoped
to "le" rather than applied wholesale, and falls back to string
comparison when a value does not parse.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
stats.Buckets is empty by default, and the lookup in HandleMeasures returned a nil slice with no error. collect() then ranged over it zero times and wrote no _bucket series, while _sum and _count were emitted unconditionally — so a histogram with no registered boundaries looked healthy and had no percentiles. DefaultBuckets holds the boundaries used by the reference Prometheus client. stats converts Duration values to seconds before bucketing, so timing histograms land on this range without configuration. This is a floor, not a replacement for choosing boundaries: a histogram whose values sit outside the range lands entirely in +Inf. What changes is that the failure is now visible in the exposition rather than absent from it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WriteStats deduplicated "# TYPE" lines on the bare field name with the scope discarded, so same-named fields arriving from different engine prefixes looked like repeats of each other and every one after the first was emitted untyped. Deriving sub-engines with WithPrefix is idiomatic across Segment services and exists precisely so subsystems can reuse short field names, so this fires readily: three sub-engines exposing hits and size shipped four of six metrics with no type. The dedup key becomes the scope and the root name together, and byNameAndLabels.Less orders by scope before name so that each family stays contiguous. Both halves are required. Sorting alone leaves the unscoped dedup suppressing types across a scope boundary. Deduping alone is worse than the defect: with the old ordering a histogram's _bucket series group by boundary across every scope while _count and _sum sort away from them, so one family declares its type thirteen times instead of once. Tests cover each half failing on its own. Less compares scope and name in turn rather than the joined string to avoid allocating per comparison in the sort. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
appendMetric wrote metric.time as an explicit timestamp on every sample. The field is optional in the exposition format, and a series that carries one opts out of Prometheus stale-marker handling: once the series stops being exported the scraper keeps serving its last value for five minutes rather than letting it go stale. An idle metric therefore looked live long after it stopped reporting. Dropping it lets the scraper assign scrape time, which is the behaviour every other exporter has. metric.time stays on the struct, where MetricTimeout and the store cleanup still depend on it. This changes staleness behaviour for anyone already scraping this package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Incr("requests") produced app_requests. Prometheus names an
accumulating count with a "total" suffix, and the convention is load
bearing: the OpenMetrics encoder keys the type line on the suffix, so a
counter without it is published as unknown rather than as a counter.
The suffix is applied in newMetricEntry alongside the cached _bucket,
_sum and _count names for histograms, so it covers every collection path
at once. The store key keeps the raw field name, so nothing about
lookup, state identity or cleanup changes — only what collect() emits.
A name already ending in _total is left alone, so a program that has
already adopted the convention does not produce requests_total_total.
This renames every counter on this path for anyone already scraping the
package.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Observe and Buckets.Set name the same metric differently. Observe takes a name relative to the engine and has the prefix attached after the name is split; Set attaches no prefix and merely splits what it is handed, so it needs the fully-qualified name. Registering buckets therefore means restating the engine prefix, and getting it wrong is an ordinary map miss: a mistyped key and no key at all produce identical output, so the histogram silently loses its buckets with no error anywhere. Deriving sub-engines with WithPrefix makes this worse, since buckets then have to be registered once per derived prefix, and services derive a dozen. SetBuckets moves key construction to the engine, which is the only thing that knows its own prefix. Callers pass the same string they pass to Observe, so the two cannot drift, and a sub-engine computes its own key. Additive: Buckets.Set is unchanged and keeps working. The test reads the expected key back out of what Observe actually emitted rather than restating the derivation, so it fails if either side changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HISTORY.md leads the v5.11.0 entry with the breaking change, since nothing fails to compile but every counter is renamed and staleness behaviour changes for anyone already scraping the package. The README gains the bucket registration the handler now needs, using Engine.SetBuckets. Snippet compile-checked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…second makeMetricBuckets appends a +Inf bucket unconditionally, without checking whether the last registered boundary already is one. Ending a bucket set with math.Inf(+1) is the idiom used by every registration in httpstats, netstats and procstats, and it produced two le="+Inf" series: identical labels, identical cumulative value, the second unreachable because metricBuckets.update stops at the first match. Prometheus drops the duplicate and counts it in prometheus_target_scrapes_sample_duplicate_timestamp_total. Those seven registrations are inert today for an unrelated reason — the registry key separator changed from ':' to '.' in b45dd38 and the keys no longer match what the handler looks up — so this fires only for callers whose keys do match. Fixing the key lookup arms it across all three packages at once, which is why the guard goes in first. The trim is at the lookup site rather than in makeMetricBuckets because metricState.update decides whether to rebuild by comparing the stored set against len(buckets)+1. A makeMetricBuckets that sometimes returned len(buckets) entries would never match, so every observation would reallocate the bucket set and discard the counts. buckets[:n-1] is a slice header over the same array, so nothing is allocated on the measure path. The registry miss check becomes len(buckets) == 0. HistogramBuckets.Set allocates with make, so registering an empty boundary list stored a non-nil zero-length slice that bypassed the DefaultBuckets fallback and left the histogram with a lone +Inf bucket. Each test was checked by reverting its fix individually. The accumulation test covers the rebuild-per-observation regression that the alternative placement would have introduced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WriteStats suppressed a repeated "# TYPE" by comparing each metric against its predecessor, which assumes every family is one unbroken run in the sorted output. Ordering by scope guarantees a scope is contiguous, but not a root name within it: the sort orders on the series name while a family is keyed on the root, and those are not the same ordering. A histogram "q" emits q_bucket, q_count and q_sum, so a sibling "q_bytes" in the same scope lands between the first two, overwrites the one-metric memory, and q_count declares the type a second time. A repeated declaration is not a dropped sample. The text format parser rejects the whole exposition — "second TYPE line for metric name" — so Prometheus discards the entire scrape: every series from that target is lost and up goes to 0. The preceding commit that falls back to DefaultBuckets widened this. An unregistered histogram used to emit no _bucket series at all, so its family began at _count and any sibling sorting ahead of that was harmless. With bucket series present the family begins at _bucket, and siblings in [_bucket, _count) now fall inside it — _bytes, _calls, _cache, _conn. Verified against v5.10.0: q_bytes and q_calls were previously fine and would have started failing, while q_depth, q_errors and q_size were already broken and are now fixed too. Tracking declared families in a set removes the dependency on contiguity rather than tightening it. metricKey already pairs scope and name and is already the store's key, so no new type is needed. The sort stays: it is still what keeps families grouped and the output readable, but readability is now all it is responsible for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
appendMetric no longer emits a timestamp, so the time copied onto every collected metric in metricState.collect has no reader on the output path. It was four dead stores per histogram series per scrape. The field stays on the metric struct because the input path needs it: HandleMeasures sets it from the measure time and metricStore.update carries it into state.time, which is what MetricTimeout expiry compares against in cleanup. A comment records that, so the assignments are not restored by someone reading collect() in isolation. TestMetricStoreCleanup asserted on the collected time. What that test covers is which entries survive expiry — the input times drive it, the collected copy was incidental — so the expectation drops the field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Handler's own doc comment still said the handler ignores histograms with no buckets set, which is what the DefaultBuckets fallback removed. It is the first thing a reader sees on the package page. HISTORY.md gains the two facts a consumer cannot derive from the entry as written. The bucket fallback is quantified: a histogram that published two series now publishes fourteen, per label set, and stats.Buckets is empty unless a program populates it, so it applies broadly. Counters and gauges are called out as unaffected, since the entry otherwise reads as a general cardinality change. The "# TYPE" entry covers the duplicate-declaration half as well as the missing-declaration half, because a repeated type line fails the whole scrape rather than mistyping one metric, and the _total entry names the version metrics the library renames about itself. Engine.SetBuckets leads with the constraint instead of closing on it. stats.Buckets is an unsynchronised map that handlers read on every histogram measure; the method's receiver makes it read like an instance call that is safe wherever the engine is in scope, and the one way to misuse it is a concurrent map write rather than a silent miss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review SummaryThe core exposition fixes hold up under trace and scratch tests: the Recommendation: COMMENT (The findings alone would recommend REQUEST_CHANGES. This repo has not opted into formal blocks, so the review posts as a comment.) 🔎 Suggested Areas for Human ReviewThese areas are the ones most likely to resist casual review — not an exhaustive list of everything worth checking. Review the rest of the diff as you normally would.
📋 PR Readiness Checklist
Reverify Drops
🟠 High Findings (3)DefaultBuckets fallback puts the library's own byte histograms on seconds buckets —
|
sreejitkar
left a comment
sreejitkar
left a comment
There was a problem hiding this comment.
Inline comments from review-toolkit agents.
| // _count but no _bucket series at all — nothing looked wrong, | ||
| // and no percentile could be computed. Fall back so that a | ||
| // histogram is never silently bucket-less. | ||
| if len(buckets) == 0 { |
There was a problem hiding this comment.
[high] DefaultBuckets fallback puts the library's own byte histograms on seconds buckets
[Found by 3 agents: bug-hunter, intent-reviewer, silent-failure-hunter] This fallback fires for every unregistered histogram, whatever its unit, including the httpstats and netstats byte histograms. Their registrations ("http.message:body.bytes", etc.) never match the {m.Name, f.Name} key looked up at line 71: the : separator is one reason, the engine prefix is the other. A scratch run with one 5000-byte response put every finite bucket at 0 and +Inf at 1. That adds about 12 near-empty series per histogram per label set, and histogram_quantile reports a plausible 10.
Suggestion: Fix the httpstats/netstats registration keys in this PR (build stats.Key directly), or make the fallback opt-in, or limit it to stats.Duration fields. The unconditional +Inf bucket already makes the exposition valid.
There was a problem hiding this comment.
Took the first option — fixed the keys — rather than gating the fallback.
There is a third reason the lookup misses, and it rules out the cheap version of that fix: HistogramBuckets.Set cannot express these keys at all. It splits on the last ., and every field involved carries one of its own (body.bytes, header.size, rtt.seconds), so no string yields {Measure: "http.message", Field: "body.bytes"}. The registrations aren't stale, they're unreachable — rewriting the strings was never available.
What landed:
HistogramBuckets.SetKey(Key, ...)— the two halves directly.HistogramBuckets.SetUnprefixed(Key, ...)+Lookup(measure, field)— a package registering frominit()runs before any engine exists and can't know the prefix its measures will carry, soLookupfalls back to dropping leading segments. Exact registrations always win, and onlySetUnprefixedregistrations are matched that way. I tried the simpler global walk first and dropped it: it letsvc.billinginherit a set registered for an unrelatedbilling, which is the same silent-wrong-boundaries defect this PR exists to remove, one level along.httpstatsandnetstatsre-registered throughSetUnprefixed.
One 5000-byte response:
before le="0.005" … le="10" all 0, le="+Inf" 1 histogram_quantile → 10
after le="100" 0 le="1000" 0 le="10000" 1 … histogram_quantile → interpolated in [1000, 10000)
The fallback stays unconditional, and on the last line of your suggestion — the +Inf bucket makes the exposition valid but not computable. promql.BucketQuantile returns NaN for a lone +Inf bucket (len(buckets) < 2), so opt-in or Duration-gating would have shipped NaN for every unregistered histogram. With the registrations resolving, the fallback now only reaches histograms nobody registered at all, which is the population DefBuckets serves in the reference client.
Left alone deliberately: otlp/handler.go:127 reads the registry with a direct map access and has the identical miss, but this PR states the other handlers are untouched, so it wants its own change. procstats registers "go.memstats:gc_pause.seconds" against fields declared type:"gauge" — inert whatever its key, so I didn't make it look fixed.
Your suggested test is in as TestHTTPStatsBucketsReachTheHandler, with one deviation: it imports httpstats for its init() registrations and feeds the handler the measure httpstats produces, rather than driving a live server — the server path reports asynchronously and needed polling to be reliable. It asserts le="10000" present and le="0.005" absent, and fails if either half of the fix is reverted independently.
| // A name that already ends in _total is left alone, so a program that | ||
| // has already adopted the convention does not end up with | ||
| // requests_total_total. | ||
| if !strings.HasSuffix(name, "_total") { |
There was a problem hiding this comment.
[high] _total suffixing double-suffixes dotted names and merges distinct fields
[Found by 2 agents: code-reviewer, bug-hunter] This check sees the raw field name, so e.Incr("requests.total") (field total) publishes app_requests_total_total, which is the case the comment above promises to leave alone. Because the store keys on the input name, a counter hits next to a field hits_total in the same scope renders two app_svc_hits_total samples with identical labels under one # TYPE line (reproduced on the PR head). One sample gets dropped silently.
Suggestion: Run the check on the name as it will be exposed (a bare total, or a sanitized name ending in _total, counts as suffixed), and detect or document the x/x_total collision. Add tests for both.
There was a problem hiding this comment.
Both halves addressed — the collision is fixed rather than documented.
Double-suffixing. The check now runs on the name as exposed, through a hasTotalSuffix helper that renders via appendMetricName. A field is joined to its scope by an _, so Incr("requests.total") arrives as the bare field total and already publishes app_requests_total; . renders as _ besides, so a dotted requests.total is pre-suffixed too. subtotal still gets the suffix — no _ boundary before total.
The collision. Resolved at collect time, where the store holds every entry under one lock and can see the whole scope:
// exposedName returns the name entry publishes under.
//
// It is entry.name except where the _total suffix added to a counter would
// land on a name some other field in the same scope already occupies ...
func (store *metricStore) exposedName(key metricKey, entry *metricEntry) string {
if entry.mtype != counter || entry.name == key.name {
return entry.name
}
if _, taken := store.entries[metricKey{scope: key.scope, name: entry.name}]; taken {
return key.name
}
return entry.name
}svc.hits beside svc.hits_total now publishes svc_hits and svc_hits_total — two distinct families, both samples kept. Dropping the suffix costs that counter a naming convention; keeping it cost a series.
Three things that made this the shape to pick:
- Resolution, not detection. The exposition format has no channel for a warning, and dropping one of the two samples is the behaviour being fixed. Making both survive is the only outcome that loses nothing.
- Order-independent. The result depends only on which names are present, not on which measure arrived first, so a process does not expose different names across restarts.
TestCounterTotalSuffixCollisionruns both arrival orders. - No mutation, no new locking.
entry.namestays as computed at creation;metricStore.collectresolves the exposed name and passes it down tometricState.collect, which only ever usedentry.namein the counter/gauge branch. Histogram names stay cached as they were.
TestMetricStore updated. You were right that it locked the collision in — its input has both A and A_total in scope test and it expected both as A_total. It now expects A and A_total, with a comment on why.
Tests, both confirmed to fail when the fix is reverted on its own:
TestCounterTotalSuffixUsesRenderedName— table overhits,hits_total,total,requests.total,subtotal. Reverting to the raw-name check fails ontotal(total_total) andrequests.total(requests.total_total).TestCounterTotalSuffixDottedName— end to end, assertsapp_requests_totaland nototal_total.TestCounterTotalSuffixCollision— both arrival orders, asserts both families present and exactly onesvc_hits_totalsample. RevertingexposedNamefails it, along withTestMetricStore.
There was a problem hiding this comment.
Correction to the above — I overstated the stability claim, and the gap was real.
I wrote that the result "depends only on which names are present, not on which measure arrived first." That held for arrival order but not for entry lifetime. metricStore.cleanup sweeps entries whose states have all expired past MetricTimeout, and exposedName was consulting the live entries, so once the colliding sibling stopped being reported the counter silently took its name back:
before cleanup after the sibling expires
svc_hits 1 svc_hits_total 1
svc_hits_total 5
A rename mid-process, which a scraper reads as one series going stale and another appearing — with a different value, since it is the counter's own.
Fixed by deciding from every name the store has ever held rather than the live set:
type metricStore struct {
mutex sync.RWMutex
entries map[metricKey]*metricEntry
// Every name the store has ever held, which cleanup deliberately does not
// prune ...
names map[metricKey]struct{}
}cleanup does not prune names. That is bounded by the program's metric vocabulary rather than by its data — label cardinality lives in metricEntry.states, not here — so it does not reintroduce a leak of the kind the 10K-op cleanup exists to prevent.
TestCounterTotalSuffixCollisionSurvivesCleanup covers it: report both, expire the sibling through cleanup, assert the counter still publishes svc_hits. It fails if exposedName goes back to reading store.entries.
Worth being explicit that ordinary counters are untouched by any of this — the suffix is dropped only where a sibling field in the same scope is literally named X_total:
svc.Incr("hits") → svc_hits_total 1
svc.Incr("requests") → svc_requests_total 1
app.Incr("hits") + app.Set("hits_total") → app_hits 1 and app_hits_total 5
| // e.SetBuckets("latency", 0.005, 0.01, 0.025, 0.05, 0.1) | ||
| // e.Observe("latency", d) | ||
| // | ||
| // A sub-engine derived with WithPrefix computes its own key, so buckets no |
There was a problem hiding this comment.
[high] Sub-engines still need their own SetBuckets call
[Found by 2 agents: bug-hunter, doc-grounding-reviewer] Buckets.Set(e.makeName(name), ...) keys on the full prefix of the engine it is called on. e.SetBuckets("latency", 0.5, 7) followed by e.WithPrefix("db").Observe("latency", 1.0) gives app_db_latency the 11 DefaultBuckets (scratch test on the PR head). README.md:220-222 makes the same claim.
Suggestion: "Call SetBuckets on the same engine or sub-engine that calls Observe. You pass the same short name, but each WithPrefix sub-engine needs its own call."
There was a problem hiding this comment.
Confirmed and fixed in the docs — but not with the suggested wording, because that replacement isn't right either.
You do not need a handle on each sub-engine. An ancestor can register for any depth by naming the path, because both sides do the same last-dot split and the same prefix concatenation:
root := stats.NewEngine("app", h)
root.SetBuckets("db.latency", 0.2, 0.4)
root.WithPrefix("db").Observe("latency", 0.3) // resolves
root.SetBuckets("sub.deeper.latency", 1.5, 3.5)
root.WithPrefix("sub").WithPrefix("deeper").Observe("latency", 2.0) // resolvesSo "each WithPrefix sub-engine needs its own call" would trade one wrong claim for another: it implies you must hold every derived engine, when one init function against the root covers the whole tree. What was actually false is the inheritance — that registering on app reaches app.db.
engine.go and README.md now say: the key is the engine's prefix joined to the name; name the metric relative to the engine you call it on; an ancestor can cover a tree by naming paths; and buckets are not inherited — a sub-engine resolves only what was registered for its own prefix.
Your parenthetical about Handler.Buckets is in both places too, and it is the sharper half of this finding: SetBuckets writes the global registry, a Handler with its own non-nil Buckets never reads it, so the call compiles, runs and does nothing. No prefix to get wrong — the API is simply inert, and you land on DefaultBuckets with no signal.
Behaviour left alone, deliberately. Making inheritance real means matching on the engine hierarchy, and the lookup only sees a string: NewEngine("app.db", h) and NewEngine("app", h).WithPrefix("db") are indistinguishable there. Resolving by name suffix instead would let svc.billing inherit a set registered for an unrelated billing — the same silent-wrong-boundaries defect this PR exists to remove. Real inheritance wants engines carrying their own buckets, which is a separate change; TestHistogramBucketsSuffixMatchingIsOptIn pins the weaker guarantee in the meantime.
SetBuckets also builds the stats.Key directly now rather than round-tripping through a string it re-parses, as the PR body said it should. New test: TestEngineSetBucketsFromAncestor, alongside the existing TestEngineSetBucketsKeyMatchesObserve cases.
|
Thanks — all three reproduced, and two ran deeper than described. Addressed as follows. Finding 1 —
|
…ollowups prometheus: land the review fixes #232 merged without
3 participants
Summary
The
prometheushandler accepts everything the stats API produces, and much of what it exposes is not valid Prometheus. Most seriously, no histogram it publishes can be evaluated byhistogram_quantile()— it never emits a+Infbucket, and by default it emits no_bucketseries at all. Counters are also published asunknownunder OpenMetrics because they lack the_totalsuffix the encoder keys on.This fixes the exposition.
Handlerkeeps its name, its fields and its place instats.MultiHandler; consumers get the fix on a version bump with no application changes.Other handlers (
datadog,influxdb,otlp,veneur) are untouched.Nothing fails to compile, but the published series change:
_totalsuffix — including the library's owngo_version_valueandstats_version_value+Inf,_sum,_count), per label set.stats.Bucketsis empty unless a program populates it, so this applies to every histogram without registered boundaries. Counters and gauges are unaffected — one series each, as beforeHISTORY.mdcarries the full entry.The defects
makeMetricBucketsallocated exactlylen(buckets)entries and never appended an overflow buckethistogram_quantile()returnsNaNunless the highest bucket is+Inf. Observations above the top boundary were counted in_sum/_countbut landed in no bucketstats.Bucketsis empty by default and a miss returned a nil slice with no errorcollectranged over it zero times and wrote no_bucketseries, while_sum/_countwere emitted unconditionally — so nothing looked wronglabel.lesscompared values as raw strings+Infsorted first (+is ASCII 43, digits start at 48) and10sorted ahead of2WriteStatsdeduplicated# TYPEon the bare field name, scope discardedWithPrefixexists precisely so subsystems can reuse short names likehits, so this fired readilyappendMetricwrote an explicit timestamp_totalsuffixunknownObserveandBuckets.Setname the same metric differentlyObservetakes a name relative to the engine;Setneeds the fully-qualified name. A mismatch is an ordinary map miss — a mistyped key and no key at all produce identical output, so the histogram silently loses its bucketsmakeMetricBucketsappended+Infwithout checking whether the registered set already ended with itle="+Inf"series with identical labels, the second unreachable. Ending a set withmath.Inf(+1)is the idiom used by every registration inhttpstats,netstatsandprocstatsbuckets == nilHistogramBuckets.Setallocates withmake, so an empty registration stored a non-nil zero-length slice that bypassed the fallback and left the histogram with a lone+InfbucketWriteStatssuppressed a repeated# TYPEby comparing each metric against its predecessorupgoes to 0Three things worth reviewer attention
Defect 10 is a regression this PR would otherwise have introduced. The
# TYPEdedup assumes every family is one unbroken run in the sorted output. Ordering by scope guarantees a scope is contiguous, but not a root name within it — the sort orders on the series name while a family is keyed on the root. A histogramqemitsq_bucket,q_count,q_sum, so a siblingq_byteslands between the first two andq_countdeclares the type again.Fixing defect 2 widened this. An unregistered histogram used to emit no
_bucketseries, so its family began at_countand siblings sorting ahead of that were harmless. With bucket series present the family begins at_bucket, and siblings in[_bucket, _count)now fall inside it. Measured against v5.10.0:qq_bytes,q_callsq_depth,q_errors,q_sizeTracking declared families in a set removes the dependency on sort contiguity rather than tightening it.
metricKeyalready pairs scope and name. The sort stays — it is still what keeps families grouped and the output readable, but readability is now all it is responsible for.The
+Inffix is not one line.metricState.updaterebuilds the bucket set whenlen(state.buckets) != len(buckets). Appending+Infmakes the stored slice permanently one longer than the registry slice, so without moving that check every observation reallocates and zeroes the counts —_countclimbing while every_bucketstays at 0 or 1. That is worse than the defect being fixed, so both changes are in one commit with a regression test. The same invariant is why defect 8 is fixed at the lookup site rather than insidemakeMetricBuckets.The
# TYPEscope fix is two changes that must land together. Dedup on scope + root name, and sort by scope before name. Sorting alone leaves the unscoped dedup suppressing types across a scope boundary. Deduping alone is worse than the defect: with the old ordering a histogram's_bucketseries group by boundary across every scope while_countand_sumsort away from them, so one family declares its type thirteen times instead of once. There is a test for each half failing on its own.New API
Engine.SetBuckets(name string, buckets ...any)derives the registry key from the engine's own prefix, so callers pass the same string they pass toObserveand the two cannot drift. AWithPrefixsub-engine computes its own key, removing the one-registration-per-derived-prefix problem. Additive —HistogramBuckets.Setis unchanged.It writes to the global
stats.Buckets, which is an unsynchronised map handlers read on every histogram measure, so it must be called frominitor program setup. The doc comment leads with that.prometheus.DefaultBucketsis the fallback for histograms with nothing registered — the reference client's boundaries, suited to latencies in seconds. It is a floor that keeps percentiles computable, not a substitute for choosing boundaries.Verification
go test ./...,go vet ./...,gofmt -l— cleango test -race ./prometheus/... .— cleanprometheus/common/expfmt, in a throwaway module sogo.modis untouched): counters parse asCOUNTER, both sub-engine families typed, histogram parses asHISTOGRAMwith+Inf==_count, buckets strictly increasing, no timestamps. Quantiles over 100 deterministic observations come out exact — p500.505, p950.9595, p990.9999, noNaN# TYPEconfirmed to be rejected by the reference parser (second TYPE line for metric name), and confirmed absent after the fix across the sibling matrix aboveNotes
Version metrics.
FieldType's zero value isCounter, andreportVersionOncebuilds bareField{}literals rather than callingMakeField, so the internal version metrics are counters and are renamed togo_version_value_total/stats_version_value_total. Consistent with the rule, though they are semantically info metrics. Left alone — changing their type is a separate decision.Not fixed here: the bucket registry key separator.
HistogramBuckets.Setsplits the key on the last., but the registrations inhttpstats,netstatsandprocstatsare written"http.message:header.size"— the:form thatsplitMeasureFieldused before b45dd38 ("fix typo insplitMeasureField()", Aug 2019) changed it. Splitting those strings on:reproduces the keys the handler looks up exactly; splitting on.does not, so all seven first-party registrations have been inert since. That is why defect 8 is dormant in this repo today — and why its guard lands here, before anything arms it. The key fix is a separate change:makeKeyneeds the:form whilemeasureOneneeds the.form, andEngine.SetBucketsshould build thestats.Keydirectly rather than round-tripping through a string it then re-parses.🤖 Generated with Claude Code