Remove the two deprecated Summary metrics - #599
Open
FZambia wants to merge 2 commits into
Open
Conversation
client_command_duration_seconds and node_survey_duration_seconds were
Summary instruments carrying objectives {0.5, 0.99, 0.999}. Both were
already marked DEPRECATED in their Help text, both already had a
_histogram companion recording the same observations unconditionally, and
both were suppressed entirely when EnableNativeHistograms was set. This
removes them and the machinery that existed only to support them:
dualObserver, noopObserverVec and noopObserver.
The motivation is not only surface area. A Prometheus Summary with
objectives keeps a streaming quantile estimator behind a mutex, so every
Observe takes a lock — and command duration is observed once per command
for every connection on the node. That made it a node-wide serialization
point on the command path. In a mutex profile of 16 connections issuing
commands concurrently, prometheus.(*summary).Observe accounted for 92.5%
of all contention.
Measured with a probe issuing presence commands from 16 concurrent
connections, benchstat n=10, default config on both sides:
before 469.3n ± 5%
after 237.3n ± 3% -49.44% (p=0.000)
Command handling is roughly 2x faster, and because the lock is shared by
the whole node rather than per connection, the gap should widen with
connection count.
This is a breaking change for the metric surface. Anyone still scraping
the Summary names loses the {quantile="..."} series, and the _sum and
_count series move to the _histogram names. The migration is to
histogram_quantile() over the existing companions:
{ns}_client_command_duration_seconds -> ..._seconds_histogram
{ns}_node_survey_duration_seconds -> ..._seconds_histogram
EnableNativeHistograms keeps its remaining meaning — classic explicit
buckets versus native sparse schema — and its documentation no longer
describes suppressing Summaries, since there are none left to suppress.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #599 +/- ##
==========================================
- Coverage 85.51% 85.49% -0.03%
==========================================
Files 58 58
Lines 15576 15532 -44
==========================================
- Hits 13320 13279 -41
+ Misses 1603 1598 -5
- Partials 653 655 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ns in
Removing the Summaries leaves histogram_quantile() as the only way to get
command latency percentiles, and the existing bucket layout does not resolve
the range Centrifuge actually operates in. Commands complete in the
microsecond range - a couple of hundred microseconds is normal - but the
finest boundary was 100us, and there was a 5x step from 1ms to 5ms.
Both gaps distort exactly the quantiles people watch. histogram_quantile()
interpolates linearly inside a bucket, assuming observations are spread
evenly across it; real latency distributions decay, so a wide bucket biases
the estimate high. Simulated over 300k samples:
typical node, median ~200us
p99 true 2611.6us old 4660.5us (+78.5%) new 2749.2us (+5.3%)
fast node, median ~50us
p50 true 50.3us old 62.3us (+24.0%) new 50.5us (+0.4%)
The median being interpolated from zero on a fast node is the more
fundamental of the two: with no boundary below 100us there is nothing to
interpolate between.
Add 25us and 50us at the bottom, and 2ms and 3ms across the gap; the survey
histogram gets the same 1ms..5ms fill, since node-to-node round trips sit in
that range and it had the identical step.
The change is additive: every boundary that existed before is still present,
so le= selectors in dashboards, recording rules and SLO ratios keep matching
and only gain resolution. Command histogram goes from 15 buckets to 19, an
increase in series per method/channel_namespace combination that seemed a
fair price for the median and p99 being roughly right.
Untouched: p90 on a fast node still reads ~30% high, since 142us falls in the
100us..250us bucket. Closing that needs a boundary near 150us, which did not
seem worth a further increase in cardinality without evidence that p90 is
something operators alert on.
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
1 participant
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.
Removes
client_command_duration_secondsandnode_survey_duration_seconds, the two Prometheus Summary instruments, along with the machinery that existed only to support them (dualObserver,noopObserverVec,noopObserver).Both were already marked
DEPRECATEDin their own Help text, both already had a_histogramcompanion recording the same observations unconditionally, and both were suppressed entirely whenEnableNativeHistogramswas set.Why now: it is a node-wide lock on the command path
This started as an unrelated investigation into broadcast lock contention, where this turned up as by far the largest contention point in the whole system.
A Prometheus Summary with objectives (here
{0.5, 0.99, 0.999}) maintains a streaming quantile estimator behind a mutex, so everyObservetakes a lock. Command duration is observed once per command, for every connection on the node — so all connections serialize on one lock per label set.Mutex profile, 16 connections issuing commands concurrently:
92.5% of all contention in that workload was this one metric.
Measured effect
Probe issuing presence commands from 16 concurrent connections,
benchstatn=10, default config on both sides:Command handling is roughly 2× faster. Because the lock is node-wide rather than per-connection, the gap should widen with connection count — this was measured at 16 connections.
Breaking change
This changes the exposed metric surface. Anyone still scraping the Summary names loses the
{quantile="..."}series, and_sum/_countmove to the_histogramnames.Migration is
histogram_quantile()over the companions that already exist:For example
p99becomes:Users who had already set
EnableNativeHistograms: truesee no change at all — the Summaries were already suppressed for them.Migrating dashboards and alerts
Names verified against a live registry on this branch. Default namespace is
centrifuge(MetricsConfig.MetricsNamespace).centrifuge_client_command_duration_secondscentrifuge_client_command_duration_seconds_histogrammethod,channel_namespace(+app_*ifClientLabelsset)centrifuge_node_survey_duration_secondscentrifuge_node_survey_duration_seconds_histogramop_sumand_countmove too. This is the easiest breakage to miss: a Summary exports_sum/_countunder its base name, socentrifuge_client_command_duration_seconds_countdisappears along with the quantiles. It becomescentrifuge_client_command_duration_seconds_histogram_count.Query translations
Quantiles — the
quantilelabel is gone, usehistogram_quantile()over the_bucketseries:Average latency and throughput — just the longer metric name:
Surveys are the same shape with the
oplabel:Buckets refined in this PR
histogram_quantile()interpolates linearly inside whichever bucket the quantile falls into, assuming observations are spread evenly across it. Real latency distributions decay, so a wide bucket biases the estimate high. The previous layout did not resolve the range Centrifuge runs in — commands complete in the microsecond range, a couple of hundred microseconds being normal, but the finest boundary was 100µs and there was a 5x step from 1ms to 5ms.Both are fixed here. Simulated over 300k samples:
Added
25µsand50µsat the bottom and2ms/3msacross the gap; the survey histogram gets the same 1ms..5ms fill. The change is additive — every boundary that existed before is still present, sole=selectors in dashboards, recording rules and SLO ratios keep matching and only gain resolution. Command histogram goes 15 → 19 buckets.Every percentile is queryable, p99.9 included — accuracy depends on which bucket the quantile lands in, not on how extreme it is.
Still approximate. Even refined, a histogram estimates where a Summary stored real observed values, so numbers will move somewhat: re-check alert thresholds rather than copying them across. And p90 on a fast node still reads ~30% high (142µs falls in the 100µs..250µs bucket) — closing that needs a boundary near 150µs, which did not seem worth further cardinality without evidence that p90 is something operators alert on. Say so if it is.
For anything precise, prefer a ratio over a quantile. Bucket counts are exact — only interpolation between them is approximate — so
sum(rate(..._histogram_bucket{le="0.005"}[5m])) / sum(rate(..._histogram_count[5m]))answers "what fraction of commands completed under 5ms" with no estimation at all, at whatever percentile you care about.The largest finite bucket is 10s. If a quantile falls into
+Inf,histogram_quantile()returns 10 rather than extrapolating. Use the ratio form to detect a genuinely slow tail.One thing that gets better
Summary quantiles could not be aggregated across instances — averaging pre-computed quantiles is statistically meaningless, so per-node graphs were the only correct option. Histogram buckets add cleanly, so
sum by (le)over all pods gives a correct fleet-wide quantile. Queries that previously had to be per-instance can now be global.If you set
EnableNativeHistogramsBuckets become native/exponential and there is no
_bucketseries to sum — query the metric directly:For busy dashboards
histogram_quantileover high-cardinality buckets is more expensive than reading a Summary quantile. If a dashboard felt instant before and does not now, a recording rule restores it:Open question for review
The histograms keep their
_histogramsuffix, which only existed to avoid colliding with the Summary base name. Renaming them to the base names would be tidier, but I deliberately did not do it here: it would break everyone who has already migrated to the_histogramnames, and it would reuse a metric name with a changed instrument type. Happy to follow up separately if you want that.Other changes
EnableNativeHistogramsdocs no longer describe suppressing Summaries — there are none left to suppress. Its remaining meaning (classic explicit buckets vs native sparse schema) is unchanged._examples/native_histograms_otel/readme.mdupdated for the same reason.Testing
New
TestMetrics_NoSummariesExposedpins the removal under the default config — the path that actually needed guarding, since summaries were previously only suppressed behind the flag. It asserts the Summary names are gone, the_histogramcompanions still carry the data, and that no Summary instrument remains anywhere in the registry. It fails on master and passes here.Full suite and
go vet ./...clean.gofmt: the struct-field block was re-aligned as a consequence of removing the longest field names; the file has exactly the same number of pre-existing gofmt deviations as on master, so no new formatting noise was introduced.