cockroachdb / cockroachdb/goodhistogram
Snapshot exposes no way to check that two snapshots share a layout
- Dominant language
- Go
- Stars
- 4
- Forks
- 2
- Avg merge
- 6d 48m
- Merged PRs (30d)
- 3
Description
`Snapshot` exposes `Schema()` but nothing about the range it covers. That makes it impossible for a caller holding two snapshots to verify they share a layout before combining them.
`Sub` and `Merge` index the two snapshots' `Counts` positionally with no bounds check or config comparison, so combining snapshots from differently-configured histograms silently produces wrong numbers (or panics out of range, depending on which is longer).
The case that motivates this: a metric that tracks an externally owned histogram by periodically ingesting its cumulative snapshot and subtracting the previous one to derive a windowed delta. The external histogram is built from `Params` owned by another library, so the two configs can drift apart in a dependency bump with no signal at the call site.
The best guard available today is:
```go
if cum.Schema() != prev.Schema() || len(cum.Counts) != len(prev.Counts) {
// reject
}
```
which pins the resolution and the number of octaves, but not where those octaves start — `[1µs, 10s]` and `[1ms, 10000s]` produce the same schema and the same bucket count, and compare equal.
Either of these would close it:
- An accessor on `Snapshot` for the configured bounds (`Lo()`/`Hi()`, or a `Params()` returning the effective post-`withDefaults` values).
- A config check inside `Sub`/`Merge` themselves, which would make the guard unnecessary at every call site rather than merely possible.
The second is stricter and harder to get wrong, since configs are already cached and deduplicated per `Params` — a pointer comparison on the cached config would be exact and free.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by reading Snapshot and the Sub and Merge entry points, then trace how Params, withDefaults, and the cached configuration are used. Compare the proposed Snapshot accessor with validation inside Sub/Merge, and confirm that the chosen behavior detects differing configured bounds before combining snapshots. The issue does not name specific files or tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- observability-sre
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100