From 979516b088e94555a33a7937e925cf01b2c7a6f9 Mon Sep 17 00:00:00 2001 From: FZambia Date: Wed, 29 Jul 2026 08:53:27 +0300 Subject: [PATCH 1/2] Remove the two deprecated Summary metrics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- _examples/native_histograms_otel/readme.md | 11 +- config.go | 27 +---- metrics.go | 122 ++++----------------- metrics_test.go | 57 +++++++++- 4 files changed, 85 insertions(+), 132 deletions(-) diff --git a/_examples/native_histograms_otel/readme.md b/_examples/native_histograms_otel/readme.md index f2c36d87..86cc88db 100644 --- a/_examples/native_histograms_otel/readme.md +++ b/_examples/native_histograms_otel/readme.md @@ -11,13 +11,12 @@ Centrifuge metrics (native histograms) → stdoutmetric exporter (prints OTel JSON) ``` -Centrifuge exposes both a Summary and a Histogram for the two duration -metrics that have historically been Summaries — `command_duration_seconds` -and `survey_duration_seconds`. When `EnableNativeHistograms` is on: +The two duration metrics that were historically Summaries — +`command_duration_seconds` and `survey_duration_seconds` — are now exposed +only as Histograms, under their `_histogram` names. When +`EnableNativeHistograms` is on: -- The Summaries are no longer exposed (no-op internally; absent from output). -- The companion `_histogram` metrics switch to native (sparse, exponential) - schema. +- Those `_histogram` metrics switch to native (sparse, exponential) schema. - The bridge translates native histograms to OTel `ExponentialHistogram` — the high-fidelity form most OTel-native backends prefer. diff --git a/config.go b/config.go index 68e8af67..e8e367c8 100644 --- a/config.go +++ b/config.go @@ -443,30 +443,15 @@ type MetricsConfig struct { ClientLabels []string // EnableNativeHistograms switches every Histogram instrument in the // package to Prometheus native (sparse, exponential) schema with no - // explicit buckets exposed, and stops exposing the legacy Summary - // counterparts of dual-instrument metrics. Designed for OpenTelemetry - // export via the client_golang Prometheus bridge — native histograms - // map to OTel ExponentialHistogram, and dropping Summaries keeps OTel - // Summary (which most backends treat as second-class) out of the - // pipeline. + // explicit buckets exposed. Designed for OpenTelemetry export via the + // client_golang Prometheus bridge, where native histograms map to OTel + // ExponentialHistogram. // - // Centrifuge exposes both a Summary and a Histogram for the two - // distribution metrics that have historically been Summaries: - // - {ns}_client_command_duration_seconds (Summary) + - // {ns}_client_command_duration_seconds_histogram (Histogram) - // - {ns}_node_survey_duration_seconds (Summary) + - // {ns}_node_survey_duration_seconds_histogram (Histogram) - // Both are constructed and observed unconditionally. - // - // Default is false: today's behavior is preserved (Summaries still - // exposed; companion Histograms additionally exposed with classic - // explicit buckets — see Histograms section in metrics docs for the - // exact bucket lists). + // Default is false: Histograms are exposed with classic explicit + // buckets — see the Histograms section in the metrics docs for the + // exact bucket lists. // // When set to true: - // - Summaries are no longer exposed (the underlying instrument - // becomes a no-op so cached observers continue to satisfy - // prometheus.Observer without nil-checks). // - All Histograms switch to native (sparse, exponential) schema // with no explicit buckets. // - Text-format Prometheus scrapes lose _bucket series on every diff --git a/metrics.go b/metrics.go index 3744093b..55a424b5 100644 --- a/metrics.go +++ b/metrics.go @@ -80,30 +80,24 @@ var ( ) type metrics struct { - messagesSentCount *prometheus.CounterVec - messagesReceivedCount *prometheus.CounterVec - actionCount *prometheus.CounterVec - buildInfoGauge *prometheus.GaugeVec - numClientsGauge prometheus.Gauge - numUsersGauge prometheus.Gauge - numSubsGauge prometheus.Gauge - numChannelsGauge prometheus.Gauge - numNodesGauge prometheus.Gauge - replyErrorCount *prometheus.CounterVec - connectionsAccepted *prometheus.CounterVec - connectionsInflight *prometheus.GaugeVec - subscriptionsAccepted *prometheus.CounterVec - subscriptionsInflight *prometheus.GaugeVec - serverUnsubscribeCount *prometheus.CounterVec - serverDisconnectCount *prometheus.CounterVec - transportOutgoingCloseCount *prometheus.CounterVec - // commandDurationSummary holds the legacy Summary by default; when - // EnableNativeHistograms is true it is a no-op (the Summary is not - // exposed). The companion commandDurationHistogram below always carries - // the real observations, with native schema when the flag is on. - commandDurationSummary prometheus.ObserverVec + messagesSentCount *prometheus.CounterVec + messagesReceivedCount *prometheus.CounterVec + actionCount *prometheus.CounterVec + buildInfoGauge *prometheus.GaugeVec + numClientsGauge prometheus.Gauge + numUsersGauge prometheus.Gauge + numSubsGauge prometheus.Gauge + numChannelsGauge prometheus.Gauge + numNodesGauge prometheus.Gauge + replyErrorCount *prometheus.CounterVec + connectionsAccepted *prometheus.CounterVec + connectionsInflight *prometheus.GaugeVec + subscriptionsAccepted *prometheus.CounterVec + subscriptionsInflight *prometheus.GaugeVec + serverUnsubscribeCount *prometheus.CounterVec + serverDisconnectCount *prometheus.CounterVec + transportOutgoingCloseCount *prometheus.CounterVec commandDurationHistogram *prometheus.HistogramVec - surveyDurationSummary prometheus.ObserverVec surveyDurationHistogram *prometheus.HistogramVec recoverCount *prometheus.CounterVec recoveredPublications *prometheus.HistogramVec @@ -375,50 +369,6 @@ func buildClientLabelsCacheKeyFromMap(labelNames []string, labelsMap map[string] return b.String() } -// dualObserver fans observations out to both a Summary and a Histogram -// observer. Used when a metric is exposed as both instrument types — the -// Summary preserves existing {quantile="..."} dashboards while the Histogram -// supplies the histogram_quantile()- and OpenTelemetry-friendly form. When -// EnableNativeHistograms is true the Summary side is a no-op, so only the -// Histogram records data. -type dualObserver struct { - summary, histogram prometheus.Observer -} - -func (d dualObserver) Observe(v float64) { - d.summary.Observe(v) - d.histogram.Observe(v) -} - -// noopObserverVec implements prometheus.ObserverVec with all no-op methods. -// Assigned to a Summary accessor when EnableNativeHistograms is true so the -// Summary side of a dual-instrument metric is not exposed and contributes no -// observation cost. Callers that cache observers via WithLabelValues do not -// need nil-checks — they get a noopObserver that silently drops Observe() -// calls. -type noopObserverVec struct{} - -func (noopObserverVec) Describe(chan<- *prometheus.Desc) {} -func (noopObserverVec) Collect(chan<- prometheus.Metric) {} -func (noopObserverVec) WithLabelValues(...string) prometheus.Observer { return noopObserver{} } -func (noopObserverVec) With(prometheus.Labels) prometheus.Observer { return noopObserver{} } -func (noopObserverVec) GetMetricWith(prometheus.Labels) (prometheus.Observer, error) { - return noopObserver{}, nil -} -func (noopObserverVec) GetMetricWithLabelValues(...string) (prometheus.Observer, error) { - return noopObserver{}, nil -} -func (noopObserverVec) CurryWith(prometheus.Labels) (prometheus.ObserverVec, error) { - return noopObserverVec{}, nil -} -func (noopObserverVec) MustCurryWith(prometheus.Labels) prometheus.ObserverVec { - return noopObserverVec{} -} - -type noopObserver struct{} - -func (noopObserver) Observe(float64) {} - // nativeHistogramOpts returns opts unchanged when native is false. When true, // it enables Prometheus native histogram schema with no explicit buckets — // the metric exposes only _count, _sum, and the native histogram chunk. @@ -527,17 +477,6 @@ func newMetricsRegistry(config MetricsConfig) (*metrics, error) { Help: "Number of channels with one or more subscribers.", }) - if config.EnableNativeHistograms { - m.surveyDurationSummary = noopObserverVec{} - } else { - m.surveyDurationSummary = prometheus.NewSummaryVec(prometheus.SummaryOpts{ - Namespace: metricsNamespace, - Subsystem: "node", - Name: "survey_duration_seconds", - Objectives: map[float64]float64{0.5: 0.05, 0.99: 0.001, 0.999: 0.0001}, - Help: "DEPRECATED — use survey_duration_seconds_histogram. Will be removed in future releases. Survey duration summary.", - }, []string{"op"}) - } m.surveyDurationHistogram = prometheus.NewHistogramVec(nativeHistogramOpts(prometheus.HistogramOpts{ Namespace: metricsNamespace, Subsystem: "node", @@ -563,17 +502,6 @@ func newMetricsRegistry(config MetricsConfig) (*metrics, error) { Help: "Number of messages received from broker.", }, []string{"type", "channel_namespace"}) - if config.EnableNativeHistograms { - m.commandDurationSummary = noopObserverVec{} - } else { - m.commandDurationSummary = prometheus.NewSummaryVec(prometheus.SummaryOpts{ - Namespace: metricsNamespace, - Subsystem: metricClientCommandDuration.Subsystem, - Name: metricClientCommandDuration.Name, - Objectives: map[float64]float64{0.5: 0.05, 0.99: 0.001, 0.999: 0.0001}, - Help: "DEPRECATED — use command_duration_seconds_histogram. Will be removed in future releases. Client command duration summary.", - }, m.buildMetricLabels([]string{"method", "channel_namespace"})) - } m.commandDurationHistogram = prometheus.NewHistogramVec(nativeHistogramOpts(prometheus.HistogramOpts{ Namespace: metricsNamespace, Subsystem: metricClientCommandDuration.Subsystem, @@ -955,10 +883,7 @@ func newMetricsRegistry(config MetricsConfig) (*metrics, error) { makeCommandObserver := func(method string) prometheus.Observer { labels := buildCommandLabels(method) - return dualObserver{ - summary: m.commandDurationSummary.WithLabelValues(labels...), - histogram: m.commandDurationHistogram.WithLabelValues(labels...), - } + return m.commandDurationHistogram.WithLabelValues(labels...) } m.commandDurationConnect = makeCommandObserver(labelForMethod(protocol.FrameTypeConnect)) m.commandDurationSubscribe = makeCommandObserver(labelForMethod(protocol.FrameTypeSubscribe)) @@ -984,7 +909,6 @@ func newMetricsRegistry(config MetricsConfig) (*metrics, error) { m.numSubsGauge, m.numChannelsGauge, m.numNodesGauge, - m.commandDurationSummary, m.commandDurationHistogram, m.replyErrorCount, m.connectionsAccepted, @@ -1002,7 +926,6 @@ func newMetricsRegistry(config MetricsConfig) (*metrics, error) { m.transportMessagesReceivedSize, m.tagsFilterDroppedCount, m.buildInfoGauge, - m.surveyDurationSummary, m.surveyDurationHistogram, m.pubSubLagHistogram, m.broadcastDurationHistogram, @@ -1098,10 +1021,7 @@ func (m *metrics) observeCommandDuration(frameType protocol.FrameType, d time.Du if !ok { baseLabels := []string{frameType.String(), channelNamespace} labelValues := m.appendClientLabels(baseLabels, c) - observer = dualObserver{ - summary: m.commandDurationSummary.WithLabelValues(labelValues...), - histogram: m.commandDurationHistogram.WithLabelValues(labelValues...), - } + observer = m.commandDurationHistogram.WithLabelValues(labelValues...) m.commandDurationCache.Store(labels, observer) } observer.(prometheus.Observer).Observe(d.Seconds()) @@ -1506,9 +1426,7 @@ func (m *metrics) incActionCount(action string, ch string) { } func (m *metrics) observeSurveyDuration(op string, d time.Duration) { - seconds := d.Seconds() - m.surveyDurationSummary.WithLabelValues(op).Observe(seconds) - m.surveyDurationHistogram.WithLabelValues(op).Observe(seconds) + m.surveyDurationHistogram.WithLabelValues(op).Observe(d.Seconds()) } type tagsFilterDroppedLabels struct { diff --git a/metrics_test.go b/metrics_test.go index 7fbf527e..430d1414 100644 --- a/metrics_test.go +++ b/metrics_test.go @@ -845,6 +845,58 @@ func BenchmarkSharedPollResultCached(b *testing.B) { }) } +// TestMetrics_NoSummariesExposed pins the removal of the two deprecated Summary +// metrics. They were previously exposed by default and only suppressed when +// EnableNativeHistograms was set, so the default path is the one that needs +// guarding. Their _histogram companions must still carry the observations. +// +// Beyond the metric surface, this also keeps the command path off +// prometheus.(*summary).Observe, which serializes every observation on a +// per-metric mutex shared by all connections on the node. +func TestMetrics_NoSummariesExposed(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m, err := newMetricsRegistry(MetricsConfig{ + MetricsNamespace: "test_nosum", + RegistererGatherer: reg, + }) + require.NoError(t, err) + + m.observeCommandDuration(protocol.FrameTypePublish, 5*time.Millisecond, "", nil) + m.observeSurveyDuration("test_op", 5*time.Millisecond) + + families, err := reg.Gather() + require.NoError(t, err) + + seen := map[string]dto.MetricType{} + for _, mf := range families { + seen[mf.GetName()] = mf.GetType() + } + + for _, name := range []string{ + "test_nosum_client_command_duration_seconds", + "test_nosum_node_survey_duration_seconds", + } { + _, exists := seen[name] + require.False(t, exists, "deprecated Summary %s must no longer be exposed", name) + } + + for _, name := range []string{ + "test_nosum_client_command_duration_seconds_histogram", + "test_nosum_node_survey_duration_seconds_histogram", + } { + typ, exists := seen[name] + require.True(t, exists, "histogram %s must still be exposed", name) + require.Equal(t, dto.MetricType_HISTOGRAM, typ, "%s should be a histogram", name) + } + + // No Summary instrument should remain anywhere in the registry. + for name, typ := range seen { + require.NotEqual(t, dto.MetricType_SUMMARY, typ, + "unexpected Summary metric %s — summaries were removed", name) + } +} + func TestMetrics_EnableNativeHistograms(t *testing.T) { t.Parallel() reg := prometheus.NewRegistry() @@ -866,9 +918,8 @@ func TestMetrics_EnableNativeHistograms(t *testing.T) { families, err := reg.Gather() require.NoError(t, err) - // With EnableNativeHistograms on, the legacy Summary metrics must not - // be exposed; their _histogram companions carry the observations in - // native (sparse, exponential) form. + // The removed Summary metrics must not be exposed; their _histogram + // companions carry the observations in native (sparse, exponential) form. mustNotExist := map[string]bool{ "test_nh_client_command_duration_seconds": true, "test_nh_node_survey_duration_seconds": true, From 3073e25d783456ee4b6f93b723873368186e2daa Mon Sep 17 00:00:00 2001 From: FZambia Date: Fri, 31 Jul 2026 11:13:31 +0300 Subject: [PATCH 2/2] Refine duration histogram buckets for the latency range Centrifuge runs 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. --- metrics.go | 65 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 27 deletions(-) diff --git a/metrics.go b/metrics.go index 55a424b5..76d942f7 100644 --- a/metrics.go +++ b/metrics.go @@ -130,15 +130,15 @@ type metrics struct { commandDurationSubRefresh prometheus.Observer commandDurationUnknown prometheus.Observer - broadcastDurationHistogram *prometheus.HistogramVec - pubSubLagHistogram *prometheus.HistogramVec - pingPongDurationHistogram *prometheus.HistogramVec + broadcastDurationHistogram *prometheus.HistogramVec + pubSubLagHistogram *prometheus.HistogramVec + pingPongDurationHistogram *prometheus.HistogramVec brokerPublishSuppressedCount *prometheus.CounterVec mapBrokerPublishSuppressedCount *prometheus.CounterVec mapBrokerRemoveSuppressedCount *prometheus.CounterVec - mapBrokerCleanupLag *prometheus.GaugeVec - mapBrokerCleanupRemoved *prometheus.CounterVec - mapBrokerCleanupErrors *prometheus.CounterVec + mapBrokerCleanupLag *prometheus.GaugeVec + mapBrokerCleanupRemoved *prometheus.CounterVec + mapBrokerCleanupErrors *prometheus.CounterVec redisBrokerPubSubErrors *prometheus.CounterVec redisBrokerPubSubDroppedMessages *prometheus.CounterVec @@ -165,27 +165,27 @@ type metrics struct { config MetricsConfig - transportMessagesSentCache sync.Map - transportMessagesReceivedCache sync.Map - commandDurationCache sync.Map - replyErrorCache sync.Map - actionCache sync.Map - recoverCache sync.Map - unsubscribeCache sync.Map - disconnectCache sync.Map - messagesSentCache sync.Map - messagesReceivedCache sync.Map - tagsFilterDroppedCache sync.Map + transportMessagesSentCache sync.Map + transportMessagesReceivedCache sync.Map + commandDurationCache sync.Map + replyErrorCache sync.Map + actionCache sync.Map + recoverCache sync.Map + unsubscribeCache sync.Map + disconnectCache sync.Map + messagesSentCache sync.Map + messagesReceivedCache sync.Map + tagsFilterDroppedCache sync.Map brokerPublishSuppressedCache sync.Map mapBrokerPublishSuppressedCache sync.Map mapBrokerRemoveSuppressedCache sync.Map - pubSubLagCache sync.Map - sharedPollHandlerCache sync.Map - sharedPollResultCache sync.Map - sharedPollChannelCache sync.Map - sharedPollPublishCache sync.Map - nsCache *otter.Cache[string, string] - codeStrings map[uint32]string + pubSubLagCache sync.Map + sharedPollHandlerCache sync.Map + sharedPollResultCache sync.Map + sharedPollChannelCache sync.Map + sharedPollPublishCache sync.Map + nsCache *otter.Cache[string, string] + codeStrings map[uint32]string // Cache for client label combinations: maps cache key -> {labelValues, cacheKey} // This allows sharing pre-computed label data across all clients with the same label values @@ -482,8 +482,12 @@ func newMetricsRegistry(config MetricsConfig) (*metrics, error) { Subsystem: "node", Name: "survey_duration_seconds_histogram", Help: "Survey duration histogram. Use for histogram_quantile() and OpenTelemetry export.", + // Surveys are node-to-node round trips, so millisecond resolution is + // the right scale, but the 1ms..5ms step had the same 5x gap as the + // command histogram and is filled in for the same reason. Additive + // only - existing boundaries are unchanged. Buckets: []float64{ - 0.001, 0.005, 0.010, 0.025, 0.050, 0.100, 0.250, 0.500, + 0.001, 0.002, 0.003, 0.005, 0.010, 0.025, 0.050, 0.100, 0.250, 0.500, 1.0, 2.5, 5.0, 10.0, }, }, config.EnableNativeHistograms), []string{"op"}) @@ -507,9 +511,16 @@ func newMetricsRegistry(config MetricsConfig) (*metrics, error) { Subsystem: metricClientCommandDuration.Subsystem, Name: metricClientCommandDuration.Name + "_histogram", Help: "Client command duration histogram. Use for histogram_quantile() and OpenTelemetry export.", + // Centrifuge command latencies live in the microsecond range - a couple + // of hundred microseconds is normal - so the ladder starts at 25us + // rather than 100us, and the 1ms..5ms step is filled in. Without the + // former the median is interpolated from zero on a fast node; without + // the latter p99 lands in a 5x-wide bucket and reads far high. Every + // boundary that existed before is still here, so le= queries and + // recording rules keep working. Buckets: []float64{ - 0.000100, 0.000250, 0.000500, - 0.001, 0.005, 0.010, 0.025, 0.050, 0.100, 0.250, 0.500, + 0.000025, 0.000050, 0.000100, 0.000250, 0.000500, + 0.001, 0.002, 0.003, 0.005, 0.010, 0.025, 0.050, 0.100, 0.250, 0.500, 1.0, 2.5, 5.0, 10.0, }, }, config.EnableNativeHistograms), m.buildMetricLabels([]string{"method", "channel_namespace"}))