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..76d942f7 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 @@ -136,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 @@ -171,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 @@ -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,24 +477,17 @@ 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", 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"}) @@ -563,25 +506,21 @@ 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, 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"})) @@ -955,10 +894,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 +920,6 @@ func newMetricsRegistry(config MetricsConfig) (*metrics, error) { m.numSubsGauge, m.numChannelsGauge, m.numNodesGauge, - m.commandDurationSummary, m.commandDurationHistogram, m.replyErrorCount, m.connectionsAccepted, @@ -1002,7 +937,6 @@ func newMetricsRegistry(config MetricsConfig) (*metrics, error) { m.transportMessagesReceivedSize, m.tagsFilterDroppedCount, m.buildInfoGauge, - m.surveyDurationSummary, m.surveyDurationHistogram, m.pubSubLagHistogram, m.broadcastDurationHistogram, @@ -1098,10 +1032,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 +1437,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,