Skip to content

refactor(metrics): replace NamedTimer with histograms#397

Open
sbalabanov wants to merge 1 commit into
mainfrom
remove-named-timer-metrics
Open

refactor(metrics): replace NamedTimer with histograms#397
sbalabanov wants to merge 1 commit into
mainfrom
remove-named-timer-metrics

Conversation

@sbalabanov

@sbalabanov sbalabanov commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

What

Removes the NamedTimer metrics helper and stops recording any duration as a tally timer. All durations are now histograms, with buckets chosen per operation.

  • Deletes metrics.NamedTimer.
  • Op.Complete no longer emits a latency timer and takes a required buckets parameter — there is no baked-in default, since operations differ widely in expected latency. Every callsite passes an appropriate set.
  • Exports three common duration bucket sets so callers have ready-made choices:
    • FastLatencyBuckets (~100µs–5s) — fast in-process work (scoring, cache lookups).
    • StorageLatencyBuckets (~1ms–1m) — DB / message-queue round-trips, RPC handlers.
    • LongLatencyBuckets (~5ms–4h) — long-running pipeline work and external calls (builds, merges, git, providers).
  • One-shot duration metrics (consumer controller_latency/ack_nack_latency, mysql subscriber poll.latency/poll.message_age) use NamedHistogram(..., buckets, ...).RecordDuration(d) with an operation-appropriate set. No dedicated one-shot helper is added.
  • Updates unit tests and platform/metrics/README.md.

Why timers are a bad fit for a distributed system

A tally timer ships raw per-datapoint durations, and the monitoring backend derives percentiles (p50/p99/max) per time series — one series per unique combination of metric name and tag values, so every distinct tag value (region, zone, …) multiplies the series count. The moment a dashboard or alert spans more than one series — rolling up a tag you didn't pin to a single value — the backend has to combine already-aggregated per-series statistics, and that is where timers fall apart:

  • Percentiles don't combine. The p99 across N series is not the average, the max, or any function of each series' p99. avg(series_a.p99, series_b.p99) is mathematically meaningless — you're combining summary statistics of different underlying distributions with different sample counts. The only aggregate that survives is the (count-weighted) mean.
  • max is lossy too. max(per-series maxes) happens to be correct, but everything between the mean and the max — exactly the tail latency you care about during an incident — is imprecise.
  • Precision degrades with the number of series aggregated. With every tag pinned (a single series) a timer is fine; the error grows as you roll up more series, and since each tag value adds series, high-cardinality tags make it worse. The rollups you reach for during an incident ("p99 across the whole region") are exactly the imprecise ones.

A histogram avoids this because it emits bucket counts, not pre-aggregated statistics. Bucket counts are additive: the backend merges any set of series by simply summing each bucket across them, reconstructing the true combined distribution. Percentiles are then read off the merged buckets, so p50/p99/max stay accurate at any aggregation level and over any time window. The trade-off is bucket granularity — hence per-operation bucket sets rather than one broad default, so resolution lands where the data is and empty far-out buckets don't waste series cardinality.

Testing

  • go test ./platform/... ./submitqueue/... ./runway/... ./stovepipe/... — pass
  • make fmt, make gazelle, go build ./..., go vet (changed pkgs) — clean

🤖 Generated with Claude Code

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sbalabanov
sbalabanov force-pushed the remove-named-timer-metrics branch 2 times, most recently from 899f30f to 63b49dd Compare July 16, 2026 20:35
Comment thread platform/metrics/metrics.go Outdated
Comment thread platform/metrics/metrics.go Outdated
Comment thread platform/metrics/metrics.go Outdated
Remove the NamedTimer helper and stop recording any duration as a
tally timer. Durations are now histograms throughout.

Addressing review feedback:
- Drop the NamedDurationHistogram convenience helper; callers use
  NamedHistogram with explicit buckets (+ RecordDuration for one-shots).
- Op.Complete now takes a required buckets parameter (no baked-in
  default) since operations differ widely in expected latency; every
  callsite passes an appropriate set.
- Export three common duration bucket sets from the metrics package:
  FastLatencyBuckets, StorageLatencyBuckets, LongLatencyBuckets.

Timers are unsafe in a distributed system: percentiles are computed
per time series and cannot be combined, so any rollup across series
(regions, zones, or any unpinned tag) is imprecise for everything but
the mean. Bucketed histograms merge exactly by summing buckets.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sbalabanov
sbalabanov force-pushed the remove-named-timer-metrics branch from 63b49dd to 4069696 Compare July 16, 2026 21:56
@sbalabanov
sbalabanov marked this pull request as ready for review July 16, 2026 22:12
@sbalabanov
sbalabanov requested review from a team and behinddwalls as code owners July 16, 2026 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants