diff --git a/HISTORY.md b/HISTORY.md index 86f5b51..e6f7603 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -2,6 +2,57 @@ ### 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.** + +- **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.** + +- **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. + +- **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 diff --git a/README.md b/README.md index e41627f..3bed6b7 100644 --- a/README.md +++ b/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. diff --git a/buckets_test.go b/buckets_test.go new file mode 100644 index 0000000..f59c500 --- /dev/null +++ b/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 +} diff --git a/engine.go b/engine.go index c2d63d6..dc07fd9 100644 --- a/engine.go +++ b/engine.go @@ -130,6 +130,34 @@ 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. +// +// 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. +// +// It writes to the global Buckets registry, so it must be called before the +// metrics it covers start being reported — an init function or program setup. +// 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...) diff --git a/prometheus/append.go b/prometheus/append.go index 38b1e8c..94fb371 100644 --- a/prometheus/append.go +++ b/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') } diff --git a/prometheus/append_test.go b/prometheus/append_test.go index 35640f3..6a04e65 100644 --- a/prometheus/append_test.go +++ b/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 `, }, } diff --git a/prometheus/handler.go b/prometheus/handler.go index 720b2bc..b9dc3a3 100644 --- a/prometheus/handler.go +++ b/prometheus/handler.go @@ -43,6 +43,8 @@ type Handler struct { // Buckets is the registry of histogram buckets used by the handler, // If nil, stats.Buckets is used instead. + // + // Histograms with no entry in the registry fall back to DefaultBuckets. Buckets stats.HistogramBuckets opcount atomic.Uint64 @@ -71,6 +73,15 @@ func (h *Handler) HandleMeasures(mtime time.Time, measures ...stats.Measure) { } else { buckets = stats.Buckets[k] } + + // A registry miss returns a nil slice with no error, which + // used to mean the histogram was published with _sum and + // _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 buckets == nil { + buckets = DefaultBuckets + } } h.metrics.update(metric{ @@ -141,15 +152,25 @@ func (h *Handler) ServeHTTP(res http.ResponseWriter, req *http.Request) { func (h *Handler) WriteStats(w io.Writer) { b := make([]byte, 1024) - var lastMetricName string + // A metric family is identified by its scope and root name together. The + // scope cannot be dropped here: two sub-engines derived with WithPrefix + // commonly expose the same field name, and comparing the bare name made + // the second family look like a repeat of the first, so its "# TYPE" line + // was suppressed and it ingested as untyped. + // + // byNameAndLabels.Less orders by scope before name for the same reason. + // Deduplicating on the scoped name without that ordering would turn + // missing type declarations into duplicate ones. + var lastScope, lastRootName string + metrics := h.metrics.collect(make([]metric, 0, 10000)) sort.Sort(byNameAndLabels(metrics)) for i, m := range metrics { b = b[:0] - name := m.rootName() + scope, name := m.scope, m.rootName() - if name == lastMetricName { + if scope == lastScope && name == lastRootName { // Silence the repeated output of type for values belonging to the // same metric. m.mtype, m.help = untyped, "" @@ -160,7 +181,7 @@ func (h *Handler) WriteStats(w io.Writer) { } _, _ = w.Write(appendMetric(b, m)) - lastMetricName = name + lastScope, lastRootName = scope, name } } @@ -201,6 +222,36 @@ var DefaultHandler = &Handler{ TrimPrefix: stats.DefaultEngine.Prefix, } +// DefaultBuckets is the bucket set used for histograms that have no boundaries +// registered in stats.Buckets or in Handler.Buckets. +// +// The boundaries are the ones used by the reference Prometheus client, chosen +// for request latencies measured in seconds. stats.Duration values are +// converted to seconds before bucketing, so timing histograms land on this +// range without configuration. +// +// They are a starting point, not a substitute for choosing boundaries: a +// bucketed percentile is only as accurate as the bucket it falls in, and a +// histogram whose values sit outside this range lands entirely in the +Inf +// bucket. Register real boundaries with Engine.SetBuckets wherever p99 +// accuracy matters. +// +// Programs may replace this during initialization, before any measure is +// handled. +var DefaultBuckets = []stats.Value{ + stats.ValueOf(0.005), + stats.ValueOf(0.01), + stats.ValueOf(0.025), + stats.ValueOf(0.05), + stats.ValueOf(0.1), + stats.ValueOf(0.25), + stats.ValueOf(0.5), + stats.ValueOf(1.0), + stats.ValueOf(2.5), + stats.ValueOf(5.0), + stats.ValueOf(10.0), +} + func typeOf(t stats.FieldType) metricType { switch t { case stats.Counter: diff --git a/prometheus/handler_test.go b/prometheus/handler_test.go index 7e5d5ae..a0e2a23 100644 --- a/prometheus/handler_test.go +++ b/prometheus/handler_test.go @@ -5,6 +5,7 @@ import ( "io" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -94,21 +95,22 @@ func TestServeHTTP(t *testing.T) { b, _ := io.ReadAll(res.Body) - const expects = `# TYPE A counter -A 3 1496614320000 -A{id="123"} 4 1496614320000 + const expects = `# TYPE A_total counter +A_total 3 +A_total{id="123"} 4 # TYPE B gauge -B{a="1"} 42 1496614320000 -B{a="1",b="2"} 21 1496614320000 +B{a="1"} 42 +B{a="1",b="2"} 21 # TYPE C histogram -C_bucket{le="0.25"} 2 1496614320000 -C_bucket{le="0.5"} 3 1496614320000 -C_bucket{le="0.75"} 3 1496614320000 -C_bucket{le="1"} 3 1496614320000 -C_count 4 1496614320000 -C_sum 10.7 1496614320000 +C_bucket{le="0.25"} 2 +C_bucket{le="0.5"} 3 +C_bucket{le="0.75"} 3 +C_bucket{le="1"} 3 +C_bucket{le="+Inf"} 4 +C_count 4 +C_sum 10.7 ` if s := string(b); s != expects { @@ -157,3 +159,176 @@ func BenchmarkHandleMetric(b *testing.B) { }) } } + +// TestHistogramWithoutRegisteredBuckets covers the fail-silent case: a +// histogram with no entry in the bucket registry used to publish _sum and +// _count with no _bucket series at all, so nothing looked wrong and no +// percentile could be computed. +func TestHistogramWithoutRegisteredBuckets(t *testing.T) { + now := time.Date(2017, 6, 4, 22, 12, 0, 0, time.UTC) + + handler := &Handler{} // no Buckets registry at all + + handler.HandleMeasures(now, + stats.Measure{Fields: []stats.Field{stats.MakeField("D", 0.003, stats.Histogram)}}, + stats.Measure{Fields: []stats.Field{stats.MakeField("D", 0.4, stats.Histogram)}}, + stats.Measure{Fields: []stats.Field{stats.MakeField("D", 900, stats.Histogram)}}, + ) + + var buf strings.Builder + handler.WriteStats(&buf) + out := buf.String() + + for _, want := range []string{ + `D_bucket{le="0.005"} 1`, + `D_bucket{le="0.5"} 2`, + `D_bucket{le="+Inf"} 3`, + `D_count 3`, + } { + if !strings.Contains(out, want) { + t.Errorf("missing %q in output:\n%s", want, out) + } + } + + // Every registered boundary plus +Inf must be present. + if n := strings.Count(out, "D_bucket{"); n != len(DefaultBuckets)+1 { + t.Errorf("found %d bucket series, expected %d", n, len(DefaultBuckets)+1) + } +} + +// TestTypeDeclarationPerScope covers same-named fields arriving from +// different engine prefixes, which is what deriving sub-engines with +// WithPrefix produces. +// +// The dedup used to compare the bare field name with the scope discarded, so +// only the first scope to emit "hits" got a "# TYPE" line and every later one +// ingested as untyped. +func TestTypeDeclarationPerScope(t *testing.T) { + now := time.Date(2017, 6, 4, 22, 12, 0, 0, time.UTC) + + handler := &Handler{} + + scopes := []string{"alpha", "beta", "gamma"} + for _, scope := range scopes { + handler.HandleMeasures(now, stats.Measure{ + Name: scope, + Fields: []stats.Field{ + stats.MakeField("hits", 1, stats.Counter), + stats.MakeField("size", 2, stats.Gauge), + }, + }) + } + + var buf strings.Builder + handler.WriteStats(&buf) + out := buf.String() + + for _, scope := range scopes { + for _, want := range []string{ + "# TYPE " + scope + "_hits_total counter", + "# TYPE " + scope + "_size gauge", + } { + if n := strings.Count(out, want); n != 1 { + t.Errorf("found %q %d times, expected exactly 1:\n%s", want, n, out) + } + } + } + + // Six metrics, six type declarations, none repeated. + if n := strings.Count(out, "# TYPE "); n != 2*len(scopes) { + t.Errorf("found %d type declarations, expected %d", n, 2*len(scopes)) + } +} + +// TestTypeDeclarationAcrossAdjacentScopes is the tighter version of the case +// above: when each scope exposes the same single field name, the families are +// adjacent in the output and a dedup that compares the bare name suppresses +// every one after the first. +func TestTypeDeclarationAcrossAdjacentScopes(t *testing.T) { + now := time.Date(2017, 6, 4, 22, 12, 0, 0, time.UTC) + + handler := &Handler{} + + scopes := []string{"alpha", "beta", "gamma"} + for _, scope := range scopes { + handler.HandleMeasures(now, stats.Measure{ + Name: scope, + Fields: []stats.Field{ + stats.MakeField("hits", 1, stats.Counter), + // A histogram spans three series names, so its family only + // stays contiguous if the sort orders by scope before name. + // Sorting on the bare name groups every scope's _bucket + // together and pushes _count and _sum away from it, which + // makes the same family declare its type more than once. + stats.MakeField("latency", 0.1, stats.Histogram), + }, + }) + } + + var buf strings.Builder + handler.WriteStats(&buf) + out := buf.String() + + for _, scope := range scopes { + for _, want := range []string{ + "# TYPE " + scope + "_hits_total counter", + "# TYPE " + scope + "_latency histogram", + } { + if n := strings.Count(out, want); n != 1 { + t.Errorf("found %q %d times, expected exactly 1:\n%s", want, n, out) + } + } + } + + if n := strings.Count(out, "# TYPE "); n != 2*len(scopes) { + t.Errorf("found %d type declarations, expected %d:\n%s", n, 2*len(scopes), out) + } +} + +// TestCounterTotalSuffix covers the _total naming rule. Only counters get the +// suffix, and a counter that already carries it is left alone. +func TestCounterTotalSuffix(t *testing.T) { + now := time.Date(2017, 6, 4, 22, 12, 0, 0, time.UTC) + + handler := &Handler{} + handler.HandleMeasures(now, stats.Measure{ + Name: "svc", + Fields: []stats.Field{ + stats.MakeField("requests", 1, stats.Counter), + stats.MakeField("errors_total", 2, stats.Counter), + stats.MakeField("queue_depth", 3, stats.Gauge), + stats.MakeField("latency", 0.1, stats.Histogram), + }, + }) + + var buf strings.Builder + handler.WriteStats(&buf) + out := buf.String() + + for _, want := range []string{ + "# TYPE svc_requests_total counter", + "svc_requests_total 1", + // Already suffixed: must not become errors_total_total. + "# TYPE svc_errors_total counter", + "svc_errors_total 2", + // Gauges and histograms are untouched. + "# TYPE svc_queue_depth gauge", + "svc_queue_depth 3", + "# TYPE svc_latency histogram", + "svc_latency_count 1", + } { + if !strings.Contains(out, want) { + t.Errorf("missing %q in output:\n%s", want, out) + } + } + + for _, unwanted := range []string{ + "svc_errors_total_total", + "svc_queue_depth_total", + "svc_latency_total", + } { + if strings.Contains(out, unwanted) { + t.Errorf("unexpected %q in output:\n%s", unwanted, out) + } + } +} diff --git a/prometheus/label.go b/prometheus/label.go index 7a8068d..5a053d8 100644 --- a/prometheus/label.go +++ b/prometheus/label.go @@ -1,6 +1,8 @@ package prometheus import ( + "strconv" + "github.com/segmentio/fasthash/jody" "github.com/segmentio/stats/v5" @@ -16,7 +18,24 @@ func (l label) equal(other label) bool { } func (l label) less(other label) bool { - return l.name < other.name || (l.name == other.name && l.value < other.value) + if l.name != other.name { + return l.name < other.name + } + + // Histogram bucket boundaries have to come out in increasing numeric + // order. Comparing them as strings puts "+Inf" first ('+' is ASCII 43, + // digits start at 48) and orders "10" ahead of "2". + // + // Scoped to "le" so that every other label keeps comparing as a string. + if l.name == "le" { + v1, err1 := strconv.ParseFloat(l.value, 64) + v2, err2 := strconv.ParseFloat(other.value, 64) + if err1 == nil && err2 == nil { + return v1 < v2 + } + } + + return l.value < other.value } type labels []label diff --git a/prometheus/label_test.go b/prometheus/label_test.go index 8fb10ad..00f0ce9 100644 --- a/prometheus/label_test.go +++ b/prometheus/label_test.go @@ -59,3 +59,42 @@ func TestLabelsLess(t *testing.T) { }) } } + +func TestLabelLessLE(t *testing.T) { + tests := []struct { + name string + l1 label + l2 label + less bool + }{ + // +Inf must sort after every numeric boundary. As raw strings it + // sorts first, because '+' is ASCII 43 and digits start at 48. + {name: "+Inf after a boundary", l1: label{"le", "0.25"}, l2: label{"le", "+Inf"}, less: true}, + {name: "+Inf not before a boundary", l1: label{"le", "+Inf"}, l2: label{"le", "0.25"}, less: false}, + {name: "+Inf equals itself", l1: label{"le", "+Inf"}, l2: label{"le", "+Inf"}, less: false}, + + // Numeric order, not lexical: "10" < "2" as a string. + {name: "10 after 2", l1: label{"le", "10"}, l2: label{"le", "2"}, less: false}, + {name: "2 before 10", l1: label{"le", "2"}, l2: label{"le", "10"}, less: true}, + {name: "fractional", l1: label{"le", "0.005"}, l2: label{"le", "0.01"}, less: true}, + + // Other labels keep comparing as strings, so a numeric-looking value + // on a non-le label is unaffected. + {name: "non-le stays lexical", l1: label{"code", "10"}, l2: label{"code", "2"}, less: true}, + + // A non-numeric le value falls back to string comparison rather than + // treating the parse failure as equality. + {name: "unparseable le", l1: label{"le", "abc"}, l2: label{"le", "abd"}, less: true}, + + // Name comparison still wins over value comparison. + {name: "different names", l1: label{"a", "9"}, l2: label{"le", "1"}, less: true}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if less := test.l1.less(test.l2); less != test.less { + t.Errorf("(%v < %v) = %t, expected %t", test.l1, test.l2, less, test.less) + } + }) + } +} diff --git a/prometheus/metric.go b/prometheus/metric.go index 77970e5..2665a6a 100644 --- a/prometheus/metric.go +++ b/prometheus/metric.go @@ -1,6 +1,7 @@ package prometheus import ( + "math" "strconv" "strings" "sync" @@ -150,9 +151,23 @@ func newMetricEntry(mtype metricType, scope, name, help string) *metricEntry { states: make(metricStateMap), } - if mtype == histogram { - // Here we cache those metric names to avoid having to recompute them - // every time we collect the state of the metrics. + // Here we cache those metric names to avoid having to recompute them + // every time we collect the state of the metrics. + switch mtype { + case counter: + // Prometheus expects an accumulating count to carry a "total" suffix. + // It is more than convention: the OpenMetrics encoder keys the type + // line on the suffix, so a counter without it is published as + // unknown. + // + // 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") { + entry.name = name + "_total" + } + + case histogram: entry.bucket = name + "_bucket" entry.sum = name + "_sum" entry.count = name + "_count" @@ -265,7 +280,10 @@ func (state *metricState) update(mtype metricType, value float64, time time.Time state.value = value case histogram: - if len(state.buckets) != len(buckets) { + // makeMetricBuckets appends a +Inf bucket, so the state holds one more + // entry than the registry slice. Comparing against len(buckets) here + // would rebuild — and zero the counts — on every observation. + if len(state.buckets) != len(buckets)+1 { state.buckets = makeMetricBuckets(buckets, state.labels) } state.buckets.update(value) @@ -361,8 +379,18 @@ type metricBucket struct { type metricBuckets []metricBucket +// makeMetricBuckets builds the bucket set for a histogram state, with one +// entry per registered boundary plus a final +Inf bucket. +// +// The +Inf bucket is not optional: histogram_quantile returns NaN unless the +// highest bucket has an upper bound of +Inf, and without it observations above +// the last registered boundary are counted in _sum and _count but land in no +// bucket at all. +// +// Callers that compare an existing bucket set against the registry slice to +// decide whether to rebuild must account for the extra entry. func makeMetricBuckets(buckets []stats.Value, labels labels) metricBuckets { - b := make(metricBuckets, len(buckets)) + b := make(metricBuckets, len(buckets)+1) s := le(buckets) for i := range buckets { @@ -372,6 +400,9 @@ func makeMetricBuckets(buckets []stats.Value, labels labels) metricBuckets { b[i].labels = labels.copyAppend(label{"le", le}) } + b[len(buckets)].limit = math.Inf(1) + b[len(buckets)].labels = labels.copyAppend(label{"le", "+Inf"}) + return b } @@ -434,8 +465,24 @@ func (metrics byNameAndLabels) Swap(i, j int) { metrics[i], metrics[j] = metrics[j], metrics[i] } +// Less orders by scope before name, so that every metric sharing a scope and +// a root name stays contiguous in the output. +// +// Ordering on the bare name would interleave same-named fields coming from +// different engine prefixes — hits from two WithPrefix sub-engines, say — +// which breaks up the family a single "# TYPE" line is meant to cover. +// +// Comparing the two parts in turn rather than the joined "scope_name" avoids +// building a string for every comparison in the sort. func (metrics byNameAndLabels) Less(i, j int) bool { m1 := &metrics[i] m2 := &metrics[j] - return m1.name < m2.name || (m1.name == m2.name && m1.labels.less(m2.labels)) + + if m1.scope != m2.scope { + return m1.scope < m2.scope + } + if m1.name != m2.name { + return m1.name < m2.name + } + return m1.labels.less(m2.labels) } diff --git a/prometheus/metric_test.go b/prometheus/metric_test.go index 362eb29..7f5da5e 100644 --- a/prometheus/metric_test.go +++ b/prometheus/metric_test.go @@ -100,7 +100,7 @@ func TestMetricStore(t *testing.T) { {mtype: counter, scope: "test", name: "A", value: 2}, {mtype: histogram, scope: "test", name: "C", value: 0.1}, {mtype: gauge, scope: "test", name: "B", value: 1, labels: labels{{"a", "1"}, {"b", "2"}}}, - {mtype: counter, scope: "test", name: "A", value: 4, labels: labels{{"id", "123"}}}, + {mtype: counter, scope: "test", name: "A_total", value: 4, labels: labels{{"id", "123"}}}, {mtype: gauge, scope: "test", name: "B", value: 42, labels: labels{{"a", "1"}}}, {mtype: histogram, scope: "test", name: "C", value: 0.1}, {mtype: gauge, scope: "test", name: "B", value: 21, labels: labels{{"a", "1"}, {"b", "2"}}}, @@ -123,14 +123,18 @@ func TestMetricStore(t *testing.T) { sort.Sort(byNameAndLabels(metrics)) expects := []metric{ - {mtype: counter, scope: "test", name: "A", value: 3, labels: labels{}}, - {mtype: counter, scope: "test", name: "A", value: 4, labels: labels{{"id", "123"}}}, + {mtype: counter, scope: "test", name: "A_total", value: 3, labels: labels{}}, + {mtype: counter, scope: "test", name: "A_total", value: 4, labels: labels{{"id", "123"}}}, {mtype: gauge, scope: "test", name: "B", value: 42, labels: labels{{"a", "1"}}}, {mtype: gauge, scope: "test", name: "B", value: 21, labels: labels{{"a", "1"}, {"b", "2"}}}, {mtype: histogram, scope: "test", name: "C_bucket", value: 2, labels: labels{{"le", "0.25"}}}, {mtype: histogram, scope: "test", name: "C_bucket", value: 3, labels: labels{{"le", "0.5"}}}, {mtype: histogram, scope: "test", name: "C_bucket", value: 3, labels: labels{{"le", "0.75"}}}, {mtype: histogram, scope: "test", name: "C_bucket", value: 3, labels: labels{{"le", "1"}}}, + // The 10 observation exceeds the highest registered boundary, so the + // +Inf bucket matches C_count. It sorts last because le compares + // numerically. + {mtype: histogram, scope: "test", name: "C_bucket", value: 4, labels: labels{{"le", "+Inf"}}}, {mtype: histogram, scope: "test", name: "C_count", value: 4, labels: labels{}}, {mtype: histogram, scope: "test", name: "C_sum", value: 10.7, labels: labels{}}, } @@ -249,7 +253,7 @@ func TestMetricStoreCleanup(t *testing.T) { sort.Sort(byNameAndLabels(metrics)) if !reflect.DeepEqual(metrics, []metric{ - {mtype: counter, name: "E", value: 1, time: now.Add(time.Second), labels: labels{}}, + {mtype: counter, name: "E_total", value: 1, time: now.Add(time.Second), labels: labels{}}, }) { t.Errorf("bad metrics: %#v", metrics) } @@ -269,3 +273,71 @@ func BenchmarkLE(b *testing.B) { le(buckets) } } + +// TestMetricStateBucketsNotRebuilt guards the interaction between the +Inf +// bucket and the rebuild check in metricState.update. +// +// makeMetricBuckets returns len(buckets)+1 entries. If update compares the +// stored slice against len(buckets) it never matches, so every observation +// reallocates the bucket set and discards the counts accumulated so far — +// leaving _count climbing while every _bucket stays at 0 or 1. +func TestMetricStateBucketsNotRebuilt(t *testing.T) { + buckets := []stats.Value{ + stats.ValueOf(0.25), + stats.ValueOf(0.5), + } + + state := newMetricState(labels{}) + + const observations = 10 + for range observations { + state.update(histogram, 0.1, time.Now(), buckets) + } + + if n := len(state.buckets); n != len(buckets)+1 { + t.Fatalf("expected %d buckets (registered + Inf), found %d", len(buckets)+1, n) + } + + // Every observation is 0.1, so all of them belong in the lowest bucket. + if c := state.buckets[0].count; c != observations { + t.Errorf("buckets were rebuilt: le=0.25 has count %d, expected %d", c, observations) + } + + if state.count != observations { + t.Errorf("count = %d, expected %d", state.count, observations) + } +} + +// TestMakeMetricBucketsAppendsInf covers the empty-registry case, where the +// +Inf bucket is the only one, and confirms it catches overflow. +func TestMakeMetricBucketsAppendsInf(t *testing.T) { + for _, test := range []struct { + name string + buckets []stats.Value + }{ + {name: "empty", buckets: nil}, + {name: "one boundary", buckets: []stats.Value{stats.ValueOf(1.0)}}, + } { + t.Run(test.name, func(t *testing.T) { + b := makeMetricBuckets(test.buckets, labels{}) + + if len(b) != len(test.buckets)+1 { + t.Fatalf("expected %d buckets, found %d", len(test.buckets)+1, len(b)) + } + + last := b[len(b)-1] + if !math.IsInf(last.limit, 1) { + t.Errorf("last bucket limit = %v, expected +Inf", last.limit) + } + if got := last.labels[len(last.labels)-1]; got != (label{"le", "+Inf"}) { + t.Errorf("last bucket label = %v, expected le=+Inf", got) + } + + // A value above every registered boundary must still be counted. + b.update(1e9) + if c := b[len(b)-1].count; c != 1 { + t.Errorf("overflow observation not counted in +Inf bucket (count = %d)", c) + } + }) + } +}