cockroachdb / cockroachdb/goodhistogram

Native histogram export omits Underflow and Overflow, violating the Prometheus count invariant

Open
#7 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
4
Forks
2
Avg merge
6d 48m
Merged PRs (30d)
3

Description

A `Histogram` sorts every observation into one of four bins: `ZeroCount` (`v <= 0`), `Underflow` (`0 < v < lo`), `Counts` (`lo <= v <= hi`), and `Overflow` (`v > hi`). `Snapshot.TotalCount` is the sum of all four, and `ToPrometheusHistogram` sets `SampleCount` from it ([export.go#L23](https://github.com/cockroachdb/goodhistogram/blob/43c30e2d6de1912127a550bb48f2f9a286f82953/export.go#L23)).

`populateNativeFields` builds `PositiveSpan`/`PositiveDelta` by walking `s.Counts` alone ([export.go#L95](https://github.com/cockroachdb/goodhistogram/blob/43c30e2d6de1912127a550bb48f2f9a286f82953/export.go#L95)), and sets `ZeroThreshold = math.SmallestNonzeroFloat64` with `ZeroCount = s.ZeroCount` ([export.go#L79-L82](https://github.com/cockroachdb/goodhistogram/blob/43c30e2d6de1912127a550bb48f2f9a286f82953/export.go#L79-L82)). `Underflow` and `Overflow` are read by nobody. So for a native-histogram consumer:

```
SampleCount != ZeroCount + Σ(bucket counts)
```

which the format requires to hold. Unlike the conventional format there is no `+Inf` bucket to absorb the difference — the schema defines an infinite series, so a native consumer has no place to put mass it can't see.

## Why it matters

`histogram_quantile` computes `rank = q × count` and then walks buckets accumulating until it reaches `rank`. When the two numbers come from different populations the walk is meaningless, and the error goes in opposite directions depending on which bin is populated:

- **Underflow-dominant** — the walk is missing mass *below* every bucket, so it reaches `rank` too late and **over-reports**. This is the common case: a metric whose floor is set for one scale of operation but which is fed observations from a finer one puts most of its mass here, and a true sub-floor p50 can read as if it were near the top of the range.
- **Overflow-dominant** — `rank` exceeds everything the buckets can account for, the walk runs off the end, and the result pins to the top of the highest *populated* in-range bucket, so it **under-reports**. This is the worse direction, because overflow is populated exactly when something is going wrong, and the metric reports a healthy-looking value while it happens.

Both are silent. Nothing in the exposition is malformed; the numbers are just wrong.

## Repro

```go
h := goodhistogram.New(goodhistogram.Params{Lo: 10e3, Hi: 10e9, ErrorBound: 0.12})
for i := 0; i < 1_000_000; i++ {
h.Record(500) // 500ns: below Lo
}
for i := 0; i < 3000; i++ {
h.Record(50_000) // 50µs: in range
}
for i := 0; i < 7; i++ {
h.Record(30_000_000_000) // 30s: above Hi
}
h.Record(0)

s := h.Snapshot()
p := s.ToPrometheusHistogram()

visible := p.GetZeroCount()
var prev int64
for _, d := range p.PositiveDelta {
prev += d
visible += uint64(prev)
}
```

```
SampleCount 1003008
native ZeroCount + span counts 3001
missing from native buckets 1000007
conventional first le=11585.237502960395 cum=1000001
conventional +Inf cum=1003008
```

99.7% of the observations are invisible to a native consumer while still being counted in `SampleCount`.

## The other two consumers are correct

Worth noting because it makes this specifically an export-path defect rather than a modelling one:

- `conventionalBuckets` seeds the running cumulative count with `ZeroCount + Underflow` ([export.go#L48](https://github.com/cockroachdb/goodhistogram/blob/43c30e2d6de1912127a550bb48f2f9a286f82953/export.go#L48)) and gives the `+Inf` bucket the full `TotalCount` rather than the accumulated value ([export.go#L60](https://github.com/cockroachdb/goodhistogram/blob/43c30e2d6de1912127a550bb48f2f9a286f82953/export.go#L60)), which picks up `Overflow`. The invariant holds and the text format is fine.
- `Snapshot.ValueAtQuantile` accounts for both bins as well, so in-process quantiles are fine.

Only the native path disagrees, and it disagrees with `SampleCount`, which it emits itself.

Filing this to catalogue the behavior; no fix proposed here.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in export.go with populateNativeFields, then compare its handling of Snapshot fields with conventionalBuckets and Snapshot.ValueAtQuantile. Reproduce the issue using the Go example in the report and trace how Underflow and Overflow affect the native representation; done means the native histogram accounts for all observations while preserving the Prometheus count invariant.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, prometheus
Domain
observability-sre
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.