prometheus: land the review fixes #232 merged without

HISTORY.md

@@ -13,7 +13,19 @@ What changed, and why:
   `app_requests` and now publishes `app_requests_total`. This is not only
   Prometheus naming convention: the OpenMetrics encoder keys the type line on
   the suffix, so a counter without it was published as `unknown`. A name that
-  already ends in `_total` is left alone. **Queries and dashboards referring to
+  already ends in `_total` is left alone — as judged on the name once exposed,
+  not as received: a field is joined to its scope by an `_`, so
+  `Incr("requests.total")` reports the bare field `total` and already publishes
+  `app_requests_total`, and `.` renders as `_` besides. Where the suffix
+  would land on a name a sibling field already occupies — a counter `hits`
+  beside a field `hits_total` in the same scope — **the suffix is dropped
+  rather than the two being merged onto one series.** `svc.hits` publishes
+  `svc_hits` and `svc.hits_total` publishes `svc_hits_total`; merging them put
+  two samples with identical labels under one `# TYPE` line, of which a scraper
+  keeps one. Which names the handler has seen decides this — not the order they
+  arrived in, and not which are still live, so a counter does not change the
+  name it publishes under when its sibling stops being reported and is swept
+  by the `MetricTimeout` cleanup. **Queries and dashboards referring to
   the old names need updating.** This includes the metrics the library reports
   about itself: `go_version_value` and `stats_version_value` become
   `go_version_value_total` and `stats_version_value_total`.
@@ -22,8 +34,8 @@ What changed, and why:
   bucket per registered boundary and never appended an overflow bucket, so
   observations above the highest boundary were counted in `_sum` and `_count`
   but landed in no bucket at all. `histogram_quantile()` returns `NaN` unless
-  the highest bucket is `+Inf`, so no histogram published by this handler could
-  be evaluated.
+  the highest bucket is `+Inf`, so any histogram whose registered boundaries
+  did not already end in `math.Inf(+1)` could not be evaluated.
 
 - **Histograms with no registered boundaries fall back to `prometheus.DefaultBuckets`.**
   `stats.Buckets` is empty by default and a miss returned a nil slice with no
@@ -33,8 +45,9 @@ What changed, and why:
   for choosing boundaries. **This adds bucket series for histograms that
   previously published none:** a histogram that published 2 series (`_sum`,
   `_count`) now publishes 14 (11 boundaries, `+Inf`, `_sum` and `_count`), per
-  label set. `stats.Buckets` is empty unless a program populates it, so this
-  applies to every histogram without registered boundaries. Counters and gauges
+  label set. This applies to every histogram the lookup finds nothing for; the
+  registrations shipped by `httpstats` and `netstats` now resolve (below), so
+  those get their own boundaries rather than these. Counters and gauges
   are unaffected — they publish one series each, as before.
 
 - **Bucket `le` labels sort numerically.** They compared as raw strings, which
@@ -57,13 +70,48 @@ What changed, and why:
   serving the last value for five minutes after a series stopped being
   exported. The scraper now assigns scrape time. `MetricTimeout` is unaffected.
 
+- **Bucket registrations made by `httpstats` and `netstats` now resolve.** They
+  never had. `HistogramBuckets.Set` splits its argument 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. The strings were never updated, so each has
+  been keyed on a name nothing looks up since. The string form cannot express
+  these keys at all: `Set` cuts at the last `.`, and every one of these fields
+  carries one of its own (`body.bytes`, `header.size`, `rtt.seconds`). A second
+  mismatch sat behind the first — the lookup uses the measure name after the
+  engine prefix is attached, and a package registering from `init()` cannot
+  know that prefix. **This changes the series published for anyone using
+  `httpstats` or `netstats`:** those histograms published no `_bucket` series
+  before, and would otherwise have taken the seconds-scale `DefaultBuckets`
+  above — eleven boundaries no byte count can reach. They now publish the byte
+  and duration boundaries those packages declare. `procstats` registers
+  `"go.memstats:gc_pause.seconds"` for fields declared `type:"gauge"`, so that
+  entry remains inert and is left alone.
+
+- **New: `HistogramBuckets.SetKey`, `SetUnprefixed` and `Lookup`.** `SetKey`
+  takes the `Measure` and `Field` halves directly, reaching keys `Set` cannot
+  express. `SetUnprefixed` registers a measure named without the prefix an
+  engine will add to it, for packages registering from `init()`, and `Lookup`
+  resolves those by dropping leading segments from the measure name 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. `Set` is unchanged, and the `otlp` handler — which reads
+  the registry directly — is untouched.
+
 - **New: `Engine.SetBuckets(name, buckets...)`.** `Observe` takes a name
   relative to the engine, while `HistogramBuckets.Set` needs the
   fully-qualified name, so registering buckets meant restating the engine
   prefix — and a mismatch was an ordinary map miss, indistinguishable from no
   registration at all. `SetBuckets` derives the key from the engine's own
-  prefix, so callers pass the same string they pass to `Observe` and a
-  `WithPrefix` sub-engine computes its own key. `Buckets.Set` is unchanged.
+  prefix, so callers pass the same string they pass to `Observe`. An ancestor
+  can register for a sub-engine by naming the path to it —
+  `root.SetBuckets("db.latency", ...)` covers
+  `root.WithPrefix("db").Observe("latency", ...)` — so one `init` function
+  covers a whole tree. Buckets are not inherited: a sub-engine resolves only
+  what was registered for its own prefix. A `Handler` holding its own non-nil
+  `Buckets` never reads the global registry, so `SetBuckets` has no effect on
+  it. `Buckets.Set` is unchanged.
 
 **The minimum supported Go version is now 1.26.** The `golang.org/x/*` modules
 (`net`, `sys`, `sync`, `text`) all declare `go 1.26.0` as of their latest

README.md

@@ -218,11 +218,22 @@ engine.SetBuckets("request.latency", 0.005, 0.01, 0.025, 0.05, 0.1, 0.5, 1)
 engine.Observe("request.latency", elapsed)
 ```
 
-A sub-engine derived with `WithPrefix` computes its own key, so buckets do not
-have to be registered once per derived prefix. Histograms with nothing
-registered fall back to `prometheus.DefaultBuckets`, which suits latencies
-measured in seconds — a floor that keeps percentiles computable, not a
-substitute for picking boundaries where accuracy matters.
+The key is the engine's prefix joined to the name, so name the metric relative
+to the engine you call it on. An ancestor can register for a sub-engine by
+naming the path to it, which lets one `init` function cover a whole tree:
+
+```go
+engine.SetBuckets("db.request.latency", 0.01, 0.05, 0.25)
+engine.WithPrefix("db").Observe("request.latency", elapsed)
+```
+
+Buckets are not inherited — a sub-engine resolves only what was registered for
+its own prefix — and a `Handler` holding its own non-nil `Buckets` never reads
+the global registry, so `SetBuckets` has no effect on it.
+
+Histograms with nothing registered fall back to `prometheus.DefaultBuckets`,
+which suits latencies measured in seconds — a floor that keeps percentiles
+computable, not a substitute for picking boundaries where accuracy matters.
 
 ### InfluxDB
 

buckets.go

@@ -22,6 +22,68 @@ func (b HistogramBuckets) Set(key string, buckets ...any) {
 	b[makeKey(key)] = v
 }
 
+// SetKey registers buckets under an exact key.
+//
+// Set cannot express every key. It splits its argument on the last ".", so a
+// field whose own name contains a "." is unreachable through it — and measures
+// reported from struct tags routinely have such fields, "body.bytes" among
+// them. SetKey takes the two halves directly.
+func (b HistogramBuckets) SetKey(key Key, buckets ...any) {
+	v := make([]Value, len(buckets))
+
+	for i, x := range buckets {
+		v[i] = MustValueOf(ValueOf(x))
+	}
+
+	b[key] = v
+}
+
+// SetUnprefixed registers buckets for a measure named without the prefix an
+// engine will add to it.
+//
+// A package that instruments something — httpstats, netstats — registers its
+// buckets from init(), before any engine exists, so it cannot know the prefix
+// the measures it describes will end up carrying. Lookup resolves these by
+// dropping leading segments from the measure name, so a set registered for
+// "http.message" applies to "myapp.http.message" as well.
+//
+// Prefer SetKey wherever the full measure name is known. Matching by suffix
+// cannot tell a derived name from an unrelated one that happens to end the
+// same way, which is why it is opted into here rather than applied to every
+// registration.
+func (b HistogramBuckets) SetUnprefixed(key Key, buckets ...any) {
+	b.SetKey(Key{Measure: anyPrefix + key.Measure, Field: key.Field}, buckets...)
+}
+
+// Lookup returns the buckets registered for a measure and field, or nil.
+//
+// An exact registration always wins. Failing that, registrations made with
+// SetUnprefixed are tried against progressively shorter suffixes of the
+// measure name, longest first.
+func (b HistogramBuckets) Lookup(measure, field string) []Value {
+	if v := b[Key{Measure: measure, Field: field}]; len(v) != 0 {
+		return v
+	}
+
+	for {
+		if v := b[Key{Measure: anyPrefix + measure, Field: field}]; len(v) != 0 {
+			return v
+		}
+
+		i := strings.IndexByte(measure, '.')
+		if i < 0 {
+			return nil
+		}
+
+		measure = measure[i+1:]
+	}
+}
+
+// anyPrefix marks a registration made by SetUnprefixed. It is not a valid
+// measure name, so it cannot collide with one registered through Set or
+// SetKey.
+const anyPrefix = "*."
+
 // Buckets is a registry where histogram buckets are placed. Some metric
 // collection backends need to have histogram buckets defined by the program
 // (like Prometheus), a common pattern is to use the init function of a package

buckets_test.go

@@ -3,8 +3,10 @@ package stats_test
 import (
 	"strings"
 	"testing"
+	"time"
 
 	stats "github.com/segmentio/stats/v5"
+	_ "github.com/segmentio/stats/v5/httpstats"
 	"github.com/segmentio/stats/v5/prometheus"
 	"github.com/segmentio/stats/v5/statstest"
 )
@@ -148,3 +150,123 @@ func keysOf(b stats.HistogramBuckets) []stats.Key {
 	}
 	return keys
 }
+
+// TestHistogramBucketsSetKeyDottedField covers the key Set cannot express.
+// Set splits its argument on the last ".", so a field carrying one of its own
+// is unreachable through it — and every histogram httpstats reports has such
+// a field.
+func TestHistogramBucketsSetKeyDottedField(t *testing.T) {
+	key := stats.Key{Measure: "http.message", Field: "body.bytes"}
+
+	b := stats.HistogramBuckets{}
+	b.SetKey(key, 100, 1000)
+
+	if _, ok := b[key]; !ok {
+		t.Fatalf("SetKey did not register %#v; registry holds %#v", key, keysOf(b))
+	}
+
+	legacy := stats.HistogramBuckets{}
+	legacy.Set("http.message.body.bytes", 100, 1000)
+
+	if _, ok := legacy[key]; ok {
+		t.Error("Set reached the dotted-field key; SetKey would be unnecessary")
+	}
+}
+
+// TestHistogramBucketsLookupUnprefixed pins the resolution a package doing its
+// registration from init() depends on: it cannot know the prefix an engine
+// will add, so the name it registers has to match a measure carrying one.
+func TestHistogramBucketsLookupUnprefixed(t *testing.T) {
+	b := stats.HistogramBuckets{}
+	b.SetUnprefixed(stats.Key{Measure: "http.message", Field: "body.bytes"}, 100, 1000)
+
+	for _, measure := range []string{
+		"http.message",
+		"myapp.http.message",
+		"myapp.sub.http.message",
+	} {
+		if v := b.Lookup(measure, "body.bytes"); len(v) != 2 {
+			t.Errorf("Lookup(%q) resolved %d buckets, expected 2", measure, len(v))
+		}
+	}
+
+	if v := b.Lookup("myapp.http.message", "header.bytes"); v != nil {
+		t.Error("Lookup matched a different field")
+	}
+	if v := b.Lookup("message", "body.bytes"); v != nil {
+		t.Error("Lookup matched half a segment")
+	}
+}
+
+// TestHistogramBucketsLookupExactWins keeps a program able to override what a
+// package registered for the same measure.
+func TestHistogramBucketsLookupExactWins(t *testing.T) {
+	b := stats.HistogramBuckets{}
+	b.SetUnprefixed(stats.Key{Measure: "conn.read", Field: "bytes"}, 1, 2, 3)
+	b.SetKey(stats.Key{Measure: "myapp.conn.read", Field: "bytes"}, 9)
+
+	if v := b.Lookup("myapp.conn.read", "bytes"); len(v) != 1 {
+		t.Errorf("exact registration lost to the unprefixed one (%d buckets)", len(v))
+	}
+}
+
+// TestHistogramBucketsSuffixMatchingIsOptIn is why SetUnprefixed exists as a
+// separate call. Matching by suffix cannot tell a derived measure from an
+// unrelated one ending the same way, so only registrations that ask for it
+// take part: svc.billing must not inherit a set registered for billing.
+func TestHistogramBucketsSuffixMatchingIsOptIn(t *testing.T) {
+	b := stats.HistogramBuckets{}
+	b.SetKey(stats.Key{Measure: "billing", Field: "size"}, 0.25, 0.75)
+
+	if v := b.Lookup("svc.billing", "size"); v != nil {
+		t.Errorf("a SetKey registration was matched by suffix: %v", v)
+	}
+}
+
+// TestEngineSetBucketsFromAncestor pins what the doc comment promises: one
+// init function on the root engine covers the whole tree, by naming the path
+// to each metric rather than holding a reference to every sub-engine.
+func TestEngineSetBucketsFromAncestor(t *testing.T) {
+	ph := &prometheus.Handler{}
+	root := stats.NewEngine("anc", ph)
+
+	root.SetBuckets("db.latency", 0.2, 0.4)
+	defer delete(stats.Buckets, stats.Key{Measure: "anc.db", Field: "latency"})
+
+	root.WithPrefix("db").Observe("latency", 0.3)
+
+	var buf strings.Builder
+	ph.WriteStats(&buf)
+	out := buf.String()
+
+	// Two registered boundaries plus +Inf. Eleven would mean the registration
+	// missed and DefaultBuckets was used.
+	if n := strings.Count(out, "anc_db_latency_bucket{"); n != 3 {
+		t.Errorf("found %d bucket series, expected 3:\n%s", n, out)
+	}
+}
+
+// TestHTTPStatsBucketsReachTheHandler is the regression this whole change is
+// for. httpstats registers byte boundaries for its message histograms; until
+// they resolved, every one of them took DefaultBuckets instead — eleven
+// boundaries between 0.005 and 10 seconds, none of which a byte count can
+// reach, so +Inf held every observation.
+func TestHTTPStatsBucketsReachTheHandler(t *testing.T) {
+	ph := &prometheus.Handler{}
+
+	ph.HandleMeasures(time.Now(), stats.Measure{
+		Name:   "myapp.http.message",
+		Fields: []stats.Field{stats.MakeField("body.bytes", 5000, stats.Histogram)},
+	})
+
+	var buf strings.Builder
+	ph.WriteStats(&buf)
+	out := buf.String()
+
+	if !strings.Contains(out, `le="10000"`) {
+		t.Errorf("registered byte boundaries missing:\n%s", out)
+	}
+	if strings.Contains(out, `le="0.005"`) {
+		t.Errorf("fell back to DefaultBuckets:\n%s", out)
+	}
+}

engine.go

@@ -131,7 +131,7 @@ func (e *Engine) ObserveAt(t time.Time, name string, value any, tags ...Tag) {
 }
 
 // SetBuckets registers histogram buckets for the metric that Observe(name)
-// reports, deriving the registry key from the engine's own prefix.
+// reports on this engine.
 //
 // It must be called before the metrics it covers start being reported — from
 // an init function or program setup. Despite the receiver it writes to the
@@ -148,18 +148,30 @@ func (e *Engine) ObserveAt(t time.Time, name string, value any, tags ...Tag) {
 // identical output, so the histogram silently loses its buckets.
 //
 // SetBuckets removes that by moving key construction to the engine, which does
-// know its prefix. Callers pass the same string they pass to Observe:
+// know its prefix. The key is the engine's prefix joined to name, so name the
+// metric relative to the engine this is called on — the same string passed to
+// Observe:
 //
-//	e := stats.NewEngine("app", h)
-//	e.SetBuckets("latency", 0.005, 0.01, 0.025, 0.05, 0.1)
-//	e.Observe("latency", d)
+//	root := stats.NewEngine("app", h)
+//	root.SetBuckets("latency", 0.005, 0.01, 0.025, 0.05, 0.1)
+//	root.Observe("latency", d)
 //
-// A sub-engine derived with WithPrefix computes its own key, so buckets no
-// longer have to be registered once per derived prefix.
+// A sub-engine's metrics can be registered from any ancestor by naming the
+// path to them, so one init function can cover a whole tree:
+//
+//	root.SetBuckets("db.latency", 0.01, 0.05, 0.25)
+//	root.WithPrefix("db").Observe("latency", d)
+//
+// Buckets are not inherited. A sub-engine resolves only what was registered
+// for its own prefix, never what an ancestor registered for itself.
+//
+// A Handler holding its own non-nil Buckets never reads the global registry,
+// so SetBuckets has no effect on it; populate that map instead.
 //
 // The existing Buckets.Set keeps working unchanged.
 func (e *Engine) SetBuckets(name string, buckets ...any) {
-	Buckets.Set(e.makeName(name), buckets...)
+	measure, field := splitMeasureField(name)
+	Buckets.SetKey(Key{Measure: e.makeName(measure), Field: field}, buckets...)
 }
 
 // Clock returns a new clock identified by name and tags.