refactor(metrics): replace NamedTimer with histograms#397
Open
sbalabanov wants to merge 1 commit into
Open
Conversation
|
|
sbalabanov
force-pushed
the
remove-named-timer-metrics
branch
2 times, most recently
from
July 16, 2026 20:35
899f30f to
63b49dd
Compare
sbalabanov
commented
Jul 16, 2026
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
force-pushed
the
remove-named-timer-metrics
branch
from
July 16, 2026 21:56
63b49dd to
4069696
Compare
sbalabanov
marked this pull request as ready for review
July 16, 2026 22:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Removes the
NamedTimermetrics helper and stops recording any duration as a tally timer. All durations are now histograms, with buckets chosen per operation.metrics.NamedTimer.Op.Completeno longer emits alatencytimer and takes a requiredbucketsparameter — there is no baked-in default, since operations differ widely in expected latency. Every callsite passes an appropriate set.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).controller_latency/ack_nack_latency, mysql subscriberpoll.latency/poll.message_age) useNamedHistogram(..., buckets, ...).RecordDuration(d)with an operation-appropriate set. No dedicated one-shot helper is added.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:
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.maxis 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.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/...— passmake fmt,make gazelle,go build ./...,go vet(changed pkgs) — clean🤖 Generated with Claude Code