From 79d4fbeae75dc9f89409c1d7473efe4540f6bc6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Rold=C3=A1n=20Betancort?= Date: Wed, 15 Apr 2026 17:53:40 +0100 Subject: [PATCH] Revert "fix: add classic histogram buckets to perf insights metric (#3027)" This reverts commit 9a27f75da0d80f3902d1e66a6a81fa1cf44d484d. --- CHANGELOG.md | 5 ++-- .../middleware/perfinsights/perfinsights.go | 27 ++++++++----------- .../perfinsights/perfinsights_test.go | 21 +++++---------- 3 files changed, 20 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b61efd8f1..4a0721d28b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,11 +8,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Added support for YAML-based validation files in DevContext (https://github.com/authzed/spicedb/pull/3024) - Added support for YAML-based validation files in the Language Server (https://github.com/authzed/spicedb/pull/3024) -### Fixed -- Perf insights not producing buckets when Prometheus native histograms is disabled (https://github.com/authzed/spicedb/pull/3027) +### Changed - Removed MySQL metrics prefixed with `go_sql_stats_connections_*` in favor of those prefixed with `go_sql_*` (https://github.com/authzed/spicedb/pull/2980) - Removed support for Spanner flag value `--datastore-spanner-metrics=deprecated-prometheus`; please use values `otel` or `native` (https://github.com/authzed/spicedb/pull/2980) - Reduced binary size (https://github.com/authzed/spicedb/pull/3005) + +### Fixed - On a Postgres setup with read replicas, some requests may silently swallow errors of sort "revision not found in replica" (https://github.com/authzed/spicedb/pull/2979) - Use cgroup-aware memory detection for cache and watch buffer sizing in containerized environments (https://github.com/authzed/spicedb/pull/3000) - Upgraded the spanner client, which changed the internal implementation to not use a session pool. This means that the `--datastore-spanner-max-sessions` and `--datastore-spanner-min-sessions` flags are now deprecated and no-op. We also strongly recommend using [Application Default Credentials](https://docs.cloud.google.com/docs/authentication/client-libraries#adc) in favor of a credentials file. (https://github.com/authzed/spicedb/pull/3038) diff --git a/internal/middleware/perfinsights/perfinsights.go b/internal/middleware/perfinsights/perfinsights.go index f678835647..0eef6229b8 100644 --- a/internal/middleware/perfinsights/perfinsights.go +++ b/internal/middleware/perfinsights/perfinsights.go @@ -54,25 +54,20 @@ func NoLabels() APIShapeLabels { // of the API call that are not the specific object IDs, e.g. the resource type, // the relation, the subject type, etc. // -// NOTE: This metric uses both classic and native histogram buckets. Classic buckets -// ensure compatibility with standard PromQL queries (e.g. histogram_quantile), while -// native histograms provide higher resolution when supported by the Prometheus server. +// NOTE: This metric is a *native* histogram, which means that instead of predefined +// the buckets, it uses a factor to determine the buckets. This is useful for latency-based +// metrics, as the latency can vary widely and having a fixed set of buckets can lead to +// imprecise results. // -// To make use of native histograms, a special flag must be set on Prometheus: +// To use make use of native histograms, a special flag must be set on Prometheus: // https://prometheus.io/docs/prometheus/latest/feature_flags/#native-histograms var APIShapeLatency = promauto.NewHistogramVec(prometheus.HistogramOpts{ - Namespace: "spicedb", - Subsystem: "perf_insights", - Name: "api_shape_latency_seconds", - Help: "The latency of API calls, by shape", - NativeHistogramBucketFactor: 1.1, - NativeHistogramMaxBucketNumber: 100, - // Bucket boundaries: sub-second values match the gRPC server handling - // time histogram in pkg/cmd/server/defaults.go (createServerMetrics); - // 1–10s buckets added for slow query visibility in Perf Insights. - Buckets: []float64{ - .001, .003, .006, .010, .018, .024, .032, .042, .056, .075, .100, .178, .316, .562, 1, 2, 3, 5, 7, 10, - }, + Namespace: "spicedb", + Subsystem: "perf_insights", + Name: "api_shape_latency_seconds", + Help: "The latency of API calls, by shape", + Buckets: nil, + NativeHistogramBucketFactor: 1.1, }, append([]string{"api_kind"}, allLabels...)) var tracer = otel.Tracer("spicedb/internal/middleware") diff --git a/internal/middleware/perfinsights/perfinsights_test.go b/internal/middleware/perfinsights/perfinsights_test.go index 5456801d02..530434d300 100644 --- a/internal/middleware/perfinsights/perfinsights_test.go +++ b/internal/middleware/perfinsights/perfinsights_test.go @@ -67,15 +67,12 @@ func TestObserveShapeLatency(t *testing.T) { reg := prometheus.NewRegistry() metric := prometheus.NewHistogramVec(prometheus.HistogramOpts{ - Namespace: "spicedb", - Subsystem: "perf_insights", - Name: "api_shape_latency_seconds", - Help: "The latency of API calls, by shape", - NativeHistogramBucketFactor: 1.1, - NativeHistogramMaxBucketNumber: 100, - Buckets: []float64{ - .001, .003, .006, .010, .018, .024, .032, .042, .056, .075, .100, .178, .316, .562, 1, 2, 3, 5, 7, 10, - }, + Namespace: "spicedb", + Subsystem: "perf_insights", + Name: "api_shape_latency_seconds", + Help: "The latency of API calls, by shape", + Buckets: nil, + NativeHistogramBucketFactor: 1.1, }, append([]string{"api_kind"}, allLabels...)) require.NoError(t, reg.Register(metric)) @@ -102,12 +99,6 @@ func TestObserveShapeLatency(t *testing.T) { require.Equal(t, float64(0.1), metric.GetMetric()[0].Histogram.GetSampleSum()) //nolint:testifylint // this value is set directly by test code require.Equal(t, "testMethod", metric.GetMetric()[0].Label[0].GetValue()) - // Verify classic buckets are populated (not just +Inf). - buckets := metric.GetMetric()[0].Histogram.GetBucket() - require.Len(t, buckets, 20) - require.InDelta(t, 0.001, buckets[0].GetUpperBound(), 1e-9) - require.InDelta(t, float64(10), buckets[len(buckets)-1].GetUpperBound(), 1e-9) - for _, label := range metric.GetMetric()[0].Label { if label.GetName() == "api_kind" { require.Equal(t, "testMethod", label.GetValue())