cockroachdb / cockroachdb/goodhistogram

Snapshot exposes no way to check that two snapshots share a layout

Open
#8 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

`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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.