prometheus: fix the exposition so histograms, counters and types are valid

HISTORY.md

@@ -2,6 +2,69 @@
 
 ### v5.11.0 (Unreleased)
 
+**The `prometheus` handler's exposition has changed. Nothing fails to compile,
+but the series it publishes are different, so anyone already scraping this
+package will see renamed metrics and different staleness behaviour on upgrade.**
+The `datadog`, `influxdb`, `otlp` and `veneur` handlers are untouched.
+
+What changed, and why:
+
+- **Counters are now suffixed with `_total`.** `Incr("requests")` published
+  `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
+  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`.
+
+- **Histograms always emit a `+Inf` bucket.** The handler allocated exactly one
+  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.
+
+- **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
+  error, so such a histogram published `_sum` and `_count` with no `_bucket`
+  series and nothing looked wrong. The defaults are the reference Prometheus
+  client's, suited to latencies in seconds; they are a floor, not a substitute
+  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
+  are unaffected — they publish one series each, as before.
+
+- **Bucket `le` labels sort numerically.** They compared as raw strings, which
+  put `+Inf` first and `10` ahead of `2`.
+
+- **`# TYPE` is declared once per scope, not once per field name.** The dedup
+  discarded the scope, so same-named fields coming from different engine
+  prefixes looked like repeats and every one after the first was published
+  untyped. Sub-engines derived with `WithPrefix` exist precisely so subsystems
+  can reuse short field names, so this fired readily.
+  The handler now tracks every family it has declared rather than comparing
+  each metric against the previous one, so a family interrupted in the sorted
+  output is no longer declared twice. A repeated `# TYPE` makes the text format
+  parser reject the entire exposition, so this also fixes scrapes that failed
+  outright when a histogram shared a scope with a field sorting between its
+  `_bucket` and `_sum` series — `latency` next to `latency_size`, say.
+
+- **Timestamps are no longer exposed.** The field is optional, and a series
+  carrying one opts out of Prometheus stale-marker handling — the scraper kept
+  serving the last value for five minutes after a series stopped being
+  exported. The scraper now assigns scrape time. `MetricTimeout` is unaffected.
+
+- **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.
+
 **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
 releases, and `stats` depends on them both directly and transitively through

README.md

@@ -205,6 +205,25 @@ http.Handle("/metrics", prometheus.DefaultHandler)
 instance. Construct your own `&prometheus.Handler{}` literal instead if you need to set
 `TrimPrefix`, `MetricTimeout`, or `Buckets`.
 
+Counters are published with a `_total` suffix, and the handler leaves timestamps
+off so the scraper assigns scrape time.
+
+Histograms need bucket boundaries. Register them with `Engine.SetBuckets`, which
+takes the same name you pass to `Observe` and derives the registry key from the
+engine's own prefix:
+
+```go
+engine := stats.NewEngine("app", prometheus.DefaultHandler)
+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.
+
 ### InfluxDB
 
 The [github.com/segmentio/stats/v5/influxdb](https://godoc.org/github.com/segmentio/stats/v5/influxdb) package sends metrics to InfluxDB using the line protocol over HTTP.

buckets_test.go

@@ -0,0 +1,150 @@
+package stats_test
+
+import (
+	"strings"
+	"testing"
+
+	stats "github.com/segmentio/stats/v5"
+	"github.com/segmentio/stats/v5/prometheus"
+	"github.com/segmentio/stats/v5/statstest"
+)
+
+// TestEngineSetBucketsKeyMatchesObserve pins the invariant SetBuckets exists
+// for: the registry key it writes is exactly the one Observe produces for the
+// same name, on the engine it was called on and on any sub-engine derived
+// from it.
+//
+// The key is read back from what Observe actually emitted rather than being
+// restated here, so the test fails if either side of the pair changes.
+func TestEngineSetBucketsKeyMatchesObserve(t *testing.T) {
+	for _, test := range []struct {
+		scenario string
+		name     string
+		engine   func(stats.Handler) *stats.Engine
+	}{
+		{
+			scenario: "engine with a prefix",
+			name:     "latency",
+			engine:   func(h stats.Handler) *stats.Engine { return stats.NewEngine("app", h) },
+		},
+		{
+			scenario: "engine with no prefix",
+			name:     "latency",
+			engine:   func(h stats.Handler) *stats.Engine { return stats.NewEngine("", h) },
+		},
+		{
+			scenario: "sub-engine derived with WithPrefix",
+			name:     "latency",
+			engine: func(h stats.Handler) *stats.Engine {
+				return stats.NewEngine("app", h).WithPrefix("sub")
+			},
+		},
+		{
+			scenario: "sub-engine derived twice",
+			name:     "latency",
+			engine: func(h stats.Handler) *stats.Engine {
+				return stats.NewEngine("app", h).WithPrefix("sub").WithPrefix("deeper")
+			},
+		},
+		{
+			// Observe splits the name on its last dot and prefixes only the
+			// measure half, so SetBuckets has to do the same.
+			scenario: "dotted name",
+			name:     "db.latency",
+			engine:   func(h stats.Handler) *stats.Engine { return stats.NewEngine("app", h) },
+		},
+	} {
+		t.Run(test.scenario, func(t *testing.T) {
+			name := test.name
+
+			h := &statstest.Handler{}
+			e := test.engine(h)
+
+			e.SetBuckets(name, 0.1, 0.2, 0.3)
+			e.Observe(name, 0.15)
+
+			// Find the measure Observe produced, skipping the go_version
+			// gauge the engine reports once.
+			var key stats.Key
+			var found bool
+			for _, m := range h.Measures() {
+				for _, f := range m.Fields {
+					if strings.HasSuffix(name, f.Name) {
+						key = stats.Key{Measure: m.Name, Field: f.Name}
+						found = true
+					}
+				}
+			}
+			if !found {
+				t.Fatalf("Observe produced no measure for %q", name)
+			}
+
+			buckets, ok := stats.Buckets[key]
+			if !ok {
+				t.Fatalf("SetBuckets did not register %#v; registry holds %#v",
+					key, keysOf(stats.Buckets))
+			}
+			if len(buckets) != 3 {
+				t.Errorf("registered %d buckets, expected 3", len(buckets))
+			}
+
+			delete(stats.Buckets, key)
+		})
+	}
+}
+
+// TestEngineSetBucketsEndToEnd runs the whole chain: buckets registered on a
+// sub-engine reach the exposition, rather than the metric silently falling
+// back to the default set.
+func TestEngineSetBucketsEndToEnd(t *testing.T) {
+	ph := &prometheus.Handler{}
+	e := stats.NewEngine("svc", ph).WithPrefix("sub")
+
+	e.SetBuckets("latency", 0.1, 0.2, 0.3)
+	defer delete(stats.Buckets, stats.Key{Measure: "svc.sub", Field: "latency"})
+
+	e.Observe("latency", 0.15)
+
+	var buf strings.Builder
+	ph.WriteStats(&buf)
+	out := buf.String()
+
+	for _, want := range []string{
+		`svc_sub_latency_bucket{le="0.1"} 0`,
+		`svc_sub_latency_bucket{le="0.2"} 1`,
+		`svc_sub_latency_bucket{le="0.3"} 1`,
+		`svc_sub_latency_bucket{le="+Inf"} 1`,
+		`svc_sub_latency_count 1`,
+	} {
+		if !strings.Contains(out, want) {
+			t.Errorf("missing %q in output:\n%s", want, out)
+		}
+	}
+
+	// Three registered boundaries plus +Inf. More would mean the registration
+	// missed and DefaultBuckets was used instead.
+	if n := strings.Count(out, "svc_sub_latency_bucket{"); n != 4 {
+		t.Errorf("found %d bucket series, expected 4:\n%s", n, out)
+	}
+}
+
+// TestBucketsSetStillWorks covers the pre-existing registration path, which
+// SetBuckets is additive to.
+func TestBucketsSetStillWorks(t *testing.T) {
+	key := stats.Key{Measure: "legacy.svc", Field: "latency"}
+	defer delete(stats.Buckets, key)
+
+	stats.Buckets.Set("legacy.svc.latency", 0.1, 0.2)
+
+	if _, ok := stats.Buckets[key]; !ok {
+		t.Errorf("Buckets.Set did not register %#v", key)
+	}
+}
+
+func keysOf(b stats.HistogramBuckets) []stats.Key {
+	keys := make([]stats.Key, 0, len(b))
+	for k := range b {
+		keys = append(keys, k)
+	}
+	return keys
+}

engine.go

@@ -130,6 +130,38 @@ func (e *Engine) ObserveAt(t time.Time, name string, value any, tags ...Tag) {
 	e.measure(t, name, value, Histogram, tags...)
 }
 
+// SetBuckets registers histogram buckets for the metric that Observe(name)
+// reports, deriving the registry key from the engine's own prefix.
+//
+// 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
+// global Buckets registry, which is an unsynchronised map that handlers read
+// on every histogram measure, so registering one concurrently with reporting
+// is a concurrent map write.
+//
+// Buckets is a single global registry shared by every engine, so it cannot
+// infer which engine a name belongs to: HistogramBuckets.Set takes the
+// fully-qualified name and merely splits what it is handed, while Observe
+// takes a name relative to the engine and has the prefix attached afterwards.
+// Registering buckets through Set therefore means restating the prefix, and a
+// mismatch is an ordinary map miss — a mistyped key and no key at all produce
+// 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:
+//
+//	e := stats.NewEngine("app", h)
+//	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
+// longer have to be registered once per derived prefix.
+//
+// The existing Buckets.Set keeps working unchanged.
+func (e *Engine) SetBuckets(name string, buckets ...any) {
+	Buckets.Set(e.makeName(name), buckets...)
+}
+
 // Clock returns a new clock identified by name and tags.
 func (e *Engine) Clock(name string, tags ...Tag) *Clock {
 	return e.ClockAt(name, time.Now(), tags...)

prometheus/append.go

@@ -34,12 +34,14 @@ func appendMetric(b []byte, metric metric) []byte {
 	b = append(b, ' ')
 	b = strconv.AppendFloat(b, metric.value, 'g', -1, 64)
 
-	if !metric.time.IsZero() {
-		t := metric.time.Unix() * 1000
-		t += int64(metric.time.Nanosecond() / 1e6) // millisecond
-		b = append(b, ' ')
-		b = strconv.AppendInt(b, t, 10)
-	}
+	// The timestamp is deliberately omitted. It is an optional field, and a
+	// series that carries one opts out of Prometheus stale-marker handling:
+	// when the series stops being exported the scraper keeps returning its
+	// last value for five minutes instead of letting it go stale.
+	//
+	// Leaving it off lets the scraper assign scrape time, which is what every
+	// other exporter does. metric.time is still tracked internally, where
+	// MetricTimeout and the store cleanup depend on it.
 
 	return append(b, '\n')
 }

prometheus/append_test.go

@@ -48,7 +48,10 @@ hello_world_bucket{le="0.5"} 42
 	},
 
 	{
-		scenario: "counter metric with help, floating point value, labels, and timestamp",
+		// metric.time is set here and must not reach the output: the
+		// timestamp is tracked for MetricTimeout and store cleanup, but
+		// exposing it opts the series out of stale-marker handling.
+		scenario: "counter metric with help, floating point value, and labels",
 		metric: metric{
 			mtype:  counter,
 			scope:  "global",
@@ -60,7 +63,7 @@ hello_world_bucket{le="0.5"} 42
 		},
 		string: `# HELP global_hello_world This is a great metric!\n
 # TYPE global_hello_world counter
-global_hello_world{question="\"???\"\n",answer="42"} 0.5 1496614320000
+global_hello_world{question="\"???\"\n",answer="42"} 0.5
 `,
 	},
 }