diff --git a/HISTORY.md b/HISTORY.md index 4a3f656..dda0f6c 100644 --- a/HISTORY.md +++ b/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 diff --git a/README.md b/README.md index 3bed6b7..8114862 100644 --- a/README.md +++ b/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 diff --git a/buckets.go b/buckets.go index 6ce6c50..af6a8c4 100644 --- a/buckets.go +++ b/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 diff --git a/buckets_test.go b/buckets_test.go index f59c500..db31c04 100644 --- a/buckets_test.go +++ b/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) + } +} diff --git a/engine.go b/engine.go index b3614ec..ec85ab0 100644 --- a/engine.go +++ b/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. diff --git a/httpstats/metrics.go b/httpstats/metrics.go index 7c6df4a..c411148 100644 --- a/httpstats/metrics.go +++ b/httpstats/metrics.go @@ -14,7 +14,7 @@ import ( ) func init() { - stats.Buckets.Set("http.message:header.size", + stats.Buckets.SetUnprefixed(stats.Key{Measure: "http.message", Field: "header.size"}, 5, 10, 20, @@ -23,7 +23,7 @@ func init() { math.Inf(+1), ) - stats.Buckets.Set("http.message:header.bytes", + stats.Buckets.SetUnprefixed(stats.Key{Measure: "http.message", Field: "header.bytes"}, 1e2, // 100 B 1e3, // 1 KB 1e4, // 10 KB @@ -32,7 +32,7 @@ func init() { math.Inf(+1), ) - stats.Buckets.Set("http.message:body.bytes", + stats.Buckets.SetUnprefixed(stats.Key{Measure: "http.message", Field: "body.bytes"}, 1e2, // 100 B 1e3, // 1 KB 1e4, // 10 KB @@ -44,7 +44,7 @@ func init() { math.Inf(+1), ) - stats.Buckets.Set("http:rtt.seconds", + stats.Buckets.SetUnprefixed(stats.Key{Measure: "http", Field: "rtt.seconds"}, 1*time.Millisecond, 10*time.Millisecond, 100*time.Millisecond, diff --git a/netstats/conn.go b/netstats/conn.go index e657366..1d972a1 100644 --- a/netstats/conn.go +++ b/netstats/conn.go @@ -14,7 +14,7 @@ import ( ) func init() { - stats.Buckets.Set("conn.read:bytes", + stats.Buckets.SetUnprefixed(stats.Key{Measure: "conn.read", Field: "bytes"}, 1e2, // 100 B 1e3, // 1 KB 1e4, // 10 KB @@ -22,7 +22,7 @@ func init() { math.Inf(+1), ) - stats.Buckets.Set("conn.write:bytes", + stats.Buckets.SetUnprefixed(stats.Key{Measure: "conn.write", Field: "bytes"}, 1e2, // 100 B 1e3, // 1 KB 1e4, // 10 KB diff --git a/prometheus/handler.go b/prometheus/handler.go index 3ba81dd..39aca27 100644 --- a/prometheus/handler.go +++ b/prometheus/handler.go @@ -72,9 +72,9 @@ func (h *Handler) HandleMeasures(mtime time.Time, measures ...stats.Measure) { k := stats.Key{Measure: m.Name, Field: f.Name} if b := h.Buckets; b != nil { - buckets = b[k] + buckets = b.Lookup(k.Measure, k.Field) } else { - buckets = stats.Buckets[k] + buckets = stats.Buckets.Lookup(k.Measure, k.Field) } // A registry miss returns a nil slice with no error, which diff --git a/prometheus/metric.go b/prometheus/metric.go index 1182f55..8975f91 100644 --- a/prometheus/metric.go +++ b/prometheus/metric.go @@ -67,6 +67,14 @@ func (m metric) rootName() string { type metricStore struct { mutex sync.RWMutex entries map[metricKey]*metricEntry + + // Every name the store has ever held, which cleanup deliberately does not + // prune: exposedName has to keep deciding the same way after a colliding + // entry expires, or a counter would change the name it publishes under + // mid-process. Metric names come from the program rather than from the + // data, so this is bounded by its vocabulary — label cardinality lives in + // metricEntry.states, not here. + names map[metricKey]struct{} } func (store *metricStore) lookup(mtype metricType, key metricKey, help string) *metricEntry { @@ -87,6 +95,11 @@ func (store *metricStore) lookup(mtype metricType, key metricKey, help string) * if entry = store.entries[key]; entry == nil || entry.mtype != mtype { entry = newMetricEntry(mtype, key.scope, key.name, help) store.entries[key] = entry + + if store.names == nil { + store.names = make(map[metricKey]struct{}) + } + store.names[key] = struct{}{} } store.mutex.Unlock() @@ -104,14 +117,40 @@ func (store *metricStore) update(metric metric, buckets []stats.Value) { func (store *metricStore) collect(metrics []metric) []metric { store.mutex.RLock() - for _, entry := range store.entries { - metrics = entry.collect(metrics) + for key, entry := range store.entries { + metrics = entry.collect(metrics, store.exposedName(key, entry)) } store.mutex.RUnlock() return metrics } +// 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: a +// counter "hits" and a sibling field "hits_total" both render +// _hits_total, one # TYPE line covers the pair, and a scraper keeps +// one of the two samples. Dropping the suffix costs the counter a naming +// convention; keeping it costs a series. +// +// The answer depends on which names the store has seen, never on the order +// they arrived in or on which are live right now. Consulting the live entries +// instead would let a counter switch names once its colliding sibling expired +// — a rename mid-process, which a scraper reads as one series going stale and +// another appearing. +// +// Callers hold store.mutex. +func (store *metricStore) exposedName(key metricKey, entry *metricEntry) string { + if entry.mtype != counter || entry.name == key.name { + return entry.name + } + if _, taken := store.names[metricKey{scope: key.scope, name: entry.name}]; taken { + return key.name + } + return entry.name +} + func (store *metricStore) cleanup(exp time.Time) { store.mutex.RLock() @@ -156,14 +195,11 @@ func newMetricEntry(mtype metricType, scope, name, help string) *metricEntry { 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") { + if !hasTotalSuffix(name) { entry.name = name + "_total" } @@ -176,6 +212,20 @@ func newMetricEntry(mtype metricType, scope, name, help string) *metricEntry { return entry } +// hasTotalSuffix reports whether name will already end in _total once it is +// exposed. +// +// The test has to run on the rendered name rather than the one received. A +// field is joined to its scope by an "_", so Incr("requests.total") arrives +// here as the bare field "total" and is published as _requests_total — +// already suffixed. Any byte invalid in a metric name, "." among them, also +// becomes "_" on the way out, so "requests.total" renders as requests_total. +// Comparing against the raw name misses both and yields _total_total. +func hasTotalSuffix(name string) bool { + b := appendMetricName(make([]byte, 0, len(name)), name) + return string(b) == "total" || strings.HasSuffix(string(b), "_total") +} + func (entry *metricEntry) lookup(labels labels) *metricState { key := labels.hash() @@ -197,13 +247,13 @@ func (entry *metricEntry) lookup(labels labels) *metricState { return state } -func (entry *metricEntry) collect(metrics []metric) []metric { +func (entry *metricEntry) collect(metrics []metric, name string) []metric { entry.mutex.RLock() if len(entry.states) != 0 { for _, states := range entry.states { for _, state := range states { - metrics = state.collect(metrics, entry) + metrics = state.collect(metrics, entry, name) } } } @@ -295,7 +345,7 @@ func (state *metricState) update(mtype metricType, value float64, time time.Time state.mutex.Unlock() } -func (state *metricState) collect(metrics []metric, entry *metricEntry) []metric { +func (state *metricState) collect(metrics []metric, entry *metricEntry, name string) []metric { state.mutex.Lock() // metric.time is deliberately not set here. appendMetric no longer writes @@ -307,7 +357,7 @@ func (state *metricState) collect(metrics []metric, entry *metricEntry) []metric metrics = append(metrics, metric{ mtype: entry.mtype, scope: entry.scope, - name: entry.name, + name: name, help: entry.help, value: state.value, labels: state.labels, diff --git a/prometheus/metric_test.go b/prometheus/metric_test.go index 4b7af8e..3b73f46 100644 --- a/prometheus/metric_test.go +++ b/prometheus/metric_test.go @@ -4,6 +4,7 @@ import ( "math" "reflect" "sort" + "strings" "sync" "testing" "time" @@ -123,7 +124,11 @@ func TestMetricStore(t *testing.T) { sort.Sort(byNameAndLabels(metrics)) expects := []metric{ - {mtype: counter, scope: "test", name: "A_total", value: 3, labels: labels{}}, + // "A" would be suffixed to A_total, which the sibling field A_total + // already occupies — one # TYPE line over two samples, of which a + // scraper keeps one. The suffix is dropped instead, leaving two + // distinct families. + {mtype: counter, scope: "test", name: "A", 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"}}}, @@ -345,3 +350,130 @@ func TestMakeMetricBucketsAppendsInf(t *testing.T) { }) } } + +// TestCounterTotalSuffixUsesRenderedName pins the suffix check to the name as exposed rather +// than as received. A field is joined to its scope by an "_", so Incr("x.total") +// arrives here as the bare field "total" and is already suffixed once written; +// "." also renders as "_", so a dotted name can be suffixed too. Testing the +// raw name misses both and publishes _total_total. +func TestCounterTotalSuffixUsesRenderedName(t *testing.T) { + for _, test := range []struct { + name string + want string + }{ + {name: "hits", want: "hits_total"}, + {name: "hits_total", want: "hits_total"}, + {name: "total", want: "total"}, + {name: "requests.total", want: "requests.total"}, + {name: "subtotal", want: "subtotal_total"}, + } { + t.Run(test.name, func(t *testing.T) { + if entry := newMetricEntry(counter, "app", test.name, ""); entry.name != test.want { + t.Errorf("newMetricEntry(%q).name = %q, expected %q", + test.name, entry.name, test.want) + } + }) + } +} + +// TestCounterTotalSuffixDottedName is the same invariant at the other end: +// e.Incr("requests.total") must not publish app_requests_total_total. +func TestCounterTotalSuffixDottedName(t *testing.T) { + h := &Handler{} + + // What Incr("requests.total") produces: measure "app.requests", field "total". + h.HandleMeasures(time.Now(), stats.Measure{ + Name: "app.requests", + Fields: []stats.Field{stats.MakeField("total", 1, stats.Counter)}, + }) + + var buf strings.Builder + h.WriteStats(&buf) + out := buf.String() + + if !strings.Contains(out, "app_requests_total 1") { + t.Errorf("expected app_requests_total:\n%s", out) + } + if strings.Contains(out, "total_total") { + t.Errorf("counter was double-suffixed:\n%s", out) + } +} + +// TestCounterTotalSuffixCollision covers the pair the suffix rule can bring +// onto one name. Both families have to survive, whichever order they arrive +// in, so that neither sample is dropped by the scraper. +func TestCounterTotalSuffixCollision(t *testing.T) { + for _, order := range [][]string{ + {"hits", "hits_total"}, + {"hits_total", "hits"}, + } { + t.Run(strings.Join(order, ","), func(t *testing.T) { + h := &Handler{} + + for _, name := range order { + h.HandleMeasures(time.Now(), stats.Measure{ + Name: "svc", + Fields: []stats.Field{stats.MakeField(name, 1, stats.Counter)}, + }) + } + + var buf strings.Builder + h.WriteStats(&buf) + out := buf.String() + + for _, want := range []string{ + "# TYPE svc_hits counter", + "svc_hits 1", + "# TYPE svc_hits_total counter", + "svc_hits_total 1", + } { + if !strings.Contains(out, want) { + t.Errorf("missing %q in output:\n%s", want, out) + } + } + + // One sample each. Two would mean they collided onto one name. + if n := strings.Count(out, "svc_hits_total 1"); n != 1 { + t.Errorf("found %d svc_hits_total samples, expected 1:\n%s", n, out) + } + }) + } +} + +// TestCounterTotalSuffixCollisionSurvivesCleanup covers the collision outliving +// the entry that caused it. The sibling stops being reported and is swept by +// the MetricTimeout cleanup; the counter must keep the name it has been +// publishing under, because changing it reads to a scraper as one series going +// stale and another appearing. +func TestCounterTotalSuffixCollisionSurvivesCleanup(t *testing.T) { + now := time.Now() + h := &Handler{} + + h.HandleMeasures(now, stats.Measure{ + Name: "svc", + Fields: []stats.Field{stats.MakeField("hits", 1, stats.Counter)}, + }) + h.HandleMeasures(now.Add(-time.Hour), stats.Measure{ + Name: "svc", + Fields: []stats.Field{stats.MakeField("hits_total", 5, stats.Counter)}, + }) + + var buf strings.Builder + h.WriteStats(&buf) + if out := buf.String(); !strings.Contains(out, "svc_hits 1") { + t.Fatalf("expected svc_hits before cleanup:\n%s", out) + } + + h.metrics.cleanup(now.Add(-2 * time.Minute)) + + buf.Reset() + h.WriteStats(&buf) + out := buf.String() + + if !strings.Contains(out, "svc_hits 1") { + t.Errorf("counter renamed itself after the sibling expired:\n%s", out) + } + if strings.Contains(out, "svc_hits_total") { + t.Errorf("counter took the expired sibling's name:\n%s", out) + } +}