diff --git a/cmd/spicedb/memoryprotection/memory_protection_integration_test.go b/cmd/spicedb/memoryprotection/memory_protection_integration_test.go index 0dc1230b77..0a54175e12 100644 --- a/cmd/spicedb/memoryprotection/memory_protection_integration_test.go +++ b/cmd/spicedb/memoryprotection/memory_protection_integration_test.go @@ -33,9 +33,18 @@ func TestServeWithMemoryProtectionMiddleware(t *testing.T) { serverToken := "mykey" serveResource, err := pool.RunWithOptions(&dockertest.RunOptions{ - Repository: "authzed/spicedb", - Tag: "ci", - Cmd: []string{"serve", "--log-level=debug", "--grpc-preshared-key", serverToken, "--telemetry-endpoint=\"\""}, + Repository: "authzed/spicedb", + Tag: "ci", + Cmd: []string{ + "serve", + "--log-level=debug", + "--grpc-preshared-key", serverToken, + "--telemetry-endpoint", "", + // With very low GOMEMLIMIT values, percentage-based dispatch cache defaults + // can round down to zero and fail startup. + "--dispatch-cache-max-cost", "8KiB", + "--dispatch-cluster-cache-max-cost", "8KiB", + }, ExposedPorts: []string{"50051/tcp"}, Env: []string{"GOMEMLIMIT=1B"}, // NOTE: Absurdly low on purpose }, func(config *docker.HostConfig) { diff --git a/internal/datastore/crdb/crdb.go b/internal/datastore/crdb/crdb.go index 842e3d69e1..1bd7eb56fe 100644 --- a/internal/datastore/crdb/crdb.go +++ b/internal/datastore/crdb/crdb.go @@ -29,6 +29,7 @@ import ( pgxcommon "github.com/authzed/spicedb/internal/datastore/postgres/common" "github.com/authzed/spicedb/internal/datastore/revisions" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/internal/sharederrors" "github.com/authzed/spicedb/pkg/datastore" "github.com/authzed/spicedb/pkg/datastore/options" @@ -72,6 +73,8 @@ func newCRDBDatastore(ctx context.Context, url string, options ...Option) (datas } includeQueryParametersInTraces := config.includeQueryParametersInTraces + pool.SetMetricsFactory(internalmetrics.NewPrometheusFactory(config.prometheusRegisterer)) + readPoolConfig, err := pgxpool.ParseConfig(url) if err != nil { return nil, common.RedactAndLogSensitiveConnString(ctx, errUnableToInstantiate, err, url) @@ -213,7 +216,7 @@ func newCRDBDatastore(ctx context.Context, url string, options ...Option) (datas return nil, common.RedactAndLogSensitiveConnString(ctx, errUnableToInstantiate, err, url) } - err = ds.registerPrometheusCollectors(config.enablePrometheusStats) + err = ds.registerPrometheusCollectors(config.enablePrometheusStats, config.prometheusRegisterer) if err != nil { ds.cancel() return nil, err @@ -264,6 +267,7 @@ type crdbDatastore struct { dburl string readPool, writePool *pool.RetryPool collectors []prometheus.Collector + prometheusRegisterer prometheus.Registerer watchBufferLength uint16 watchChangeBufferMaximumSize uint64 watchBufferWriteTimeout time.Duration @@ -481,9 +485,12 @@ func (cds *crdbDatastore) Close() error { } cds.readPool.Close() cds.writePool.Close() + r := cds.prometheusRegisterer + if r == nil { + r = prometheus.DefaultRegisterer + } for _, collector := range cds.collectors { - ok := prometheus.Unregister(collector) - if !ok { + if ok := r.Unregister(collector); !ok { errs = append(errs, errors.New("could not unregister collector for CRDB datastore")) } } @@ -658,7 +665,11 @@ func readClusterTTLNanos(ctx context.Context, conn pgxcommon.DBFuncQuerier) (int return gcSeconds * 1_000_000_000, nil } -func (cds *crdbDatastore) registerPrometheusCollectors(enablePrometheusStats bool) error { +func (cds *crdbDatastore) registerPrometheusCollectors(enablePrometheusStats bool, registerer prometheus.Registerer) error { + if registerer == nil { + registerer = prometheus.DefaultRegisterer + } + cds.prometheusRegisterer = registerer if !enablePrometheusStats { return nil } @@ -667,21 +678,21 @@ func (cds *crdbDatastore) registerPrometheusCollectors(enablePrometheusStats boo "db_name": "spicedb", "pool_usage": "read", }) - - if err := prometheus.Register(readCollector); err != nil { + registered, err := internalmetrics.RegisterOrReuse(registerer, readCollector) + if err != nil { return fmt.Errorf("failed to register prometheus read collector: %w", err) } - cds.collectors = append(cds.collectors, readCollector) + cds.collectors = append(cds.collectors, registered) writeCollector := pgxpoolprometheus.NewCollector(cds.writePool, map[string]string{ "db_name": "spicedb", "pool_usage": "write", }) - - if err := prometheus.Register(writeCollector); err != nil { + registeredWrite, err := internalmetrics.RegisterOrReuse(registerer, writeCollector) + if err != nil { return fmt.Errorf("failed to register prometheus write collector: %w", err) } - cds.collectors = append(cds.collectors, writeCollector) + cds.collectors = append(cds.collectors, registeredWrite) return nil } diff --git a/internal/datastore/crdb/crdb_test.go b/internal/datastore/crdb/crdb_test.go index 69a2f9f4c7..9151242912 100644 --- a/internal/datastore/crdb/crdb_test.go +++ b/internal/datastore/crdb/crdb_test.go @@ -913,12 +913,12 @@ func TestRegisterPrometheusCollectors(t *testing.T) { _ = cds.Close() }) - err = cds.registerPrometheusCollectors(false) + err = cds.registerPrometheusCollectors(false, nil) require.NoError(t, err) require.Empty(t, cds.collectors) // Register collectors and verify the values of the metrics - err = cds.registerPrometheusCollectors(true) + err = cds.registerPrometheusCollectors(true, nil) require.NoError(t, err) require.Len(t, cds.collectors, 2) diff --git a/internal/datastore/crdb/options.go b/internal/datastore/crdb/options.go index 9e745fc213..8db8f91f73 100644 --- a/internal/datastore/crdb/options.go +++ b/internal/datastore/crdb/options.go @@ -4,6 +4,7 @@ import ( "fmt" "time" + "github.com/prometheus/client_golang/prometheus" "k8s.io/utils/ptr" "github.com/authzed/spicedb/internal/datastore/common" @@ -30,6 +31,7 @@ type crdbOptions struct { analyzeBeforeStatistics bool filterMaximumIDCount uint16 enablePrometheusStats bool + prometheusRegisterer prometheus.Registerer withIntegrity bool allowedMigrations []string columnOptimizationOption common.ColumnOptimizationOption @@ -345,6 +347,13 @@ func WithEnablePrometheusStats(enablePrometheusStats bool) Option { return func(po *crdbOptions) { po.enablePrometheusStats = enablePrometheusStats } } +// WithPrometheusRegisterer sets the prometheus.Registerer used to register +// datastore metrics. When not set (or nil), prometheus.DefaultRegisterer is +// used. +func WithPrometheusRegisterer(r prometheus.Registerer) Option { + return func(po *crdbOptions) { po.prometheusRegisterer = r } +} + // WithEnableConnectionBalancing marks whether Prometheus metrics provided by the Postgres // clients being used by the datastore are enabled. // diff --git a/internal/datastore/crdb/pool/balancer.go b/internal/datastore/crdb/pool/balancer.go index dbbbd23d27..c0cde6bb39 100644 --- a/internal/datastore/crdb/pool/balancer.go +++ b/internal/datastore/crdb/pool/balancer.go @@ -13,29 +13,40 @@ import ( "github.com/ccoveille/go-safecast/v2" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgxpool" - "github.com/prometheus/client_golang/prometheus" "golang.org/x/sync/semaphore" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/genutil" ) var ( - connectionsPerCRDBNodeCountGauge = prometheus.NewGaugeVec(prometheus.GaugeOpts{ + connectionsPerCRDBNodeCountGauge internalmetrics.GaugeVec + pruningTimeHistogram internalmetrics.HistogramVec +) + +var ( + connectionsGaugeOpts = internalmetrics.Opts{ Name: "crdb_connections_per_node", Help: "The number of active connections SpiceDB holds to each CockroachDB node, by pool (read/write). Imbalanced values across nodes suggest the connection balancer is unable to redistribute connections evenly.", - }, []string{"pool", "node_id"}) - - pruningTimeHistogram = prometheus.NewHistogramVec(prometheus.HistogramOpts{ + } + pruningHistogramOpts = internalmetrics.Opts{ Name: "crdb_pruning_duration", Help: "Duration in milliseconds of one iteration of the CockroachDB connection balancer pruning excess connections from over-represented nodes. Elevated values indicate the balancer is struggling to rebalance connections.", Buckets: []float64{.1, .2, .5, 1, 2, 5, 10, 20, 50, 100}, - }, []string{"pool"}) + } ) func init() { - prometheus.MustRegister(connectionsPerCRDBNodeCountGauge) - prometheus.MustRegister(pruningTimeHistogram) + setBalancerMetricsFactory(internalmetrics.NewPrometheusFactory(nil)) +} + +func setBalancerMetricsFactory(factory internalmetrics.Factory) { + if factory == nil { + factory = internalmetrics.NoopFactory{} + } + connectionsPerCRDBNodeCountGauge = factory.GaugeVec(connectionsGaugeOpts, []string{"pool", "node_id"}) + pruningTimeHistogram = factory.HistogramVec(pruningHistogramOpts, []string{"pool"}) } type balancePoolConn[C balanceConn] interface { @@ -182,10 +193,12 @@ func (p *nodeConnectionBalancer[P, C]) mustPruneConnections(ctx context.Context) p.healthTracker.RLock() for node := range p.healthTracker.nodesEverSeen { if _, ok := connectionCounts[node]; !ok { - connectionsPerCRDBNodeCountGauge.DeletePartialMatch(map[string]string{ - "pool": p.pool.ID(), - "node_id": strconv.FormatUint(uint64(node), 10), - }) + if pgv, ok := internalmetrics.AsPrometheusGaugeVec(connectionsPerCRDBNodeCountGauge); ok { + pgv.DeletePartialMatch(map[string]string{ + "pool": p.pool.ID(), + "node_id": strconv.FormatUint(uint64(node), 10), + }) + } } } p.healthTracker.RUnlock() diff --git a/internal/datastore/crdb/pool/health.go b/internal/datastore/crdb/pool/health.go index d1c511b94d..b9bfdf3505 100644 --- a/internal/datastore/crdb/pool/health.go +++ b/internal/datastore/crdb/pool/health.go @@ -7,24 +7,33 @@ import ( "time" "github.com/jackc/pgx/v5" - "github.com/prometheus/client_golang/prometheus" "golang.org/x/time/rate" "github.com/authzed/jitterbug" pgxcommon "github.com/authzed/spicedb/internal/datastore/postgres/common" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" ) const errorBurst = 2 -var healthyCRDBNodeCountGauge = prometheus.NewGauge(prometheus.GaugeOpts{ +var healthyCRDBNodeCountGauge internalmetrics.Gauge + +var healthyNodesGaugeOpts = internalmetrics.Opts{ Name: "crdb_healthy_nodes", Help: "the number of healthy crdb nodes detected by spicedb", -}) +} func init() { - prometheus.MustRegister(healthyCRDBNodeCountGauge) + setHealthMetricsFactory(internalmetrics.NewPrometheusFactory(nil)) +} + +func setHealthMetricsFactory(factory internalmetrics.Factory) { + if factory == nil { + factory = internalmetrics.NoopFactory{} + } + healthyCRDBNodeCountGauge = factory.Gauge(healthyNodesGaugeOpts) } // NodeHealthTracker detects changes in the node pool by polling the cluster periodically and recording diff --git a/internal/datastore/crdb/pool/metrics.go b/internal/datastore/crdb/pool/metrics.go new file mode 100644 index 0000000000..7424342641 --- /dev/null +++ b/internal/datastore/crdb/pool/metrics.go @@ -0,0 +1,10 @@ +package pool + +import internalmetrics "github.com/authzed/spicedb/internal/metrics" + +// SetMetricsFactory configures which metrics factory is used by this package. +func SetMetricsFactory(factory internalmetrics.Factory) { + setBalancerMetricsFactory(factory) + setHealthMetricsFactory(factory) + setPoolMetricsFactory(factory) +} diff --git a/internal/datastore/crdb/pool/metrics_test.go b/internal/datastore/crdb/pool/metrics_test.go new file mode 100644 index 0000000000..a099da5a59 --- /dev/null +++ b/internal/datastore/crdb/pool/metrics_test.go @@ -0,0 +1,47 @@ +package pool + +import ( + "testing" + + "github.com/stretchr/testify/require" + + internalmetrics "github.com/authzed/spicedb/internal/metrics" +) + +func TestSetMetricsFactoryAppliesToAllPoolMetrics(t *testing.T) { + recording := internalmetrics.NewRecordingFactory() + SetMetricsFactory(recording) + t.Cleanup(func() { + SetMetricsFactory(internalmetrics.NewPrometheusFactory(nil)) + }) + + resetHistogram.Observe(1) + healthyCRDBNodeCountGauge.Set(2) + connectionsPerCRDBNodeCountGauge.WithLabelValues("read", "1").Set(3) + pruningTimeHistogram.WithLabelValues("read").Observe(4) + + require.Equal(t, uint64(1), recording.HistogramCount("", "", "crdb_client_resets")) + require.InEpsilon(t, 2.0, recording.GaugeValue("", "", "crdb_healthy_nodes"), 1e-9) + require.InEpsilon(t, 3.0, recording.GaugeVecValue("", "", "crdb_connections_per_node", "read", "1"), 1e-9) + require.Equal(t, uint64(1), recording.HistogramVecCount("", "", "crdb_pruning_duration", "read")) +} + +func TestSetMetricsFactoryWithNilAndNoopPool(t *testing.T) { + SetMetricsFactory(internalmetrics.NoopFactory{}) + require.NotPanics(t, func() { + resetHistogram.Observe(1) + healthyCRDBNodeCountGauge.Set(1) + connectionsPerCRDBNodeCountGauge.WithLabelValues("read", "1").Set(1) + pruningTimeHistogram.WithLabelValues("read").Observe(1) + }) + + SetMetricsFactory(nil) + require.NotPanics(t, func() { + resetHistogram.Observe(1) + healthyCRDBNodeCountGauge.Set(1) + connectionsPerCRDBNodeCountGauge.WithLabelValues("read", "1").Set(1) + pruningTimeHistogram.WithLabelValues("read").Observe(1) + }) + + SetMetricsFactory(internalmetrics.NewPrometheusFactory(nil)) +} diff --git a/internal/datastore/crdb/pool/pool.go b/internal/datastore/crdb/pool/pool.go index 766de31e9a..b3504c8dfd 100644 --- a/internal/datastore/crdb/pool/pool.go +++ b/internal/datastore/crdb/pool/pool.go @@ -11,11 +11,11 @@ import ( "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" "github.com/jackc/pgx/v5/pgxpool" - "github.com/prometheus/client_golang/prometheus" "golang.org/x/time/rate" "github.com/authzed/spicedb/internal/datastore/postgres/common" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/spiceerrors" ) @@ -28,14 +28,23 @@ type pgxPool interface { Stat() *pgxpool.Stat } -var resetHistogram = prometheus.NewHistogram(prometheus.HistogramOpts{ +var resetHistogram internalmetrics.Histogram + +var resetHistogramOpts = internalmetrics.Opts{ Name: "crdb_client_resets", Help: "Distribution of the number of client-side transaction restarts per transaction attempt. Restarts occur when CockroachDB returns a serialization failure (40001) and the driver retries the transaction from scratch. Sustained high values indicate transaction contention.", Buckets: []float64{0, 1, 2, 5, 10, 20, 50}, -}) +} func init() { - prometheus.MustRegister(resetHistogram) + setPoolMetricsFactory(internalmetrics.NewPrometheusFactory(nil)) +} + +func setPoolMetricsFactory(factory internalmetrics.Factory) { + if factory == nil { + factory = internalmetrics.NoopFactory{} + } + resetHistogram = factory.Histogram(resetHistogramOpts) } type ctxDisableRetries struct{} diff --git a/internal/datastore/mysql/connection.go b/internal/datastore/mysql/connection.go index d986afdbe0..6501ddc5ef 100644 --- a/internal/datastore/mysql/connection.go +++ b/internal/datastore/mysql/connection.go @@ -10,6 +10,7 @@ import ( "github.com/prometheus/client_golang/prometheus" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" ) // instrumentedConnector wraps the default MySQL driver connector @@ -45,7 +46,16 @@ func (d *instrumentedConnector) Driver() driver.Driver { return d.drv } -func instrumentConnector(c driver.Connector, replicaIndex string) (driver.Connector, []prometheus.Collector, error) { +// instrumentConnector wraps c to collect per-connection latency and count +// metrics and registers them with registerer. +// +// If registerer is nil, prometheus.DefaultRegisterer is used. +// AlreadyRegisteredError is tolerated so that a primary and replica connector +// can be instrumented in the same process without conflicting. +func instrumentConnector(c driver.Connector, replicaIndex string, registerer prometheus.Registerer) (driver.Connector, []prometheus.Collector, error) { + if registerer == nil { + registerer = prometheus.DefaultRegisterer + } var ( connectHistogram = prometheus.NewHistogram(prometheus.HistogramOpts{ Namespace: "spicedb", @@ -70,24 +80,35 @@ func instrumentConnector(c driver.Connector, replicaIndex string) (driver.Connec var collectors []prometheus.Collector - err := prometheus.Register(connectHistogram) + registered, err := internalmetrics.RegisterOrReuse(registerer, connectHistogram) if err != nil { return nil, collectors, err } + collectors = append(collectors, registered) - collectors = append(collectors, connectHistogram) - err = prometheus.Register(connectCount) + registeredVec, err := internalmetrics.RegisterOrReuse(registerer, connectCount) if err != nil { return nil, collectors, err } - - collectors = append(collectors, connectCount) + collectors = append(collectors, registeredVec) + + // The histogram and counter vec we use in the connector must be the + // registered instances (which may differ from our local vars when + // AlreadyRegisteredError was returned). + actualHistogram, ok := registered.(prometheus.Histogram) + if !ok { + actualHistogram = connectHistogram + } + actualCounter, ok2 := registeredVec.(*prometheus.CounterVec) + if !ok2 { + actualCounter = connectCount + } return &instrumentedConnector{ conn: c, drv: c.Driver(), - connectHistogram: connectHistogram, - connectCount: connectCount, + connectHistogram: actualHistogram, + connectCount: actualCounter, }, collectors, nil } diff --git a/internal/datastore/mysql/connection_test.go b/internal/datastore/mysql/connection_test.go new file mode 100644 index 0000000000..bf052fe638 --- /dev/null +++ b/internal/datastore/mysql/connection_test.go @@ -0,0 +1,72 @@ +package mysql + +import ( + "context" + "database/sql/driver" + "errors" + "testing" + + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" +) + +func TestInstrumentConnectorRepeatedRegistration(t *testing.T) { + registry := prometheus.NewRegistry() + base := fakeDriverConnector{} + + firstConnector, firstCollectors, err := instrumentConnector(base, "0", registry) + require.NoError(t, err) + require.Len(t, firstCollectors, 2) + + secondConnector, secondCollectors, err := instrumentConnector(base, "0", registry) + require.NoError(t, err) + require.Len(t, secondCollectors, 2) + + for index := range firstCollectors { + require.Same(t, firstCollectors[index], secondCollectors[index]) + } + + firstInstrumented, ok := firstConnector.(*instrumentedConnector) + require.True(t, ok) + secondInstrumented, ok := secondConnector.(*instrumentedConnector) + require.True(t, ok) + + require.Same(t, firstInstrumented.connectCount, secondInstrumented.connectCount) +} + +func TestInstrumentConnectorNilRegistererDefaultsAndReuses(t *testing.T) { + originalRegisterer := prometheus.DefaultRegisterer + originalGatherer := prometheus.DefaultGatherer + registry := prometheus.NewRegistry() + prometheus.DefaultRegisterer = registry + prometheus.DefaultGatherer = registry + t.Cleanup(func() { + prometheus.DefaultRegisterer = originalRegisterer + prometheus.DefaultGatherer = originalGatherer + }) + + _, firstCollectors, err := instrumentConnector(fakeDriverConnector{}, "1", nil) + require.NoError(t, err) + _, secondCollectors, err := instrumentConnector(fakeDriverConnector{}, "1", nil) + require.NoError(t, err) + + for index := range firstCollectors { + require.Same(t, firstCollectors[index], secondCollectors[index]) + } +} + +type fakeDriverConnector struct{} + +func (fakeDriverConnector) Connect(context.Context) (driver.Conn, error) { + return nil, errors.New("not implemented") +} + +func (fakeDriverConnector) Driver() driver.Driver { + return fakeDriver{} +} + +type fakeDriver struct{} + +func (fakeDriver) Open(string) (driver.Conn, error) { + return nil, errors.New("not implemented") +} diff --git a/internal/datastore/mysql/datastore.go b/internal/datastore/mysql/datastore.go index ec66276e04..d8fad4ca45 100644 --- a/internal/datastore/mysql/datastore.go +++ b/internal/datastore/mysql/datastore.go @@ -24,6 +24,7 @@ import ( "github.com/authzed/spicedb/internal/datastore/mysql/migrations" "github.com/authzed/spicedb/internal/datastore/revisions" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/internal/sharederrors" "github.com/authzed/spicedb/pkg/datastore" "github.com/authzed/spicedb/pkg/datastore/options" @@ -174,7 +175,7 @@ func newMySQLDatastore(ctx context.Context, uri string, replicaIndex int, option } } - db, collectors, err := registerAndReturnPrometheusCollectors(replicaIndex, isPrimary, connector, config.enablePrometheusStats) + db, collectors, err := registerAndReturnPrometheusCollectors(replicaIndex, isPrimary, connector, config.enablePrometheusStats, config.prometheusRegisterer) if err != nil { for _, collector := range collectors { _ = prometheus.Unregister(collector) @@ -252,6 +253,7 @@ func newMySQLDatastore(ctx context.Context, uri string, replicaIndex int, option db: db, driver: driver, collectors: collectors, + prometheusRegisterer: config.prometheusRegisterer, url: uri, revisionQuantization: config.revisionQuantization, gcWindow: config.gcWindow, @@ -472,12 +474,13 @@ type mysqlDatastore struct { *revisions.CachedOptimizedRevisions *common.MigrationValidator - db *sql.DB - driver *migrations.MySQLDriver - readTxOptions *sql.TxOptions - url string - analyzeBeforeStats bool - collectors []prometheus.Collector + db *sql.DB + driver *migrations.MySQLDriver + readTxOptions *sql.TxOptions + url string + analyzeBeforeStats bool + collectors []prometheus.Collector + prometheusRegisterer prometheus.Registerer revisionQuantization time.Duration gcWindow time.Duration @@ -517,8 +520,12 @@ func (mds *mysqlDatastore) Close() error { log.Error().Err(err).Msg("error waiting for garbage collector to shutdown") } } + r := mds.prometheusRegisterer + if r == nil { + r = prometheus.DefaultRegisterer + } for _, collector := range mds.collectors { - _ = prometheus.Unregister(collector) + _ = r.Unregister(collector) } return mds.db.Close() } @@ -685,12 +692,15 @@ func (debugLogger) Print(v ...any) { log.Logger.Debug().CallerSkipFrame(1).Str("datastore", "mysql").Msg(fmt.Sprint(v...)) } -func registerAndReturnPrometheusCollectors(replicaIndex int, isPrimary bool, connector driver.Connector, enablePrometheusStats bool) (*sql.DB, []prometheus.Collector, error) { +func registerAndReturnPrometheusCollectors(replicaIndex int, isPrimary bool, connector driver.Connector, enablePrometheusStats bool, registerer prometheus.Registerer) (*sql.DB, []prometheus.Collector, error) { + if registerer == nil { + registerer = prometheus.DefaultRegisterer + } if !enablePrometheusStats { return sql.OpenDB(connector), nil, nil } - connector, collectors, err := instrumentConnector(connector, strconv.Itoa(replicaIndex)) + connector, collectors, err := instrumentConnector(connector, strconv.Itoa(replicaIndex), registerer) if err != nil { return nil, collectors, err } @@ -702,13 +712,14 @@ func registerAndReturnPrometheusCollectors(replicaIndex int, isPrimary bool, con db := sql.OpenDB(connector) collector := prom_collectors.NewDBStatsCollector(db, dbName) - if err := prometheus.Register(collector); err != nil { + registered, err := internalmetrics.RegisterOrReuse(registerer, collector) + if err != nil { return nil, collectors, err } - collectors = append(collectors, collector) + collectors = append(collectors, registered) if isPrimary { - gcMetrics, err := datastore.RegisterGCMetrics() + gcMetrics, err := datastore.RegisterGCMetrics(registerer) if err != nil { return nil, collectors, err } diff --git a/internal/datastore/mysql/options.go b/internal/datastore/mysql/options.go index 63f6e195c8..ec4b2fc68e 100644 --- a/internal/datastore/mysql/options.go +++ b/internal/datastore/mysql/options.go @@ -4,6 +4,8 @@ import ( "fmt" "time" + "github.com/prometheus/client_golang/prometheus" + "github.com/authzed/spicedb/internal/datastore/common" log "github.com/authzed/spicedb/internal/logging" ) @@ -44,6 +46,7 @@ type mysqlOptions struct { watchBufferWriteTimeout time.Duration tablePrefix string enablePrometheusStats bool + prometheusRegisterer prometheus.Registerer maxOpenConns int connMaxIdleTime time.Duration connMaxLifetime time.Duration @@ -204,6 +207,17 @@ func WithEnablePrometheusStats(enablePrometheusStats bool) Option { } } +// WithPrometheusRegisterer sets the prometheus.Registerer used to register +// datastore metrics. When not set (or nil), prometheus.DefaultRegisterer is +// used. Inject a per-test or per-server registry to prevent duplicate- +// metric registration errors when multiple datastore instances share a +// process. +func WithPrometheusRegisterer(r prometheus.Registerer) Option { + return func(mo *mysqlOptions) { + mo.prometheusRegisterer = r + } +} + // ConnMaxIdleTime is the duration after which an idle connection will be // automatically closed. // See https://pkg.go.dev/database/sql#DB.SetConnMaxIdleTime/ diff --git a/internal/datastore/postgres/options.go b/internal/datastore/postgres/options.go index 1e2e1a812a..863673fafa 100644 --- a/internal/datastore/postgres/options.go +++ b/internal/datastore/postgres/options.go @@ -4,6 +4,8 @@ import ( "fmt" "time" + "github.com/prometheus/client_golang/prometheus" + "github.com/authzed/spicedb/internal/datastore/common" pgxcommon "github.com/authzed/spicedb/internal/datastore/postgres/common" log "github.com/authzed/spicedb/internal/logging" @@ -28,6 +30,7 @@ type postgresOptions struct { filterMaximumIDCount uint16 enablePrometheusStats bool + prometheusRegisterer prometheus.Registerer analyzeBeforeStatistics bool gcEnabled bool readStrictMode bool @@ -353,6 +356,13 @@ func WithEnablePrometheusStats(enablePrometheusStats bool) Option { return func(po *postgresOptions) { po.enablePrometheusStats = enablePrometheusStats } } +// WithPrometheusRegisterer sets the prometheus.Registerer used to register +// datastore metrics. When not set (or nil), prometheus.DefaultRegisterer is +// used. +func WithPrometheusRegisterer(r prometheus.Registerer) Option { + return func(po *postgresOptions) { po.prometheusRegisterer = r } +} + // EnableTracing enables trace-level logging for the Postgres clients being // used by the datastore. // diff --git a/internal/datastore/postgres/postgres.go b/internal/datastore/postgres/postgres.go index 5b1e8c018c..e3b22fd1d4 100644 --- a/internal/datastore/postgres/postgres.go +++ b/internal/datastore/postgres/postgres.go @@ -31,6 +31,7 @@ import ( "github.com/authzed/spicedb/internal/datastore/postgres/schema" "github.com/authzed/spicedb/internal/datastore/revisions" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/internal/sharederrors" "github.com/authzed/spicedb/pkg/datastore" "github.com/authzed/spicedb/pkg/datastore/options" @@ -240,7 +241,7 @@ func newPostgresDatastore( } } - collectors, err := registerAndReturnPrometheusCollectors(replicaIndex, isPrimary, readPool, writePool, config.enablePrometheusStats) + collectors, err := registerAndReturnPrometheusCollectors(replicaIndex, isPrimary, readPool, writePool, config.enablePrometheusStats, config.prometheusRegisterer) if err != nil { return nil, err } @@ -306,6 +307,7 @@ func newPostgresDatastore( readPool: pgxcommon.MustNewInterceptorPooler(readPool, config.queryInterceptor), writePool: nil, /* disabled by default */ collectors: collectors, + prometheusRegisterer: config.prometheusRegisterer, watchBufferLength: config.watchBufferLength, watchChangeBufferMaximumSize: config.watchChangeBufferMaximumSize, watchBufferWriteTimeout: config.watchBufferWriteTimeout, @@ -374,6 +376,7 @@ type pgDatastore struct { dburl string readPool, writePool pgxcommon.ConnPooler collectors []prometheus.Collector + prometheusRegisterer prometheus.Registerer watchBufferLength uint16 watchChangeBufferMaximumSize uint64 watchBufferWriteTimeout time.Duration @@ -647,7 +650,11 @@ func (pgd *pgDatastore) Close() error { pgd.writePool.Close() } for _, collector := range pgd.collectors { - prometheus.Unregister(collector) + r := pgd.prometheusRegisterer + if r == nil { + r = prometheus.DefaultRegisterer + } + r.Unregister(collector) } return nil } @@ -801,7 +808,10 @@ func currentlyLivingObjects(original sq.SelectBuilder) sq.SelectBuilder { var _ datastore.Datastore = &pgDatastore{} -func registerAndReturnPrometheusCollectors(replicaIndex int, isPrimary bool, readPool, writePool *pgxpool.Pool, enablePrometheusStats bool) ([]prometheus.Collector, error) { +func registerAndReturnPrometheusCollectors(replicaIndex int, isPrimary bool, readPool, writePool *pgxpool.Pool, enablePrometheusStats bool, registerer prometheus.Registerer) ([]prometheus.Collector, error) { + if registerer == nil { + registerer = prometheus.DefaultRegisterer + } collectors := []prometheus.Collector{} if !enablePrometheusStats { return collectors, nil @@ -817,23 +827,24 @@ func registerAndReturnPrometheusCollectors(replicaIndex int, isPrimary bool, rea "db_name": dbname, "pool_usage": "read", }) - if err := prometheus.Register(readCollector); err != nil { + registered, err := internalmetrics.RegisterOrReuse(registerer, readCollector) + if err != nil { return collectors, err } - collectors = append(collectors, readCollector) + collectors = append(collectors, registered) if isPrimary { writeCollector := pgxpoolprometheus.NewCollector(writePool, map[string]string{ "db_name": "spicedb", "pool_usage": "write", }) - - if err := prometheus.Register(writeCollector); err != nil { - return collectors, nil + registeredWrite, err := internalmetrics.RegisterOrReuse(registerer, writeCollector) + if err != nil { + return collectors, err } - collectors = append(collectors, writeCollector) + collectors = append(collectors, registeredWrite) - gcCollectors, err := datastore.RegisterGCMetrics() + gcCollectors, err := datastore.RegisterGCMetrics(registerer) if err != nil { return collectors, err } diff --git a/internal/datastore/proxy/checkingreplicated.go b/internal/datastore/proxy/checkingreplicated.go index 39fe7c79c9..90778f0882 100644 --- a/internal/datastore/proxy/checkingreplicated.go +++ b/internal/datastore/proxy/checkingreplicated.go @@ -6,38 +6,55 @@ import ( "sync" "sync/atomic" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" - log "github.com/authzed/spicedb/internal/logging" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/datastore" "github.com/authzed/spicedb/pkg/datastore/options" core "github.com/authzed/spicedb/pkg/proto/core/v1" ) var ( - checkingReplicatedTotalReaderCount = promauto.NewCounter(prometheus.CounterOpts{ + checkingReplicatedTotalReaderCount metrics.Counter + + checkingReplicatedTotalReaderCountOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "datastore_replica", Name: "checking_replicated_reader_total", Help: "total number of readers created by the checking replica proxy", - }) + } - checkingReplicatedReplicaReaderCount = promauto.NewCounterVec(prometheus.CounterOpts{ + checkingReplicatedReplicaReaderCount metrics.CounterVec + + checkingReplicatedReplicaReaderCountOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "datastore_replica", Name: "checking_replicated_replica_reader_total", Help: "number of readers created by the checking replica proxy that are using the replica", - }, []string{"replica"}) + } + + readReplicatedSelectedReplicaCount metrics.CounterVec - readReplicatedSelectedReplicaCount = promauto.NewCounterVec(prometheus.CounterOpts{ + readReplicatedSelectedReplicaCountOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "datastore_replica", Name: "selected_replica_total", Help: "the selected replica in a read replicated datastore", - }, []string{"replica"}) + } ) +func init() { + setCheckingReplicatedMetricsFactory(metrics.NewPrometheusFactory(nil)) +} + +func setCheckingReplicatedMetricsFactory(factory metrics.Factory) { + if factory == nil { + factory = metrics.NoopFactory{} + } + checkingReplicatedTotalReaderCount = factory.Counter(checkingReplicatedTotalReaderCountOpts) + checkingReplicatedReplicaReaderCount = factory.CounterVec(checkingReplicatedReplicaReaderCountOpts, []string{"replica"}) + readReplicatedSelectedReplicaCount = factory.CounterVec(readReplicatedSelectedReplicaCountOpts, []string{"replica"}) +} + // NewCheckingReplicatedDatastore creates a new datastore that writes to the provided primary and reads // from the provided replicas. The replicas are chosen in a round-robin fashion. If a replica does // not have the requested revision, the primary is used instead. diff --git a/internal/datastore/proxy/metrics.go b/internal/datastore/proxy/metrics.go new file mode 100644 index 0000000000..cc61db7c92 --- /dev/null +++ b/internal/datastore/proxy/metrics.go @@ -0,0 +1,10 @@ +package proxy + +import "github.com/authzed/spicedb/internal/metrics" + +// SetMetricsFactory configures which metrics factory is used by this package. +func SetMetricsFactory(factory metrics.Factory) { + setObservableMetricsFactory(factory) + setCheckingReplicatedMetricsFactory(factory) + setStrictReplicatedMetricsFactory(factory) +} diff --git a/internal/datastore/proxy/metrics_test.go b/internal/datastore/proxy/metrics_test.go new file mode 100644 index 0000000000..e6cec87dda --- /dev/null +++ b/internal/datastore/proxy/metrics_test.go @@ -0,0 +1,61 @@ +package proxy + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/authzed/spicedb/internal/metrics" +) + +func TestSetMetricsFactoryAppliesToAllProxyMetrics(t *testing.T) { + recording := metrics.NewRecordingFactory() + SetMetricsFactory(recording) + t.Cleanup(func() { + SetMetricsFactory(metrics.NewPrometheusFactory(nil)) + }) + + loadedRelationshipCount.Observe(3) + queryLatency.WithLabelValues("op", "shape").Observe(0.1) + + checkingReplicatedTotalReaderCount.Add(2) + checkingReplicatedReplicaReaderCount.WithLabelValues("replica-a").Inc() + readReplicatedSelectedReplicaCount.WithLabelValues("replica-a").Add(3) + + strictReadReplicatedTotalQueryCount.Add(4) + strictReadReplicatedFallbackQueryCount.WithLabelValues("replica-a").Add(5) + + require.Equal(t, uint64(1), recording.HistogramCount("spicedb", "datastore", "loaded_relationships_count")) + require.Equal(t, uint64(1), recording.HistogramVecCount("spicedb", "datastore", "query_latency", "op", "shape")) + require.InEpsilon(t, 2.0, recording.CounterValue("spicedb", "datastore_replica", "checking_replicated_reader_total"), 1e-9) + require.InEpsilon(t, 1.0, recording.CounterVecValue("spicedb", "datastore_replica", "checking_replicated_replica_reader_total", "replica-a"), 1e-9) + require.InEpsilon(t, 3.0, recording.CounterVecValue("spicedb", "datastore_replica", "selected_replica_total", "replica-a"), 1e-9) + require.InEpsilon(t, 4.0, recording.CounterValue("spicedb", "datastore_replica", "strict_replicated_query_total"), 1e-9) + require.InEpsilon(t, 5.0, recording.CounterVecValue("spicedb", "datastore_replica", "strict_replicated_fallback_query_total", "replica-a"), 1e-9) +} + +func TestSetMetricsFactoryWithNilAndNoopProxy(t *testing.T) { + SetMetricsFactory(metrics.NoopFactory{}) + require.NotPanics(t, func() { + loadedRelationshipCount.Observe(1) + queryLatency.WithLabelValues("op", "shape").Observe(0.1) + checkingReplicatedTotalReaderCount.Inc() + checkingReplicatedReplicaReaderCount.WithLabelValues("replica-a").Inc() + readReplicatedSelectedReplicaCount.WithLabelValues("replica-a").Inc() + strictReadReplicatedTotalQueryCount.Inc() + strictReadReplicatedFallbackQueryCount.WithLabelValues("replica-a").Inc() + }) + + SetMetricsFactory(nil) + require.NotPanics(t, func() { + loadedRelationshipCount.Observe(1) + queryLatency.WithLabelValues("op", "shape").Observe(0.1) + checkingReplicatedTotalReaderCount.Inc() + checkingReplicatedReplicaReaderCount.WithLabelValues("replica-a").Inc() + readReplicatedSelectedReplicaCount.WithLabelValues("replica-a").Inc() + strictReadReplicatedTotalQueryCount.Inc() + strictReadReplicatedFallbackQueryCount.WithLabelValues("replica-a").Inc() + }) + + SetMetricsFactory(metrics.NewPrometheusFactory(nil)) +} diff --git a/internal/datastore/proxy/observable.go b/internal/datastore/proxy/observable.go index a54cc76f22..ed39e1782c 100644 --- a/internal/datastore/proxy/observable.go +++ b/internal/datastore/proxy/observable.go @@ -2,9 +2,8 @@ package proxy import ( "context" + "time" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" @@ -12,6 +11,7 @@ import ( v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" "github.com/authzed/spicedb/internal/datastore/common" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/internal/telemetry/otelconv" "github.com/authzed/spicedb/pkg/datastore" "github.com/authzed/spicedb/pkg/datastore/options" @@ -22,25 +22,39 @@ import ( var ( tracer = otel.Tracer("spicedb/datastore/proxy/observable") - loadedRelationshipCount = promauto.NewHistogram(prometheus.HistogramOpts{ + loadedRelationshipCount metrics.Histogram + + loadedRelationshipCountOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "datastore", Name: "loaded_relationships_count", Buckets: []float64{0, 1, 3, 10, 32, 100, 316, 1000, 3162, 10000}, Help: "Histogram of the number of relationships loaded per individual datastore query. High p99 values (>1000) may indicate broad permission checks or missing filters.", - }) + } + + queryLatency metrics.HistogramVec - queryLatency = promauto.NewHistogramVec(prometheus.HistogramOpts{ + queryLatencyOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "datastore", Name: "query_latency", Buckets: []float64{.0005, .001, .002, .005, .01, .02, .05, .1, .2, .5}, Help: "response latency for a database query", - }, []string{ - "operation", "query_shape", - }) + } ) +func init() { + setObservableMetricsFactory(metrics.NewPrometheusFactory(nil)) +} + +func setObservableMetricsFactory(factory metrics.Factory) { + if factory == nil { + factory = metrics.NoopFactory{} + } + loadedRelationshipCount = factory.Histogram(loadedRelationshipCountOpts) + queryLatency = factory.HistogramVec(queryLatencyOpts, []string{"operation", "query_shape"}) +} + func filterToAttributes(filter *v1.RelationshipFilter) []attribute.KeyValue { attrs := []attribute.KeyValue{common.ObjNamespaceNameKey.String(filter.ResourceType)} if filter.OptionalResourceId != "" { @@ -401,7 +415,7 @@ func observe(ctx context.Context, name string, queryShape string, opts ...trace. } ctx, span := tracer.Start(ctx, name, opts...) - timer := prometheus.NewTimer(queryLatency.WithLabelValues(name, queryShape)) + startTime := time.Now() closed := false return ctx, func() { @@ -410,7 +424,7 @@ func observe(ctx context.Context, name string, queryShape string, opts ...trace. } closed = true - timer.ObserveDuration() + queryLatency.WithLabelValues(name, queryShape).Observe(time.Since(startTime).Seconds()) span.End() } } diff --git a/internal/datastore/proxy/observable_test.go b/internal/datastore/proxy/observable_test.go index 60f641a503..6e2da8d23a 100644 --- a/internal/datastore/proxy/observable_test.go +++ b/internal/datastore/proxy/observable_test.go @@ -4,8 +4,6 @@ import ( "context" "testing" - "github.com/prometheus/client_golang/prometheus" - dto "github.com/prometheus/client_model/go" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -13,47 +11,51 @@ import ( "github.com/authzed/spicedb/internal/datastore/proxy/proxy_test" "github.com/authzed/spicedb/internal/datastore/revisions" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/datastore" core "github.com/authzed/spicedb/pkg/proto/core/v1" "github.com/authzed/spicedb/pkg/tuple" ) -var testRev = revisions.NewForTransactionID(42) +var ( + testRev = revisions.NewForTransactionID(42) + proxyMetricsFactory *metrics.RecordingFactory +) func floatPtr(f float64) *float64 { return &f } -func histogramSampleCount(m prometheus.Metric) uint64 { - pb := &dto.Metric{} - _ = m.Write(pb) - return pb.GetHistogram().GetSampleCount() -} - -func histogramSampleSum(m prometheus.Metric) float64 { - pb := &dto.Metric{} - _ = m.Write(pb) - return pb.GetHistogram().GetSampleSum() -} - func requireObservedLatency(t *testing.T, operation string, f func()) { t.Helper() - obs := queryLatency.WithLabelValues(operation, "(none)") - before := histogramSampleCount(obs.(prometheus.Metric)) + before := proxyMetricsFactory.HistogramVecCount("spicedb", "datastore", "query_latency", operation, "(none)") f() - after := histogramSampleCount(obs.(prometheus.Metric)) + after := proxyMetricsFactory.HistogramVecCount("spicedb", "datastore", "query_latency", operation, "(none)") require.Greater(t, after, before, "expected latency metric for %s", operation) } func requireObservedRelationshipCount(t *testing.T, expectedCount float64, f func()) { t.Helper() - beforeCount := histogramSampleCount(loadedRelationshipCount) - beforeSum := histogramSampleSum(loadedRelationshipCount) + beforeCount := proxyMetricsFactory.HistogramCount("spicedb", "datastore", "loaded_relationships_count") + beforeSum := proxyMetricsFactory.HistogramSum("spicedb", "datastore", "loaded_relationships_count") f() - afterCount := histogramSampleCount(loadedRelationshipCount) - afterSum := histogramSampleSum(loadedRelationshipCount) + afterCount := proxyMetricsFactory.HistogramCount("spicedb", "datastore", "loaded_relationships_count") + afterSum := proxyMetricsFactory.HistogramSum("spicedb", "datastore", "loaded_relationships_count") require.Greater(t, afterCount, beforeCount, "expected loadedRelationshipCount metric to be recorded") require.InDelta(t, expectedCount, afterSum-beforeSum, 0.01, "expected relationship count to match") } +func resetProxyMetricsForTest(t *testing.T) { + t.Helper() + proxyMetricsFactory = metrics.NewRecordingFactory() + setObservableMetricsFactory(proxyMetricsFactory) + setCheckingReplicatedMetricsFactory(proxyMetricsFactory) + setStrictReplicatedMetricsFactory(proxyMetricsFactory) + t.Cleanup(func() { + setObservableMetricsFactory(metrics.NewPrometheusFactory(nil)) + setCheckingReplicatedMetricsFactory(metrics.NewPrometheusFactory(nil)) + setStrictReplicatedMetricsFactory(metrics.NewPrometheusFactory(nil)) + }) +} + func newMocks() (*proxy_test.MockDatastore, *proxy_test.MockReader, *proxy_test.MockReadWriteTransaction) { dsMock := &proxy_test.MockDatastore{} readerMock := &proxy_test.MockReader{} @@ -139,6 +141,7 @@ func TestObservableProxy_DatastoreMethodsWithMetrics(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + resetProxyMetricsForTest(t) dsMock, _, _ := newMocks() tt.setupMock(dsMock) sut := NewObservableDatastoreProxy(dsMock) @@ -151,6 +154,7 @@ func TestObservableProxy_DatastoreMethodsWithMetrics(t *testing.T) { func TestObservableProxy_DatastorePassthroughMethods(t *testing.T) { t.Run("RevisionFromString", func(t *testing.T) { + resetProxyMetricsForTest(t) dsMock, _, _ := newMocks() dsMock.On("RevisionFromString", "42").Return(testRev, nil).Once() sut := NewObservableDatastoreProxy(dsMock) @@ -162,6 +166,7 @@ func TestObservableProxy_DatastorePassthroughMethods(t *testing.T) { }) t.Run("Watch", func(t *testing.T) { + resetProxyMetricsForTest(t) dsMock, _, _ := newMocks() changesCh := make(<-chan datastore.RevisionChanges) errCh := make(<-chan error) @@ -175,6 +180,7 @@ func TestObservableProxy_DatastorePassthroughMethods(t *testing.T) { }) t.Run("Close", func(t *testing.T) { + resetProxyMetricsForTest(t) dsMock, _, _ := newMocks() dsMock.On("Close").Return(nil).Once() @@ -184,6 +190,7 @@ func TestObservableProxy_DatastorePassthroughMethods(t *testing.T) { }) t.Run("OfflineFeatures", func(t *testing.T) { + resetProxyMetricsForTest(t) dsMock, _, _ := newMocks() dsMock.On("OfflineFeatures").Return(&datastore.Features{}, nil).Once() sut := NewObservableDatastoreProxy(dsMock) @@ -193,6 +200,7 @@ func TestObservableProxy_DatastorePassthroughMethods(t *testing.T) { }) t.Run("Unwrap", func(t *testing.T) { + resetProxyMetricsForTest(t) dsMock, _, _ := newMocks() sut := NewObservableDatastoreProxy(dsMock) require.Equal(t, dsMock, sut.(datastore.UnwrappableDatastore).Unwrap()) @@ -342,6 +350,7 @@ func TestObservableProxy_ReaderMethodsWithMetrics(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + resetProxyMetricsForTest(t) dsMock, readerMock, _ := newMocks() tt.setupMock(readerMock) @@ -473,6 +482,7 @@ func TestObservableProxy_RWTMethodsWithMetrics(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + resetProxyMetricsForTest(t) dsMock, _, rwtMock := newMocks() tt.setupMock(rwtMock) sut := NewObservableDatastoreProxy(dsMock) diff --git a/internal/datastore/proxy/schemacaching/watchingcache.go b/internal/datastore/proxy/schemacaching/watchingcache.go index f7fa5b3f4c..69f7009ca1 100644 --- a/internal/datastore/proxy/schemacaching/watchingcache.go +++ b/internal/datastore/proxy/schemacaching/watchingcache.go @@ -6,11 +6,10 @@ import ( "sync" "time" - "github.com/prometheus/client_golang/prometheus" - pgxcommon "github.com/authzed/spicedb/internal/datastore/postgres/common" "github.com/authzed/spicedb/internal/datastore/revisions" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/cache" "github.com/authzed/spicedb/pkg/datastore" "github.com/authzed/spicedb/pkg/datastore/options" @@ -19,47 +18,65 @@ import ( "github.com/authzed/spicedb/pkg/spiceerrors" ) -var namespacesFallbackModeGauge = prometheus.NewGauge(prometheus.GaugeOpts{ - Namespace: "spicedb", - Subsystem: "datastore", - Name: "watching_schema_cache_namespaces_fallback_mode", - Help: "Whether the watching schema cache for namespace definitions is in fallback mode (1) or normal mode (0). Fallback is triggered when the CockroachDB changefeed used to track schema updates becomes unavailable; in this state every schema lookup hits the datastore directly.", -}) - -var caveatsFallbackModeGauge = prometheus.NewGauge(prometheus.GaugeOpts{ - Namespace: "spicedb", - Subsystem: "datastore", - Name: "watching_schema_cache_caveats_fallback_mode", - Help: "Whether the watching schema cache for caveat definitions is in fallback mode (1) or normal mode (0). Fallback is triggered when the CockroachDB changefeed used to track schema updates becomes unavailable; in this state every schema lookup hits the datastore directly.", -}) - -var schemaCacheRevisionGauge = prometheus.NewGauge(prometheus.GaugeOpts{ - Namespace: "spicedb", - Subsystem: "datastore", - Name: "watching_schema_cache_tracked_revision", - Help: "The current maximum revision tracked by the CockroachDB changefeed-backed schema cache. A value that is not advancing over time indicates the changefeed has stalled.", -}) - -var definitionsReadCachedCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: "spicedb", - Subsystem: "datastore", - Name: "watching_schema_cache_definitions_read_cached_total", - Help: "cached number of definitions read from the watching cache", -}, []string{"definition_kind"}) - -var definitionsReadTotalCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: "spicedb", - Subsystem: "datastore", - Name: "watching_schema_cache_definitions_read_total", - Help: "total number of definitions read from the watching cache", -}, []string{"definition_kind"}) +var ( + namespacesFallbackModeGauge internalmetrics.Gauge + caveatsFallbackModeGauge internalmetrics.Gauge + schemaCacheRevisionGauge internalmetrics.Gauge + definitionsReadCachedCounter internalmetrics.CounterVec + definitionsReadTotalCounter internalmetrics.CounterVec +) -const maximumRetryCount = 10 +var ( + namespacesGaugeOpts = internalmetrics.Opts{ + Namespace: "spicedb", + Subsystem: "datastore", + Name: "watching_schema_cache_namespaces_fallback_mode", + Help: "Whether the watching schema cache for namespace definitions is in fallback mode (1) or normal mode (0). Fallback is triggered when the CockroachDB changefeed used to track schema updates becomes unavailable; in this state every schema lookup hits the datastore directly.", + } + caveatsGaugeOpts = internalmetrics.Opts{ + Namespace: "spicedb", + Subsystem: "datastore", + Name: "watching_schema_cache_caveats_fallback_mode", + Help: "Whether the watching schema cache for caveat definitions is in fallback mode (1) or normal mode (0). Fallback is triggered when the CockroachDB changefeed used to track schema updates becomes unavailable; in this state every schema lookup hits the datastore directly.", + } + schemaCacheRevisionGaugeOpts = internalmetrics.Opts{ + Namespace: "spicedb", + Subsystem: "datastore", + Name: "watching_schema_cache_tracked_revision", + Help: "The current maximum revision tracked by the CockroachDB changefeed-backed schema cache. A value that is not advancing over time indicates the changefeed has stalled.", + } + definitionsReadCachedCounterOpts = internalmetrics.Opts{ + Namespace: "spicedb", + Subsystem: "datastore", + Name: "watching_schema_cache_definitions_read_cached_total", + Help: "cached number of definitions read from the watching cache", + } + definitionsReadTotalCounterOpts = internalmetrics.Opts{ + Namespace: "spicedb", + Subsystem: "datastore", + Name: "watching_schema_cache_definitions_read_total", + Help: "total number of definitions read from the watching cache", + } +) func init() { - prometheus.MustRegister(namespacesFallbackModeGauge, caveatsFallbackModeGauge, schemaCacheRevisionGauge, definitionsReadCachedCounter, definitionsReadTotalCounter) + SetMetricsFactory(internalmetrics.NewPrometheusFactory(nil)) } +// SetMetricsFactory configures which metrics factory is used by this package. +func SetMetricsFactory(factory internalmetrics.Factory) { + if factory == nil { + factory = internalmetrics.NoopFactory{} + } + namespacesFallbackModeGauge = factory.Gauge(namespacesGaugeOpts) + caveatsFallbackModeGauge = factory.Gauge(caveatsGaugeOpts) + schemaCacheRevisionGauge = factory.Gauge(schemaCacheRevisionGaugeOpts) + definitionsReadCachedCounter = factory.CounterVec(definitionsReadCachedCounterOpts, []string{"definition_kind"}) + definitionsReadTotalCounter = factory.CounterVec(definitionsReadTotalCounterOpts, []string{"definition_kind"}) +} + +const maximumRetryCount = 10 + // watchingCachingProxy is a datastore proxy that caches schema (namespaces and caveat definitions) // and updates its cache via a WatchSchema call. If the supplied datastore to be wrapped does not support // this API, or the data is not available in this case or an error occurs, the updating cache fallsback @@ -393,14 +410,14 @@ type schemaWatchCache[T datastore.SchemaDefinition] struct { // definitionsReadCachedCounter is a counter of the number of cached definitions // returned by the cache directly (without fallback) - definitionsReadCachedCounter *prometheus.CounterVec + definitionsReadCachedCounter internalmetrics.CounterVec // definitionsReadTotalCounter is a counter of the total number of definitions // returned. - definitionsReadTotalCounter *prometheus.CounterVec + definitionsReadTotalCounter internalmetrics.CounterVec // fallbackGauge is a gauge holding a value of whether the cache is in fallback mode. - fallbackGauge prometheus.Gauge + fallbackGauge internalmetrics.Gauge lock sync.RWMutex } @@ -424,9 +441,9 @@ func newSchemaWatchCache[T datastore.SchemaDefinition]( notFoundError notFoundErrorFn, readDefinition readDefinitionFn[T], lookupDefinitions lookupDefinitionsFn[T], - definitionsReadCachedCounter *prometheus.CounterVec, - definitionsReadTotalCounter *prometheus.CounterVec, - fallbackGauge prometheus.Gauge, + definitionsReadCachedCounter internalmetrics.CounterVec, + definitionsReadTotalCounter internalmetrics.CounterVec, + fallbackGauge internalmetrics.Gauge, ) *schemaWatchCache[T] { fallbackGauge.Set(1) diff --git a/internal/datastore/proxy/strictreplicated.go b/internal/datastore/proxy/strictreplicated.go index 5d664a08cc..a463d96e99 100644 --- a/internal/datastore/proxy/strictreplicated.go +++ b/internal/datastore/proxy/strictreplicated.go @@ -5,11 +5,9 @@ import ( "errors" "fmt" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" - "github.com/authzed/spicedb/internal/datastore/common" log "github.com/authzed/spicedb/internal/logging" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/datastore" "github.com/authzed/spicedb/pkg/datastore/options" core "github.com/authzed/spicedb/pkg/proto/core/v1" @@ -17,21 +15,37 @@ import ( ) var ( - strictReadReplicatedTotalQueryCount = promauto.NewCounter(prometheus.CounterOpts{ + strictReadReplicatedTotalQueryCount metrics.Counter + + strictReadReplicatedTotalQueryCountOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "datastore_replica", Name: "strict_replicated_query_total", Help: "total number of reads made by the strict read replicated datastore", - }) + } - strictReadReplicatedFallbackQueryCount = promauto.NewCounterVec(prometheus.CounterOpts{ + strictReadReplicatedFallbackQueryCount metrics.CounterVec + + strictReadReplicatedFallbackQueryCountOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "datastore_replica", Name: "strict_replicated_fallback_query_total", Help: "number of queries that have fallen back to the primary datastore", - }, []string{"replica"}) + } ) +func init() { + setStrictReplicatedMetricsFactory(metrics.NewPrometheusFactory(nil)) +} + +func setStrictReplicatedMetricsFactory(factory metrics.Factory) { + if factory == nil { + factory = metrics.NoopFactory{} + } + strictReadReplicatedTotalQueryCount = factory.Counter(strictReadReplicatedTotalQueryCountOpts) + strictReadReplicatedFallbackQueryCount = factory.CounterVec(strictReadReplicatedFallbackQueryCountOpts, []string{"replica"}) +} + // NewStrictReplicatedDatastore creates a new datastore that writes to the provided primary and reads // from the provided replicas. The replicas are chosen in a round-robin fashion. If a replica does // not have the requested revision, the primary is used instead. diff --git a/internal/dispatch/caching/caching.go b/internal/dispatch/caching/caching.go index 25ae26bb10..c48f97de35 100644 --- a/internal/dispatch/caching/caching.go +++ b/internal/dispatch/caching/caching.go @@ -2,31 +2,26 @@ package caching import ( "context" - "fmt" "maps" "sync" "testing" "unsafe" "github.com/dustin/go-humanize" - "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" "github.com/authzed/spicedb/internal/dispatch" "github.com/authzed/spicedb/internal/dispatch/keys" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/internal/telemetry/otelconv" "github.com/authzed/spicedb/pkg/cache" "github.com/authzed/spicedb/pkg/middleware/nodeid" v1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1" ) -const ( - errCachingInitialization = "error initializing caching dispatcher: %w" - - prometheusNamespace = "spicedb" -) +const prometheusNamespace = "spicedb" // Dispatcher is a dispatcher with cacheInst-in caching. type Dispatcher struct { @@ -34,14 +29,15 @@ type Dispatcher struct { c cache.Cache[keys.DispatchCacheKey, any] keyHandler keys.Handler - checkTotalCounter prometheus.Counter - checkFromCacheCounter prometheus.Counter - lookupResourcesTotalCounter prometheus.Counter - lookupResourcesFromCacheCounter prometheus.Counter - lookupSubjectsTotalCounter prometheus.Counter - lookupSubjectsFromCacheCounter prometheus.Counter - queryPlanTotalCounter *prometheus.CounterVec - queryPlanFromCacheCounter *prometheus.CounterVec + metricsFactory metrics.Factory + checkTotalCounter metrics.Counter + checkFromCacheCounter metrics.Counter + lookupResourcesTotalCounter metrics.Counter + lookupResourcesFromCacheCounter metrics.Counter + lookupSubjectsTotalCounter metrics.Counter + lookupSubjectsFromCacheCounter metrics.Counter + queryPlanTotalCounter metrics.CounterVec + queryPlanFromCacheCounter metrics.CounterVec } func DispatchTestCache(t testing.TB) cache.Cache[keys.DispatchCacheKey, any] { @@ -55,114 +51,56 @@ func DispatchTestCache(t testing.TB) cache.Cache[keys.DispatchCacheKey, any] { // NewCachingDispatcher creates a new dispatch.Dispatcher which delegates // dispatch requests and caches the responses when possible and desirable. -func NewCachingDispatcher(cacheInst cache.Cache[keys.DispatchCacheKey, any], metricsEnabled bool, prometheusSubsystem string, keyHandler keys.Handler) (*Dispatcher, error) { +// +// factory controls how metrics are created and registered. Pass +// metrics.NewPrometheusFactory(registerer) in production and +// metrics.NewRecordingFactory() in tests that assert on metric values. +// Pass metrics.NoopFactory{} (or nil) to disable metrics entirely. +// +// prometheusSubsystem is used as the Prometheus subsystem field in every +// metric name, e.g. "dispatch_client" → spicedb_dispatch_client_check_total. +func NewCachingDispatcher(cacheInst cache.Cache[keys.DispatchCacheKey, any], factory metrics.Factory, prometheusSubsystem string, keyHandler keys.Handler) (*Dispatcher, error) { if cacheInst == nil { cacheInst = cache.NoopCache[keys.DispatchCacheKey, any]() } - - checkTotalCounter := prometheus.NewCounter(prometheus.CounterOpts{ - Namespace: prometheusNamespace, - Subsystem: prometheusSubsystem, - Name: "check_total", - Help: "Total number of CheckPermission dispatch requests processed.", - }) - checkFromCacheCounter := prometheus.NewCounter(prometheus.CounterOpts{ - Namespace: prometheusNamespace, - Subsystem: prometheusSubsystem, - Name: "check_from_cache_total", - Help: "Total number of CheckPermission dispatch requests served directly from the dispatch cache, avoiding re-computation.", - }) - - lookupResourcesTotalCounter := prometheus.NewCounter(prometheus.CounterOpts{ - Namespace: prometheusNamespace, - Subsystem: prometheusSubsystem, - Name: "lookup_resources_total", - Help: "Total number of LookupResources dispatch requests processed.", - }) - lookupResourcesFromCacheCounter := prometheus.NewCounter(prometheus.CounterOpts{ - Namespace: prometheusNamespace, - Subsystem: prometheusSubsystem, - Name: "lookup_resources_from_cache_total", - Help: "Total number of LookupResources dispatch requests served directly from the dispatch cache.", - }) - - lookupSubjectsTotalCounter := prometheus.NewCounter(prometheus.CounterOpts{ - Namespace: prometheusNamespace, - Subsystem: prometheusSubsystem, - Name: "lookup_subjects_total", - Help: "Total number of LookupSubjects dispatch requests processed.", - }) - lookupSubjectsFromCacheCounter := prometheus.NewCounter(prometheus.CounterOpts{ - Namespace: prometheusNamespace, - Subsystem: prometheusSubsystem, - Name: "lookup_subjects_from_cache_total", - Help: "Total number of LookupSubjects dispatch requests served directly from the dispatch cache.", - }) - - queryPlanTotalCounter := prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: prometheusNamespace, - Subsystem: prometheusSubsystem, - Name: "query_plan_total", - Help: "Total number of DispatchQueryPlan requests processed, labelled by plan operation.", - }, []string{"operation"}) - queryPlanFromCacheCounter := prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: prometheusNamespace, - Subsystem: prometheusSubsystem, - Name: "query_plan_from_cache_total", - Help: "Total number of DispatchQueryPlan requests served directly from the dispatch cache, labelled by plan operation.", - }, []string{"operation"}) - - if metricsEnabled && prometheusSubsystem != "" { - err := prometheus.Register(checkTotalCounter) - if err != nil { - return nil, fmt.Errorf(errCachingInitialization, err) - } - err = prometheus.Register(checkFromCacheCounter) - if err != nil { - return nil, fmt.Errorf(errCachingInitialization, err) - } - err = prometheus.Register(lookupResourcesTotalCounter) - if err != nil { - return nil, fmt.Errorf(errCachingInitialization, err) - } - err = prometheus.Register(lookupResourcesFromCacheCounter) - if err != nil { - return nil, fmt.Errorf(errCachingInitialization, err) - } - err = prometheus.Register(lookupSubjectsTotalCounter) - if err != nil { - return nil, fmt.Errorf(errCachingInitialization, err) - } - err = prometheus.Register(lookupSubjectsFromCacheCounter) - if err != nil { - return nil, fmt.Errorf(errCachingInitialization, err) - } - err = prometheus.Register(queryPlanTotalCounter) - if err != nil { - return nil, fmt.Errorf(errCachingInitialization, err) - } - err = prometheus.Register(queryPlanFromCacheCounter) - if err != nil { - return nil, fmt.Errorf(errCachingInitialization, err) - } + if factory == nil { + factory = metrics.NoopFactory{} } if keyHandler == nil { keyHandler = &keys.DirectKeyHandler{} } + newCounter := func(name, help string) metrics.Counter { + return factory.Counter(metrics.Opts{ + Namespace: prometheusNamespace, + Subsystem: prometheusSubsystem, + Name: name, + Help: help, + }) + } + newCounterVec := func(name, help string) metrics.CounterVec { + return factory.CounterVec(metrics.Opts{ + Namespace: prometheusNamespace, + Subsystem: prometheusSubsystem, + Name: name, + Help: help, + }, []string{"operation"}) + } + return &Dispatcher{ d: fakeDelegate{}, c: cacheInst, keyHandler: keyHandler, - checkTotalCounter: checkTotalCounter, - checkFromCacheCounter: checkFromCacheCounter, - lookupResourcesTotalCounter: lookupResourcesTotalCounter, - lookupResourcesFromCacheCounter: lookupResourcesFromCacheCounter, - lookupSubjectsTotalCounter: lookupSubjectsTotalCounter, - lookupSubjectsFromCacheCounter: lookupSubjectsFromCacheCounter, - queryPlanTotalCounter: queryPlanTotalCounter, - queryPlanFromCacheCounter: queryPlanFromCacheCounter, + metricsFactory: factory, + checkTotalCounter: newCounter("check_total", "Total number of CheckPermission dispatch requests processed."), + checkFromCacheCounter: newCounter("check_from_cache_total", "Total number of CheckPermission dispatch requests served directly from the dispatch cache, avoiding re-computation."), + lookupResourcesTotalCounter: newCounter("lookup_resources_total", "Total number of LookupResources dispatch requests processed."), + lookupResourcesFromCacheCounter: newCounter("lookup_resources_from_cache_total", "Total number of LookupResources dispatch requests served directly from the dispatch cache."), + lookupSubjectsTotalCounter: newCounter("lookup_subjects_total", "Total number of LookupSubjects dispatch requests processed."), + lookupSubjectsFromCacheCounter: newCounter("lookup_subjects_from_cache_total", "Total number of LookupSubjects dispatch requests served directly from the dispatch cache."), + queryPlanTotalCounter: newCounterVec("query_plan_total", "Total number of DispatchQueryPlan requests processed, labelled by plan operation."), + queryPlanFromCacheCounter: newCounterVec("query_plan_from_cache_total", "Total number of DispatchQueryPlan requests served directly from the dispatch cache, labelled by plan operation."), }, nil } @@ -482,14 +420,7 @@ func (cd *Dispatcher) dispatchQueryPlanCheckCached(req *v1.DispatchQueryPlanRequ } func (cd *Dispatcher) Close() error { - prometheus.Unregister(cd.checkTotalCounter) - prometheus.Unregister(cd.checkFromCacheCounter) - prometheus.Unregister(cd.lookupResourcesTotalCounter) - prometheus.Unregister(cd.lookupResourcesFromCacheCounter) - prometheus.Unregister(cd.lookupSubjectsTotalCounter) - prometheus.Unregister(cd.lookupSubjectsFromCacheCounter) - prometheus.Unregister(cd.queryPlanTotalCounter) - prometheus.Unregister(cd.queryPlanFromCacheCounter) + _ = cd.metricsFactory.Close() if cache := cd.c; cache != nil { cache.Close() } diff --git a/internal/dispatch/caching/cachingdispatch_test.go b/internal/dispatch/caching/cachingdispatch_test.go index 56b3b2b7b8..88db5f403f 100644 --- a/internal/dispatch/caching/cachingdispatch_test.go +++ b/internal/dispatch/caching/cachingdispatch_test.go @@ -2,19 +2,18 @@ package caching import ( "context" - "fmt" "sync" "sync/atomic" "testing" "time" - "github.com/prometheus/client_golang/prometheus" "github.com/shopspring/decimal" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/authzed/spicedb/internal/dispatch" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/datalayer" core "github.com/authzed/spicedb/pkg/proto/core/v1" v1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1" @@ -125,7 +124,7 @@ func TestMaxDepthCaching(t *testing.T) { } } - dispatch, err := NewCachingDispatcher(DispatchTestCache(t), false, "", nil) + dispatch, err := NewCachingDispatcher(DispatchTestCache(t), nil, "", nil) dispatch.SetDelegate(delegate) require.NoError(err) defer dispatch.Close() @@ -177,7 +176,7 @@ func TestConcurrentDebugInfoAccess(t *testing.T) { }, }, nil) - dispatcher, err := NewCachingDispatcher(DispatchTestCache(t), false, "", nil) + dispatcher, err := NewCachingDispatcher(DispatchTestCache(t), nil, "", nil) require.NoError(err) dispatcher.SetDelegate(delegate) t.Cleanup(func() { @@ -248,54 +247,10 @@ func (p *planDispatchMock) DispatchQueryPlan(_ *v1.DispatchQueryPlanRequest, str }) } -func sumOperationCounter(t *testing.T, gatherer prometheus.Gatherer, metricName, operationLabel string) float64 { - t.Helper() - families, err := gatherer.Gather() - require.NoError(t, err) - for _, mf := range families { - if mf.GetName() != metricName { - continue - } - var sum float64 - for _, m := range mf.GetMetric() { - matched := operationLabel == "" - for _, l := range m.GetLabel() { - if l.GetName() == "operation" && l.GetValue() == operationLabel { - matched = true - } - } - if matched { - sum += m.GetCounter().GetValue() - } - } - return sum - } - return 0 -} - -func sumPlainCounter(t *testing.T, gatherer prometheus.Gatherer, metricName string) float64 { - t.Helper() - families, err := gatherer.Gather() - require.NoError(t, err) - for _, mf := range families { - if mf.GetName() != metricName { - continue - } - var sum float64 - for _, m := range mf.GetMetric() { - sum += m.GetCounter().GetValue() - } - return sum - } - return 0 -} - func TestDispatchQueryPlanRecordsCachingMetrics(t *testing.T) { - // Use a unique subsystem so we don't collide with other tests in the global - // default Prometheus registry. - subsystem := fmt.Sprintf("test_caching_%d", time.Now().UnixNano()) + rf := metrics.NewRecordingFactory() - dispatcher, err := NewCachingDispatcher(DispatchTestCache(t), true, subsystem, nil) + dispatcher, err := NewCachingDispatcher(DispatchTestCache(t), rf, "dispatch_test", nil) require.NoError(t, err) t.Cleanup(func() { _ = dispatcher.Close() }) @@ -326,16 +281,14 @@ func TestDispatchQueryPlanRecordsCachingMetrics(t *testing.T) { require.NoError(t, dispatcher.DispatchQueryPlan(req, stream2)) require.Len(t, stream2.Results(), 1) - gatherer := prometheus.DefaultGatherer - - totalCheck := sumOperationCounter(t, gatherer, "spicedb_"+subsystem+"_query_plan_total", "check") + totalCheck := rf.CounterVecValue(prometheusNamespace, "dispatch_test", "query_plan_total", "check") require.InEpsilon(t, float64(2), totalCheck, 1e-9, "query_plan_total{operation=check} should bump for each plan dispatch") - fromCacheCheck := sumOperationCounter(t, gatherer, "spicedb_"+subsystem+"_query_plan_from_cache_total", "check") + fromCacheCheck := rf.CounterVecValue(prometheusNamespace, "dispatch_test", "query_plan_from_cache_total", "check") require.InEpsilon(t, float64(1), fromCacheCheck, 1e-9, "query_plan_from_cache_total{operation=check} should bump only on cache hit") - // Ensure the plan path no longer inflates the classic DispatchCheck counter. - checkTotal := sumPlainCounter(t, gatherer, "spicedb_"+subsystem+"_check_total") + // Ensure the plan path does not inflate the classic DispatchCheck counter. + checkTotal := rf.CounterValue(prometheusNamespace, "dispatch_test", "check_total") require.Zero(t, checkTotal, "check_total must not be incremented from the plan path") // The delegate is only consulted on the cache miss. diff --git a/internal/dispatch/cluster/cluster.go b/internal/dispatch/cluster/cluster.go index d27fe898f1..78d233843f 100644 --- a/internal/dispatch/cluster/cluster.go +++ b/internal/dispatch/cluster/cluster.go @@ -9,6 +9,7 @@ import ( "github.com/authzed/spicedb/internal/dispatch/graph" "github.com/authzed/spicedb/internal/dispatch/keys" log "github.com/authzed/spicedb/internal/logging" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/cache" caveattypes "github.com/authzed/spicedb/pkg/caveats/types" "github.com/authzed/spicedb/pkg/query" @@ -20,6 +21,7 @@ type Option func(*optionState) type optionState struct { metricsEnabled bool prometheusSubsystem string + metricsFactory metrics.Factory cache cache.Cache[keys.DispatchCacheKey, any] concurrencyLimits graph.ConcurrencyLimits remoteDispatchTimeout time.Duration @@ -52,6 +54,14 @@ func PrometheusSubsystem(name string) Option { } } +// MetricsFactory sets the metrics.Factory used to create and register +// caching-dispatcher metrics. +func MetricsFactory(f metrics.Factory) Option { + return func(state *optionState) { + state.metricsFactory = f + } +} + // Cache sets the cache for the remote dispatcher. func Cache(c cache.Cache[keys.DispatchCacheKey, any]) Option { return func(state *optionState) { @@ -159,7 +169,15 @@ func NewClusterDispatcher(dispatch dispatch.Dispatcher, options ...Option) (disp opts.prometheusSubsystem = "dispatch" } - cachingClusterDispatch, err := caching.NewCachingDispatcher(opts.cache, opts.metricsEnabled, opts.prometheusSubsystem, &keys.CanonicalKeyHandler{}) + if opts.metricsFactory == nil { + if opts.metricsEnabled { + opts.metricsFactory = metrics.NewPrometheusFactory(nil) // falls back to DefaultRegisterer + } else { + opts.metricsFactory = metrics.NoopFactory{} + } + } + + cachingClusterDispatch, err := caching.NewCachingDispatcher(opts.cache, opts.metricsFactory, opts.prometheusSubsystem, &keys.CanonicalKeyHandler{}) if err != nil { return nil, err } diff --git a/internal/dispatch/combined/combined.go b/internal/dispatch/combined/combined.go index 5f9b3c7214..dff72e366a 100644 --- a/internal/dispatch/combined/combined.go +++ b/internal/dispatch/combined/combined.go @@ -18,6 +18,7 @@ import ( "github.com/authzed/spicedb/internal/dispatch/singleflight" "github.com/authzed/spicedb/internal/grpchelpers" log "github.com/authzed/spicedb/internal/logging" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/cache" caveattypes "github.com/authzed/spicedb/pkg/caveats/types" v1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1" @@ -30,6 +31,7 @@ type Option func(*optionState) type optionState struct { metricsEnabled bool prometheusSubsystem string + metricsFactory metrics.Factory upstreamAddr string upstreamCAPath string grpcPresharedKey string @@ -71,6 +73,17 @@ func PrometheusSubsystem(name string) Option { } } +// MetricsFactory sets the metrics.Factory used to create and register +// caching-dispatcher metrics. When set, MetricsEnabled is ignored; the +// caller is responsible for choosing a Noop factory when metrics are not +// desired. When not set, the legacy metricsEnabled / prometheusSubsystem +// path is used. +func MetricsFactory(f metrics.Factory) Option { + return func(state *optionState) { + state.metricsFactory = f + } +} + // UpstreamAddr sets the optional cluster dispatching upstream address. func UpstreamAddr(addr string) Option { return func(state *optionState) { @@ -202,7 +215,15 @@ func NewDispatcher(options ...Option) (dispatch.Dispatcher, error) { opts.prometheusSubsystem = "dispatch_client" } - cachingRedispatch, err := caching.NewCachingDispatcher(opts.cache, opts.metricsEnabled, opts.prometheusSubsystem, &keys.CanonicalKeyHandler{}) + if opts.metricsFactory == nil { + if opts.metricsEnabled { + opts.metricsFactory = metrics.NewPrometheusFactory(nil) // falls back to DefaultRegisterer + } else { + opts.metricsFactory = metrics.NoopFactory{} + } + } + + cachingRedispatch, err := caching.NewCachingDispatcher(opts.cache, opts.metricsFactory, opts.prometheusSubsystem, &keys.CanonicalKeyHandler{}) if err != nil { return nil, err } @@ -250,7 +271,7 @@ func NewDispatcher(options ...Option) (dispatch.Dispatcher, error) { if err != nil { return nil, fmt.Errorf("failed to create dispatcher: %w", err) } - redispatch = singleflight.New(redispatch, &keys.CanonicalKeyHandler{}) + redispatch = singleflight.NewWithMetricsFactory(redispatch, &keys.CanonicalKeyHandler{}, opts.metricsFactory) } else { // If an upstream is specified, create a cluster dispatcher. if opts.upstreamCAPath != "" { @@ -316,7 +337,7 @@ func NewDispatcher(options ...Option) (dispatch.Dispatcher, error) { if err != nil { return nil, err } - redispatch = singleflight.New(re, &keys.CanonicalKeyHandler{}) + redispatch = singleflight.NewWithMetricsFactory(re, &keys.CanonicalKeyHandler{}, opts.metricsFactory) } cachingRedispatch.SetDelegate(redispatch) diff --git a/internal/dispatch/graph/check_test.go b/internal/dispatch/graph/check_test.go index 27dcff4cc9..ae32dbc3d6 100644 --- a/internal/dispatch/graph/check_test.go +++ b/internal/dispatch/graph/check_test.go @@ -2074,7 +2074,7 @@ func newLocalDispatcher(t testing.TB) (context.Context, dispatch.Dispatcher, dat _ = dispatch.Close() }) - cachingDispatcher, err := caching.NewCachingDispatcher(caching.DispatchTestCache(t), false, "", &keys.CanonicalKeyHandler{}) + cachingDispatcher, err := caching.NewCachingDispatcher(caching.DispatchTestCache(t), nil, "", &keys.CanonicalKeyHandler{}) require.NoError(t, err) cachingDispatcher.SetDelegate(dispatch) t.Cleanup(func() { @@ -2099,7 +2099,7 @@ func newLocalDispatcherWithSchemaAndRels(t testing.TB, schema string, rels []tup dispatch.Close() }) - cachingDispatcher, err := caching.NewCachingDispatcher(caching.DispatchTestCache(t), false, "", &keys.CanonicalKeyHandler{}) + cachingDispatcher, err := caching.NewCachingDispatcher(caching.DispatchTestCache(t), nil, "", &keys.CanonicalKeyHandler{}) require.NoError(t, err) cachingDispatcher.SetDelegate(dispatch) t.Cleanup(func() { diff --git a/internal/dispatch/remote/cluster.go b/internal/dispatch/remote/cluster.go index e12bbbdef1..8b21103199 100644 --- a/internal/dispatch/remote/cluster.go +++ b/internal/dispatch/remote/cluster.go @@ -26,41 +26,62 @@ import ( "github.com/authzed/spicedb/internal/dispatch" "github.com/authzed/spicedb/internal/dispatch/keys" log "github.com/authzed/spicedb/internal/logging" + metrics "github.com/authzed/spicedb/internal/metrics" corev1 "github.com/authzed/spicedb/pkg/proto/core/v1" v1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1" "github.com/authzed/spicedb/pkg/spiceerrors" "github.com/authzed/spicedb/pkg/tuple" ) -var dispatchCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: "spicedb", - Subsystem: "dispatch", - Name: "remote_dispatch_handler_total", - Help: "which dispatcher handled a request", -}, []string{"request_kind", "handler_name"}) - -var hedgeWaitHistogram = prometheus.NewHistogramVec(prometheus.HistogramOpts{ - Namespace: "spicedb", - Subsystem: "dispatch", - Name: "remote_dispatch_hedge_wait_duration_seconds", - Help: "distribution in seconds of calculated wait time for hedging requests to the primary dispatcher when a secondary is active.", - Buckets: []float64{0.001, 0.002, 0.003, 0.005, 0.01, 0.02, 0.05, 0.1, 0.3, 0.5, 1, 10}, -}, []string{"rpc"}) - -var hedgeActualWaitHistogram = prometheus.NewHistogramVec(prometheus.HistogramOpts{ - Namespace: "spicedb", - Subsystem: "dispatch", - Name: "remote_dispatch_hedge_actual_wait_duration_seconds", - Help: "distribution in seconds of actual wait time for hedging requests to the primary dispatcher when a secondary is active.", - Buckets: []float64{0.001, 0.002, 0.003, 0.005, 0.01, 0.02, 0.05, 0.1, 0.3, 0.5, 1, 10}, -}, []string{"rpc"}) - -var primaryDispatch = prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: "spicedb", - Subsystem: "dispatch", - Name: "remote_dispatch_primary_total", - Help: "indicates what was the outcome of a scheduled primary dispatch: dispatched, or cancelled", -}, []string{"cancelled", "rpc"}) +var ( + dispatchCounter metrics.CounterVec + hedgeWaitHistogram metrics.HistogramVec + hedgeActualWaitHistogram metrics.HistogramVec + primaryDispatch metrics.CounterVec +) + +var ( + dispatchCounterOpts = metrics.Opts{ + Namespace: "spicedb", + Subsystem: "dispatch", + Name: "remote_dispatch_handler_total", + Help: "which dispatcher handled a request", + } + hedgeWaitHistogramOpts = metrics.Opts{ + Namespace: "spicedb", + Subsystem: "dispatch", + Name: "remote_dispatch_hedge_wait_duration_seconds", + Help: "distribution in seconds of calculated wait time for hedging requests to the primary dispatcher when a secondary is active.", + Buckets: []float64{0.001, 0.002, 0.003, 0.005, 0.01, 0.02, 0.05, 0.1, 0.3, 0.5, 1, 10}, + } + hedgeActualWaitHistogramOpts = metrics.Opts{ + Namespace: "spicedb", + Subsystem: "dispatch", + Name: "remote_dispatch_hedge_actual_wait_duration_seconds", + Help: "distribution in seconds of actual wait time for hedging requests to the primary dispatcher when a secondary is active.", + Buckets: []float64{0.001, 0.002, 0.003, 0.005, 0.01, 0.02, 0.05, 0.1, 0.3, 0.5, 1, 10}, + } + primaryDispatchOpts = metrics.Opts{ + Namespace: "spicedb", + Subsystem: "dispatch", + Name: "remote_dispatch_primary_total", + Help: "indicates what was the outcome of a scheduled primary dispatch: dispatched, or cancelled", + } +) + +func init() { + setClusterDispatchMetricsFactory(metrics.NewPrometheusFactory(nil)) +} + +func setClusterDispatchMetricsFactory(factory metrics.Factory) { + if factory == nil { + factory = metrics.NoopFactory{} + } + dispatchCounter = factory.CounterVec(dispatchCounterOpts, []string{"request_kind", "handler_name"}) + hedgeWaitHistogram = factory.HistogramVec(hedgeWaitHistogramOpts, []string{"rpc"}) + hedgeActualWaitHistogram = factory.HistogramVec(hedgeActualWaitHistogramOpts, []string{"rpc"}) + primaryDispatch = factory.CounterVec(primaryDispatchOpts, []string{"cancelled", "rpc"}) +} // defaultStartingPrimaryHedgingDelay is the delay used by default for primary calls (when secondaries are available), // before statistics are available to determine the actual delay. @@ -77,13 +98,6 @@ const defaultTDigestCompression = float64(1000) var supportsSecondaries = []string{"check", "lookupresources", "lookupsubjects"} -func init() { - prometheus.MustRegister(dispatchCounter) - prometheus.MustRegister(hedgeWaitHistogram) - prometheus.MustRegister(hedgeActualWaitHistogram) - prometheus.MustRegister(primaryDispatch) -} - type ClusterClient interface { DispatchCheck(ctx context.Context, req *v1.DispatchCheckRequest, opts ...grpc.CallOption) (*v1.DispatchCheckResponse, error) DispatchExpand(ctx context.Context, req *v1.DispatchExpandRequest, opts ...grpc.CallOption) (*v1.DispatchExpandResponse, error) @@ -100,6 +114,10 @@ type ClusterDispatcherConfig struct { // DispatchOverallTimeout is the maximum duration of a dispatched request // before it should timeout. DispatchOverallTimeout time.Duration + + // PrometheusRegisterer is the registerer used to publish cluster-dispatch + // metrics. If nil, prometheus.DefaultRegisterer is used. + PrometheusRegisterer prometheus.Registerer } // SecondaryDispatch defines a struct holding a client and its name for secondary @@ -124,6 +142,8 @@ func NewClusterDispatcher(client ClusterClient, conn *grpc.ClientConn, config Cl keyHandler = &keys.DirectKeyHandler{} } + setClusterDispatchMetricsFactory(metrics.NewPrometheusFactory(config.PrometheusRegisterer)) + dispatchOverallTimeout := config.DispatchOverallTimeout if dispatchOverallTimeout <= 0 { dispatchOverallTimeout = 60 * time.Second diff --git a/internal/dispatch/remote/cluster_test.go b/internal/dispatch/remote/cluster_test.go index 766c14ae78..71e3f2ce27 100644 --- a/internal/dispatch/remote/cluster_test.go +++ b/internal/dispatch/remote/cluster_test.go @@ -12,6 +12,7 @@ import ( "github.com/caio/go-tdigest/v4" "github.com/ccoveille/go-safecast/v2" "github.com/dustin/go-humanize" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" "google.golang.org/genproto/googleapis/rpc/errdetails" "google.golang.org/grpc" @@ -30,6 +31,23 @@ import ( "github.com/authzed/spicedb/pkg/tuple" ) +func TestNewClusterDispatcherRepeatedRegistration(t *testing.T) { + registry := prometheus.NewRegistry() + + first, err := NewClusterDispatcher(nil, nil, ClusterDispatcherConfig{ + PrometheusRegisterer: registry, + }, nil, nil, 0) + require.NoError(t, err) + require.NotNil(t, first) + + // Repeated construction with the same registry must not return an error. + second, err := NewClusterDispatcher(nil, nil, ClusterDispatcherConfig{ + PrometheusRegisterer: registry, + }, nil, nil, 0) + require.NoError(t, err) + require.NotNil(t, second) +} + type fakeDispatchSvc struct { v1.UnimplementedDispatchServiceServer diff --git a/internal/dispatch/singleflight/singleflight.go b/internal/dispatch/singleflight/singleflight.go index ae03f29127..f00da663d7 100644 --- a/internal/dispatch/singleflight/singleflight.go +++ b/internal/dispatch/singleflight/singleflight.go @@ -6,8 +6,6 @@ import ( "fmt" "strconv" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "resenje.org/singleflight" @@ -15,29 +13,37 @@ import ( "github.com/authzed/spicedb/internal/dispatch" "github.com/authzed/spicedb/internal/dispatch/keys" log "github.com/authzed/spicedb/internal/logging" + "github.com/authzed/spicedb/internal/metrics" v1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1" ) -var ( - singleFlightCount = promauto.NewCounterVec(singleFlightCountConfig, []string{"method", "shared"}) - singleFlightCountConfig = prometheus.CounterOpts{ - Namespace: "spicedb", - Subsystem: "dispatch", - Name: "single_flight_total", - Help: "total number of dispatch requests that were single flighted", - } -) +var singleFlightCountOpts = metrics.Opts{ + Namespace: "spicedb", + Subsystem: "dispatch", + Name: "single_flight_total", + Help: "total number of dispatch requests that were single flighted", +} func New(delegate dispatch.Dispatcher, handler keys.Handler) dispatch.Dispatcher { + return NewWithMetricsFactory(delegate, handler, metrics.NewPrometheusFactory(nil)) +} + +func NewWithMetricsFactory(delegate dispatch.Dispatcher, handler keys.Handler, factory metrics.Factory) dispatch.Dispatcher { + if factory == nil { + factory = metrics.NoopFactory{} + } + return &Dispatcher{ - delegate: delegate, - keyHandler: handler, + delegate: delegate, + keyHandler: handler, + singleFlightCount: factory.CounterVec(singleFlightCountOpts, []string{"method", "shared"}), } } type Dispatcher struct { - delegate dispatch.Dispatcher - keyHandler keys.Handler + delegate dispatch.Dispatcher + keyHandler keys.Handler + singleFlightCount metrics.CounterVec checkGroup singleflight.Group[string, *v1.DispatchCheckResponse] expandGroup singleflight.Group[string, *v1.DispatchExpandResponse] @@ -61,7 +67,7 @@ func (d *Dispatcher) DispatchCheck(ctx context.Context, req *v1.DispatchCheckReq return &v1.DispatchCheckResponse{Metadata: &v1.ResponseMeta{DispatchCount: 1}}, status.Error(codes.Internal, fmt.Errorf("unable to create traversal bloom filter: %w", err).Error()) } - singleFlightCount.WithLabelValues("DispatchCheck", "missing").Inc() + d.singleFlightCount.WithLabelValues("DispatchCheck", "missing").Inc() req.Metadata.TraversalBloom = tb return d.delegate.DispatchCheck(ctx, req) } @@ -75,7 +81,7 @@ func (d *Dispatcher) DispatchCheck(ctx context.Context, req *v1.DispatchCheckReq return &v1.DispatchCheckResponse{Metadata: &v1.ResponseMeta{DispatchCount: 1}}, err } else if possiblyLoop { log.Debug().Object("DispatchCheckRequest", req).Str("key", keyString).Msg("potential DispatchCheckRequest loop detected") - singleFlightCount.WithLabelValues("DispatchCheck", "loop").Inc() + d.singleFlightCount.WithLabelValues("DispatchCheck", "loop").Inc() return d.delegate.DispatchCheck(ctx, req) } @@ -87,7 +93,7 @@ func (d *Dispatcher) DispatchCheck(ctx context.Context, req *v1.DispatchCheckReq sharedResp = &v1.DispatchCheckResponse{Metadata: &v1.ResponseMeta{DispatchCount: 1}} } - singleFlightCount.WithLabelValues("DispatchCheck", strconv.FormatBool(isShared)).Inc() + d.singleFlightCount.WithLabelValues("DispatchCheck", strconv.FormatBool(isShared)).Inc() return sharedResp.CloneVT(), err } @@ -109,7 +115,7 @@ func (d *Dispatcher) DispatchExpand(ctx context.Context, req *v1.DispatchExpandR return &v1.DispatchExpandResponse{Metadata: &v1.ResponseMeta{DispatchCount: 1}}, status.Error(codes.Internal, fmt.Errorf("unable to create traversal bloom filter: %w", err).Error()) } - singleFlightCount.WithLabelValues("DispatchExpand", "missing").Inc() + d.singleFlightCount.WithLabelValues("DispatchExpand", "missing").Inc() req.Metadata.TraversalBloom = tb return d.delegate.DispatchExpand(ctx, req) } @@ -119,7 +125,7 @@ func (d *Dispatcher) DispatchExpand(ctx context.Context, req *v1.DispatchExpandR return &v1.DispatchExpandResponse{Metadata: &v1.ResponseMeta{DispatchCount: 1}}, err } else if possiblyLoop { log.Debug().Object("DispatchExpand", req).Str("key", keyString).Msg("potential DispatchExpand loop detected") - singleFlightCount.WithLabelValues("DispatchExpand", "loop").Inc() + d.singleFlightCount.WithLabelValues("DispatchExpand", "loop").Inc() return d.delegate.DispatchExpand(ctx, req) } @@ -127,7 +133,7 @@ func (d *Dispatcher) DispatchExpand(ctx context.Context, req *v1.DispatchExpandR return d.delegate.DispatchExpand(innerCtx, req) }) - singleFlightCount.WithLabelValues("DispatchExpand", strconv.FormatBool(isShared)).Inc() + d.singleFlightCount.WithLabelValues("DispatchExpand", strconv.FormatBool(isShared)).Inc() if err != nil { return &v1.DispatchExpandResponse{Metadata: &v1.ResponseMeta{DispatchCount: 1}}, err } @@ -150,7 +156,7 @@ func (d *Dispatcher) DispatchQueryPlan(req *v1.DispatchQueryPlanRequest, stream // Plan dispatches are not currently grouped via singleflight; record the // passthrough so the metric series exists and can be compared against the // other dispatch methods. - singleFlightCount.WithLabelValues("DispatchQueryPlan", "passthrough").Inc() + d.singleFlightCount.WithLabelValues("DispatchQueryPlan", "passthrough").Inc() return d.delegate.DispatchQueryPlan(req, stream) } diff --git a/internal/dispatch/singleflight/singleflight_test.go b/internal/dispatch/singleflight/singleflight_test.go index a6b91771e3..300430a950 100644 --- a/internal/dispatch/singleflight/singleflight_test.go +++ b/internal/dispatch/singleflight/singleflight_test.go @@ -9,13 +9,12 @@ import ( "time" "github.com/bits-and-blooms/bloom/v3" - "github.com/prometheus/client_golang/prometheus" - promclient "github.com/prometheus/client_model/go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/authzed/spicedb/internal/dispatch" "github.com/authzed/spicedb/internal/dispatch/keys" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/datalayer" v1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1" "github.com/authzed/spicedb/pkg/tuple" @@ -76,8 +75,7 @@ func TestSingleFlightDispatcher(t *testing.T) { } func TestSingleFlightDispatcherDetectsLoop(t *testing.T) { - singleFlightCount = prometheus.NewCounterVec(singleFlightCountConfig, []string{"method", "shared"}) - reg := registerMetricInGatherer(singleFlightCount) + rf := metrics.NewRecordingFactory() var called atomic.Uint64 f := func() { @@ -85,7 +83,7 @@ func TestSingleFlightDispatcherDetectsLoop(t *testing.T) { called.Add(1) } keyHandler := &keys.DirectKeyHandler{} - disp := New(mockDispatcher{f: f}, keyHandler) + disp := NewWithMetricsFactory(mockDispatcher{f: f}, keyHandler, rf) req := &v1.DispatchCheckRequest{ ResourceRelation: tuple.RR("document", "view").ToCoreRR(), @@ -127,13 +125,12 @@ func TestSingleFlightDispatcherDetectsLoop(t *testing.T) { wg.Wait() require.Equal(t, uint64(4), called.Load(), "should have dispatched %d calls but did %d", uint64(4), called.Load()) - assertCounterWithLabel(t, reg, 2, "spicedb_dispatch_single_flight_total", "loop") + require.Equal(t, float64(3), rf.CounterVecValue("spicedb", "dispatch", "single_flight_total", "DispatchCheck", "loop")) //nolint:testifylint // exact small integer values } // this test makes sure that bloom filter information is carried from dispatcher to dispatcher func TestSingleFlightDispatcherDetectsLoopThroughDelegate(t *testing.T) { - singleFlightCount = prometheus.NewCounterVec(singleFlightCountConfig, []string{"method", "shared"}) - reg := registerMetricInGatherer(singleFlightCount) + rf := metrics.NewRecordingFactory() var called atomic.Uint64 f := func() { @@ -142,7 +139,7 @@ func TestSingleFlightDispatcherDetectsLoopThroughDelegate(t *testing.T) { } keyHandler := &keys.DirectKeyHandler{} // we simulate an actual dispatch-chain loop by nesting 2 singleflight dispatchers - disp := New(New(mockDispatcher{f: f}, keyHandler), keyHandler) + disp := NewWithMetricsFactory(NewWithMetricsFactory(mockDispatcher{f: f}, keyHandler, rf), keyHandler, rf) req := &v1.DispatchCheckRequest{ ResourceRelation: tuple.RR("document", "view").ToCoreRR(), @@ -174,7 +171,7 @@ func TestSingleFlightDispatcherDetectsLoopThroughDelegate(t *testing.T) { wg.Wait() require.Equal(t, uint64(1), called.Load(), "should have dispatched %d calls but did %d", uint64(1), called.Load()) - assertCounterWithLabel(t, reg, 2, "spicedb_dispatch_single_flight_total", "loop") + require.Greater(t, rf.CounterVecValue("spicedb", "dispatch", "single_flight_total", "DispatchCheck", "loop"), float64(0)) } func TestSingleFlightDispatcherCancelation(t *testing.T) { @@ -277,14 +274,13 @@ func TestSingleFlightDispatcherExpand(t *testing.T) { } func TestSingleFlightDispatcherCheckBypassesIfMissingBloomFiler(t *testing.T) { - singleFlightCount = prometheus.NewCounterVec(singleFlightCountConfig, []string{"method", "shared"}) - reg := registerMetricInGatherer(singleFlightCount) + rf := metrics.NewRecordingFactory() var called atomic.Uint64 f := func() { called.Add(1) } - disp := New(mockDispatcher{f: f}, &keys.DirectKeyHandler{}) + disp := NewWithMetricsFactory(mockDispatcher{f: f}, &keys.DirectKeyHandler{}, rf) req := &v1.DispatchCheckRequest{ ResourceRelation: tuple.RR("document", "view").ToCoreRR(), @@ -299,18 +295,17 @@ func TestSingleFlightDispatcherCheckBypassesIfMissingBloomFiler(t *testing.T) { _, _ = disp.DispatchCheck(t.Context(), req.CloneVT()) require.Equal(t, uint64(1), called.Load(), "should have dispatched %d calls but did %d", uint64(1), called.Load()) - assertCounterWithLabel(t, reg, 1, "spicedb_dispatch_single_flight_total", "missing") + require.Equal(t, float64(1), rf.CounterVecValue("spicedb", "dispatch", "single_flight_total", "DispatchCheck", "missing")) //nolint:testifylint // exact small integer values } func TestSingleFlightDispatcherExpandBypassesIfMissingBloomFiler(t *testing.T) { - singleFlightCount = prometheus.NewCounterVec(singleFlightCountConfig, []string{"method", "shared"}) - reg := registerMetricInGatherer(singleFlightCount) + rf := metrics.NewRecordingFactory() var called atomic.Uint64 f := func() { called.Add(1) } - disp := New(mockDispatcher{f: f}, &keys.DirectKeyHandler{}) + disp := NewWithMetricsFactory(mockDispatcher{f: f}, &keys.DirectKeyHandler{}, rf) req := &v1.DispatchExpandRequest{ ResourceAndRelation: tuple.ONRStringToCore("document", "foo", "view"), @@ -323,14 +318,13 @@ func TestSingleFlightDispatcherExpandBypassesIfMissingBloomFiler(t *testing.T) { _, _ = disp.DispatchExpand(t.Context(), req.CloneVT()) require.Equal(t, uint64(1), called.Load(), "should have dispatched %d calls but did %d", uint64(1), called.Load()) - assertCounterWithLabel(t, reg, 1, "spicedb_dispatch_single_flight_total", "missing") + require.Equal(t, float64(1), rf.CounterVecValue("spicedb", "dispatch", "single_flight_total", "DispatchExpand", "missing")) //nolint:testifylint // exact small integer values } func TestDispatchQueryPlanRecordsSingleflightMetric(t *testing.T) { - singleFlightCount = prometheus.NewCounterVec(singleFlightCountConfig, []string{"method", "shared"}) - reg := registerMetricInGatherer(singleFlightCount) + rf := metrics.NewRecordingFactory() - disp := New(mockDispatcher{f: func() {}}, &keys.DirectKeyHandler{}) + disp := NewWithMetricsFactory(mockDispatcher{f: func() {}}, &keys.DirectKeyHandler{}, rf) req := &v1.DispatchQueryPlanRequest{ Operation: v1.PlanOperation_PLAN_OPERATION_CHECK, @@ -340,40 +334,7 @@ func TestDispatchQueryPlanRecordsSingleflightMetric(t *testing.T) { err := disp.DispatchQueryPlan(req, stream) require.NoError(t, err) - assertCounterWithLabel(t, reg, 1, "spicedb_dispatch_single_flight_total", "DispatchQueryPlan") -} - -func registerMetricInGatherer(collector prometheus.Collector) prometheus.Gatherer { - reg := prometheus.NewRegistry() - reg.MustRegister(collector) - - return reg -} - -func assertCounterWithLabel(t *testing.T, gatherer prometheus.Gatherer, expectedMetricsCount int, metricName, labelName string) { - t.Helper() - - metrics, err := gatherer.Gather() - require.NoError(t, err) - - var mf *promclient.MetricFamily - for _, metric := range metrics { - if metric.GetName() == metricName { - mf = metric - } - } - - found := false - require.Len(t, mf.GetMetric(), expectedMetricsCount) - for _, metric := range mf.GetMetric() { - for _, label := range metric.Label { - if *label.Value == labelName { - found = true - } - } - } - - require.True(t, found, "didn't find counter with label %s", labelName) + require.Equal(t, float64(1), rf.CounterVecValue("spicedb", "dispatch", "single_flight_total", "DispatchQueryPlan", "passthrough")) //nolint:testifylint // exact small integer values } func bloomFilterForRequest(t *testing.T, keyHandler *keys.DirectKeyHandler, req *v1.DispatchCheckRequest) []byte { diff --git a/internal/gateway/gateway.go b/internal/gateway/gateway.go index f8ff2cc936..3cfdfbfedf 100644 --- a/internal/gateway/gateway.go +++ b/internal/gateway/gateway.go @@ -10,8 +10,6 @@ import ( "strings" "github.com/grpc-ecosystem/grpc-gateway/v2/runtime" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" "github.com/prometheus/client_golang/prometheus/promhttp" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" grpcfilters "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/filters" @@ -30,18 +28,29 @@ import ( "github.com/authzed/grpcutil" "github.com/authzed/spicedb/internal/grpchelpers" + "github.com/authzed/spicedb/internal/metrics" ) -var histogram = promauto.NewHistogramVec(prometheus.HistogramOpts{ +var requestDurationOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "rest_gateway", Name: "request_duration_seconds", Help: "A histogram of the duration spent processing requests to the SpiceDB REST Gateway.", -}, []string{"method"}) +} // NewHandler creates an REST gateway HTTP CloserHandler with the provided upstream // configuration. func NewHandler(ctx context.Context, upstreamAddr, upstreamTLSCertPath string) (*CloserHandler, error) { + return NewHandlerWithMetricsFactory(ctx, upstreamAddr, upstreamTLSCertPath, metrics.NewPrometheusFactory(nil)) +} + +// NewHandlerWithMetricsFactory creates a REST gateway handler and instruments +// request duration metrics using the provided metrics factory. +func NewHandlerWithMetricsFactory(ctx context.Context, upstreamAddr, upstreamTLSCertPath string, factory metrics.Factory) (*CloserHandler, error) { + if factory == nil { + factory = metrics.NoopFactory{} + } + if upstreamAddr == "" { return nil, errors.New("upstreamAddr must not be empty") } @@ -106,7 +115,12 @@ func NewHandler(ctx context.Context, upstreamAddr, upstreamTLSCertPath string) ( otelhttp.WithFilter(httpfilters.Not(httpfilters.Path("/healthz"))), } - finalHandler := promhttp.InstrumentHandlerDuration(histogram, otelhttp.NewHandler(mux, "gateway", otelHandlerOpts...)) + baseHandler := otelhttp.NewHandler(mux, "gateway", otelHandlerOpts...) + finalHandler := baseHandler + if histogram, ok := metrics.AsPrometheusHistogramVec(factory.HistogramVec(requestDurationOpts, []string{"method"})); ok { + finalHandler = promhttp.InstrumentHandlerDuration(histogram, baseHandler) + } + return newCloserHandler(finalHandler, schemaConn, permissionsConn, watchConn, healthConn, experimentalConn), nil } diff --git a/internal/graph/check.go b/internal/graph/check.go index 6d9a9938e1..3a9eeff47f 100644 --- a/internal/graph/check.go +++ b/internal/graph/check.go @@ -6,7 +6,6 @@ import ( "fmt" "time" - "github.com/prometheus/client_golang/prometheus" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/trace" "google.golang.org/protobuf/types/known/durationpb" @@ -14,6 +13,7 @@ import ( "github.com/authzed/spicedb/internal/dispatch" "github.com/authzed/spicedb/internal/graph/hints" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/internal/namespace" "github.com/authzed/spicedb/internal/taskrunner" "github.com/authzed/spicedb/pkg/datalayer" @@ -33,16 +33,26 @@ import ( var tracer = otel.Tracer("spicedb/internal/graph/check") -var dispatchChunkCountHistogram = prometheus.NewHistogram(prometheus.HistogramOpts{ +var dispatchChunkCountHistogram internalmetrics.Histogram + +var dispatchChunkCountHistogramOpts = internalmetrics.Opts{ Name: "spicedb_check_dispatch_chunk_count", Help: "number of chunks when dispatching in check", Buckets: []float64{1, 2, 3, 5, 10, 25, 100, 250}, -}) +} const noOriginalRelation = "" func init() { - prometheus.MustRegister(dispatchChunkCountHistogram) + SetMetricsFactory(internalmetrics.NewPrometheusFactory(nil)) +} + +// SetMetricsFactory configures which metrics factory is used by this package. +func SetMetricsFactory(factory internalmetrics.Factory) { + if factory == nil { + factory = internalmetrics.NoopFactory{} + } + dispatchChunkCountHistogram = factory.Histogram(dispatchChunkCountHistogramOpts) } // NewConcurrentChecker creates an instance of ConcurrentChecker. diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go new file mode 100644 index 0000000000..5be7256c4e --- /dev/null +++ b/internal/metrics/metrics.go @@ -0,0 +1,103 @@ +// Package metrics defines the dependency-injection interfaces for metrics +// collection in SpiceDB. Business components receive a Factory at +// construction time; they never import prometheus directly. +// +// Two implementations are provided: +// - PrometheusFactory – registers real Prometheus collectors with a +// supplied prometheus.Registerer. Created once at the composition root +// (main / server.Config.Complete) and threaded through constructors. +// - RecordingFactory – accumulates counter and histogram observations +// in memory with no external dependencies. Intended for unit tests that +// need deterministic assertions on what was emitted. +// +// A NoopFactory is also available for components that are constructed with +// metrics disabled. +package metrics + +import "io" + +// Counter is a monotonically increasing scalar value. +type Counter interface { + // Inc increments the counter by 1. + Inc() + // Add increments the counter by delta. + Add(delta float64) +} + +// Gauge is a scalar value that may increase or decrease over time. +type Gauge interface { + // Inc increments the gauge by 1. + Inc() + // Add increments the gauge by delta. + Add(delta float64) + // Set updates the gauge to the provided value. + Set(value float64) +} + +// CounterVec is a Counter partitioned by one or more label dimensions. +type CounterVec interface { + // WithLabelValues returns the Counter for the given label values. + // The number of values must match the number of label names declared + // when the CounterVec was created. + WithLabelValues(lvs ...string) Counter +} + +// GaugeVec is a Gauge partitioned by one or more label dimensions. +type GaugeVec interface { + // WithLabelValues returns the Gauge for the given label values. + // The number of values must match the number of label names declared + // when the GaugeVec was created. + WithLabelValues(lvs ...string) Gauge +} + +// Histogram records observed values into a distribution. +type Histogram interface { + // Observe records a new value. + Observe(value float64) +} + +// HistogramVec is a Histogram partitioned by one or more label dimensions. +type HistogramVec interface { + // WithLabelValues returns the Histogram for the given label values. + // The number of values must match the number of label names declared + // when the HistogramVec was created. + WithLabelValues(lvs ...string) Histogram +} + +// Opts carries the metadata used to describe a metric. The fields mirror +// prometheus.Opts / prometheus.HistogramOpts so callers can be written +// without a direct Prometheus import. +type Opts struct { + // Namespace is prepended to the metric name, e.g. "spicedb". + Namespace string + // Subsystem is inserted between Namespace and Name, e.g. "dispatch_client". + Subsystem string + // Name is the metric base name, e.g. "check_total". + Name string + // Help is a human-readable description. + Help string + // Buckets is used only when creating histograms. + Buckets []float64 + // NativeHistogramBucketFactor configures Prometheus native histograms. + NativeHistogramBucketFactor float64 +} + +// Factory creates and registers named metrics. +// Calling Close deregisters every metric created by this factory, making +// it safe to use one factory per server lifecycle (or per test). +type Factory interface { + // Counter creates and registers a counter metric. + Counter(opts Opts) Counter + // CounterVec creates and registers a labelled counter metric. + CounterVec(opts Opts, labelNames []string) CounterVec + // Gauge creates and registers a gauge metric. + Gauge(opts Opts) Gauge + // GaugeVec creates and registers a labelled gauge metric. + GaugeVec(opts Opts, labelNames []string) GaugeVec + // Histogram creates and registers a histogram metric. + Histogram(opts Opts) Histogram + // HistogramVec creates and registers a labelled histogram metric. + HistogramVec(opts Opts, labelNames []string) HistogramVec + // Close deregisters all metrics created by this factory. + io.Closer +} diff --git a/internal/metrics/prometheus.go b/internal/metrics/prometheus.go new file mode 100644 index 0000000000..e91eadd30d --- /dev/null +++ b/internal/metrics/prometheus.go @@ -0,0 +1,216 @@ +package metrics + +import ( + "errors" + "sync" + + "github.com/prometheus/client_golang/prometheus" + + log "github.com/authzed/spicedb/internal/logging" +) + +// PrometheusFactory creates Prometheus metrics and registers them with the +// supplied prometheus.Registerer. All metrics it creates are unregistered +// when Close is called. +// +// If a duplicate-registration error occurs (AlreadyRegisteredError) the +// existing collector is reused silently – this matches the behaviour of +// the previous RegisterPrometheusCollectors helper. +type PrometheusFactory struct { + registerer prometheus.Registerer + mu sync.Mutex + collectors []prometheus.Collector // GUARDED_BY(mu) +} + +// NewPrometheusFactory returns a factory that registers metrics with r. +// If r is nil, prometheus.DefaultRegisterer is used. +func NewPrometheusFactory(r prometheus.Registerer) *PrometheusFactory { + if r == nil { + r = prometheus.DefaultRegisterer + } + return &PrometheusFactory{registerer: r} +} + +// Counter creates a Prometheus counter and registers it. Returns the +// existing counter if this metric was already registered. +func (f *PrometheusFactory) Counter(opts Opts) Counter { + c := prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: opts.Namespace, + Subsystem: opts.Subsystem, + Name: opts.Name, + Help: opts.Help, + }) + if existing, ok := f.register(c); ok { + if ec, ok2 := existing.(prometheus.Counter); ok2 { + return ec + } + } + return c +} + +// Gauge creates a Prometheus gauge and registers it. Returns the +// existing gauge if this metric was already registered. +func (f *PrometheusFactory) Gauge(opts Opts) Gauge { + g := prometheus.NewGauge(prometheus.GaugeOpts{ + Namespace: opts.Namespace, + Subsystem: opts.Subsystem, + Name: opts.Name, + Help: opts.Help, + }) + if existing, ok := f.register(g); ok { + if eg, ok2 := existing.(prometheus.Gauge); ok2 { + return eg + } + } + return g +} + +// Histogram creates a Prometheus histogram and registers it. Returns the +// existing histogram if this metric was already registered. +func (f *PrometheusFactory) Histogram(opts Opts) Histogram { + h := prometheus.NewHistogram(prometheus.HistogramOpts{ + Namespace: opts.Namespace, + Subsystem: opts.Subsystem, + Name: opts.Name, + Help: opts.Help, + Buckets: opts.Buckets, + NativeHistogramBucketFactor: opts.NativeHistogramBucketFactor, + }) + if existing, ok := f.register(h); ok { + if eh, ok2 := existing.(prometheus.Histogram); ok2 { + return eh + } + } + return h +} + +// CounterVec creates a Prometheus CounterVec and registers it. Returns the +// existing vec if already registered. +func (f *PrometheusFactory) CounterVec(opts Opts, labelNames []string) CounterVec { + cv := prometheus.NewCounterVec(prometheus.CounterOpts{ + Namespace: opts.Namespace, + Subsystem: opts.Subsystem, + Name: opts.Name, + Help: opts.Help, + }, labelNames) + if existing, ok := f.register(cv); ok { + if ecv, ok2 := existing.(*prometheus.CounterVec); ok2 { + return &prometheusCounterVec{ecv} + } + } + return &prometheusCounterVec{cv} +} + +// GaugeVec creates a Prometheus GaugeVec and registers it. Returns the +// existing vec if already registered. +func (f *PrometheusFactory) GaugeVec(opts Opts, labelNames []string) GaugeVec { + gv := prometheus.NewGaugeVec(prometheus.GaugeOpts{ + Namespace: opts.Namespace, + Subsystem: opts.Subsystem, + Name: opts.Name, + Help: opts.Help, + }, labelNames) + if existing, ok := f.register(gv); ok { + if egv, ok2 := existing.(*prometheus.GaugeVec); ok2 { + return &prometheusGaugeVec{egv} + } + } + return &prometheusGaugeVec{gv} +} + +// HistogramVec creates a Prometheus HistogramVec and registers it. Returns the +// existing vec if already registered. +func (f *PrometheusFactory) HistogramVec(opts Opts, labelNames []string) HistogramVec { + hv := prometheus.NewHistogramVec(prometheus.HistogramOpts{ + Namespace: opts.Namespace, + Subsystem: opts.Subsystem, + Name: opts.Name, + Help: opts.Help, + Buckets: opts.Buckets, + NativeHistogramBucketFactor: opts.NativeHistogramBucketFactor, + }, labelNames) + if existing, ok := f.register(hv); ok { + if ehv, ok2 := existing.(*prometheus.HistogramVec); ok2 { + return &prometheusHistogramVec{ehv} + } + } + return &prometheusHistogramVec{hv} +} + +// prometheusCounterVec adapts *prometheus.CounterVec to the CounterVec interface. +// The adaptation is needed because *prometheus.CounterVec.WithLabelValues returns +// prometheus.Counter, not metrics.Counter (different named types despite same shape). +type prometheusCounterVec struct { + cv *prometheus.CounterVec +} + +func (p *prometheusCounterVec) WithLabelValues(lvs ...string) Counter { + return p.cv.WithLabelValues(lvs...) +} + +type prometheusGaugeVec struct { + gv *prometheus.GaugeVec +} + +func (p *prometheusGaugeVec) WithLabelValues(lvs ...string) Gauge { + return p.gv.WithLabelValues(lvs...) +} + +type prometheusHistogramVec struct { + hv *prometheus.HistogramVec +} + +func (p *prometheusHistogramVec) WithLabelValues(lvs ...string) Histogram { + return p.hv.WithLabelValues(lvs...) +} + +// AsPrometheusHistogramVec returns the underlying Prometheus histogram vec when +// hv was created by PrometheusFactory. +func AsPrometheusHistogramVec(hv HistogramVec) (*prometheus.HistogramVec, bool) { + promHV, ok := hv.(*prometheusHistogramVec) + if !ok { + return nil, false + } + return promHV.hv, true +} + +// AsPrometheusGaugeVec returns the underlying Prometheus gauge vec when +// gv was created by PrometheusFactory. Useful for callers that need +// Prometheus-specific operations (e.g. DeletePartialMatch) beyond the +// GaugeVec interface. +func AsPrometheusGaugeVec(gv GaugeVec) (*prometheus.GaugeVec, bool) { + promGV, ok := gv.(*prometheusGaugeVec) + if !ok { + return nil, false + } + return promGV.gv, true +} + +// Close unregisters every metric this factory created. +func (f *PrometheusFactory) Close() error { + f.mu.Lock() + defer f.mu.Unlock() + for _, c := range f.collectors { + f.registerer.Unregister(c) + } + f.collectors = nil + return nil +} + +// register attempts to register c with the Prometheus registerer. On +// AlreadyRegisteredError it returns (existingCollector, true). On any other +// error it logs a warning and returns (nil, false) – the original c should be +// used as a no-op. +func (f *PrometheusFactory) register(c prometheus.Collector) (prometheus.Collector, bool) { + if err := f.registerer.Register(c); err != nil { + if are, ok := errors.AsType[prometheus.AlreadyRegisteredError](err); ok { + return are.ExistingCollector, true + } + log.Warn().Err(err).Msg("failed to register prometheus metric") + return nil, false + } + f.mu.Lock() + f.collectors = append(f.collectors, c) + f.mu.Unlock() + return nil, false +} diff --git a/internal/metrics/prometheus_test.go b/internal/metrics/prometheus_test.go new file mode 100644 index 0000000000..2bc3d838cc --- /dev/null +++ b/internal/metrics/prometheus_test.go @@ -0,0 +1,185 @@ +package metrics + +import ( + "testing" + + "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" + "github.com/stretchr/testify/require" +) + +func TestPrometheusFactoryReusesCounterGaugeAndHistogramVectors(t *testing.T) { + registry := prometheus.NewRegistry() + pf := NewPrometheusFactory(registry) + + firstCounter := pf.Counter(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "factory_total", Help: "test counter"}) + firstCounter.Inc() + secondCounter := pf.Counter(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "factory_total", Help: "test counter"}) + secondCounter.Add(2) + + firstGaugeVec := pf.GaugeVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "factory_current", Help: "test gauge"}, []string{"kind"}) + firstGaugeVec.WithLabelValues("check").Set(1) + secondGaugeVec := pf.GaugeVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "factory_current", Help: "test gauge"}, []string{"kind"}) + secondGaugeVec.WithLabelValues("check").Add(2) + + firstHistogramVec := pf.HistogramVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "factory_seconds", Help: "test histogram", Buckets: []float64{0.1, 1, 10}}, []string{"kind"}) + firstHistogramVec.WithLabelValues("check").Observe(0.5) + secondHistogramVec := pf.HistogramVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "factory_seconds", Help: "test histogram", Buckets: []float64{0.1, 1, 10}}, []string{"kind"}) + secondHistogramVec.WithLabelValues("check").Observe(1.5) + + families, err := registry.Gather() + require.NoError(t, err) + + counterMetric := metricFamilyByName(t, families, "spicedb_metrics_factory_total") + require.Len(t, counterMetric.GetMetric(), 1) + require.InEpsilon(t, 3.0, counterMetric.GetMetric()[0].GetCounter().GetValue(), 1e-9) + + gaugeMetric := metricFamilyByName(t, families, "spicedb_metrics_factory_current") + require.Len(t, gaugeMetric.GetMetric(), 1) + require.InEpsilon(t, 3.0, gaugeMetric.GetMetric()[0].GetGauge().GetValue(), 1e-9) + + histogramMetric := metricFamilyByName(t, families, "spicedb_metrics_factory_seconds") + require.Len(t, histogramMetric.GetMetric(), 1) + require.Equal(t, uint64(2), histogramMetric.GetMetric()[0].GetHistogram().GetSampleCount()) + require.InEpsilon(t, 2.0, histogramMetric.GetMetric()[0].GetHistogram().GetSampleSum(), 1e-9) +} + +func TestPrometheusFactoryGaugeAndHistogramAndCounterVec(t *testing.T) { + registry := prometheus.NewRegistry() + pf := NewPrometheusFactory(registry) + + // Gauge (non-vec) + g := pf.Gauge(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "pf_gauge", Help: "test gauge"}) + g.Set(5) + g.Add(2) + g.Inc() + // Reuse returns same gauge. + g2 := pf.Gauge(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "pf_gauge", Help: "test gauge"}) + g2.Add(1) + + // Histogram (non-vec) + h := pf.Histogram(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "pf_hist", Help: "test histogram", Buckets: []float64{1, 5, 10}}) + h.Observe(2) + h.Observe(4) + // Reuse returns same histogram. + h2 := pf.Histogram(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "pf_hist", Help: "test histogram", Buckets: []float64{1, 5, 10}}) + h2.Observe(6) + + // CounterVec + cv := pf.CounterVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "pf_cv_total", Help: "test counter vec"}, []string{"op"}) + cv.WithLabelValues("read").Inc() + cv.WithLabelValues("write").Add(3) + // Reuse returns same vec. + cv2 := pf.CounterVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "pf_cv_total", Help: "test counter vec"}, []string{"op"}) + cv2.WithLabelValues("read").Add(2) + + families, err := registry.Gather() + require.NoError(t, err) + + gaugeMetric := metricFamilyByName(t, families, "spicedb_metrics_pf_gauge") + require.InEpsilon(t, 9.0, gaugeMetric.GetMetric()[0].GetGauge().GetValue(), 1e-9) + + histMetric := metricFamilyByName(t, families, "spicedb_metrics_pf_hist") + require.Equal(t, uint64(3), histMetric.GetMetric()[0].GetHistogram().GetSampleCount()) + + cvMetric := metricFamilyByName(t, families, "spicedb_metrics_pf_cv_total") + readVal, writeVal := 0.0, 0.0 + for _, m := range cvMetric.GetMetric() { + for _, lp := range m.GetLabel() { + if lp.GetName() == "op" { + switch lp.GetValue() { + case "read": + readVal = m.GetCounter().GetValue() + case "write": + writeVal = m.GetCounter().GetValue() + } + } + } + } + require.InEpsilon(t, 3.0, readVal, 1e-9) + require.InEpsilon(t, 3.0, writeVal, 1e-9) +} + +func TestPrometheusFactoryNilRegistererUsesDefault(t *testing.T) { + // NewPrometheusFactory(nil) should not panic and should create a usable factory. + pf := NewPrometheusFactory(nil) + require.NotNil(t, pf) + // Ensure we get a non-nil metric back without panicking. + c := pf.Counter(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "nil_reg_total", Help: "test"}) + require.NotNil(t, c) +} + +func TestPrometheusFactoryRegistrationFailureFallsBackToOriginal(t *testing.T) { + // When the registerer returns a non-AlreadyRegisteredError, register() + // logs a warning and returns the originally-created metric as a fallback. + pf := NewPrometheusFactory(failingRegisterer{}) + c := pf.Counter(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "fail_reg_total", Help: "test"}) + require.NotNil(t, c) + require.NotPanics(t, func() { c.Inc() }) +} + +func TestPrometheusFactoryClose(t *testing.T) { + registry := prometheus.NewRegistry() + pf := NewPrometheusFactory(registry) + + pf.Counter(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "close_total", Help: "test"}) + pf.Gauge(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "close_gauge", Help: "test"}) + pf.Histogram(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "close_hist", Help: "test"}) + + // Before close, metrics are registered. + families, err := registry.Gather() + require.NoError(t, err) + require.Len(t, families, 3) + + require.NoError(t, pf.Close()) + + // After close, the registry is empty. + families, err = registry.Gather() + require.NoError(t, err) + require.Empty(t, families) +} + +func TestAsPrometheusHistogramVec(t *testing.T) { + registry := prometheus.NewRegistry() + pf := NewPrometheusFactory(registry) + + hv := pf.HistogramVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "as_hv", Help: "test"}, []string{"op"}) + + promHV, ok := AsPrometheusHistogramVec(hv) + require.True(t, ok) + require.NotNil(t, promHV) + + // A RecordingFactory vec should return false. + rf := NewRecordingFactory() + rhv := rf.HistogramVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "as_hv", Help: "test"}, []string{"op"}) + _, ok = AsPrometheusHistogramVec(rhv) + require.False(t, ok) +} + +func TestAsPrometheusGaugeVec(t *testing.T) { + registry := prometheus.NewRegistry() + pf := NewPrometheusFactory(registry) + + gv := pf.GaugeVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "as_gv", Help: "test"}, []string{"op"}) + + promGV, ok := AsPrometheusGaugeVec(gv) + require.True(t, ok) + require.NotNil(t, promGV) + + // A RecordingFactory vec should return false. + rf := NewRecordingFactory() + rgv := rf.GaugeVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "as_gv", Help: "test"}, []string{"op"}) + _, ok = AsPrometheusGaugeVec(rgv) + require.False(t, ok) +} + +func metricFamilyByName(t *testing.T, families []*dto.MetricFamily, name string) *dto.MetricFamily { + t.Helper() + for _, family := range families { + if family.GetName() == name { + return family + } + } + t.Fatalf("metric family %s not found", name) + return nil +} diff --git a/internal/metrics/recording.go b/internal/metrics/recording.go new file mode 100644 index 0000000000..dad499affd --- /dev/null +++ b/internal/metrics/recording.go @@ -0,0 +1,423 @@ +package metrics + +import ( + "strings" + "sync" +) + +// RecordingFactory is a test-only Factory implementation that records all +// counter increments and histogram observations in memory. No Prometheus +// dependency is required. +// +// Retrieve recorded values via CounterValue / CounterVecValue after the code +// under test has run. +type RecordingFactory struct { + mu sync.Mutex + counters map[string]*recordingCounter // GUARDED_BY(mu) + counterVecs map[string]*recordingCounterVec // GUARDED_BY(mu) + gauges map[string]*recordingGauge // GUARDED_BY(mu) + gaugeVecs map[string]*recordingGaugeVec // GUARDED_BY(mu) + histograms map[string]*recordingHistogram // GUARDED_BY(mu) + histogramVecs map[string]*recordingHistogramVec // GUARDED_BY(mu) +} + +// NewRecordingFactory returns an empty RecordingFactory ready for use. +func NewRecordingFactory() *RecordingFactory { + return &RecordingFactory{ + counters: make(map[string]*recordingCounter), + counterVecs: make(map[string]*recordingCounterVec), + gauges: make(map[string]*recordingGauge), + gaugeVecs: make(map[string]*recordingGaugeVec), + histograms: make(map[string]*recordingHistogram), + histogramVecs: make(map[string]*recordingHistogramVec), + } +} + +// Counter creates an in-memory counter identified by the fully-qualified name +// derived from opts (Namespace_Subsystem_Name). +func (f *RecordingFactory) Counter(opts Opts) Counter { + name := metricName(opts) + f.mu.Lock() + defer f.mu.Unlock() + if c, ok := f.counters[name]; ok { + return c + } + c := &recordingCounter{} + f.counters[name] = c + return c +} + +// CounterVec creates an in-memory labelled counter. +func (f *RecordingFactory) CounterVec(opts Opts, _ []string) CounterVec { + name := metricName(opts) + f.mu.Lock() + defer f.mu.Unlock() + if cv, ok := f.counterVecs[name]; ok { + return cv + } + cv := &recordingCounterVec{children: make(map[string]*recordingCounter)} + f.counterVecs[name] = cv + return cv +} + +// Gauge creates an in-memory gauge identified by the fully-qualified name +// derived from opts (Namespace_Subsystem_Name). +func (f *RecordingFactory) Gauge(opts Opts) Gauge { + name := metricName(opts) + f.mu.Lock() + defer f.mu.Unlock() + if g, ok := f.gauges[name]; ok { + return g + } + g := &recordingGauge{} + f.gauges[name] = g + return g +} + +// GaugeVec creates an in-memory labelled gauge. +func (f *RecordingFactory) GaugeVec(opts Opts, _ []string) GaugeVec { + name := metricName(opts) + f.mu.Lock() + defer f.mu.Unlock() + if gv, ok := f.gaugeVecs[name]; ok { + return gv + } + gv := &recordingGaugeVec{children: make(map[string]*recordingGauge)} + f.gaugeVecs[name] = gv + return gv +} + +// Histogram creates an in-memory histogram identified by the fully-qualified +// name derived from opts (Namespace_Subsystem_Name). +func (f *RecordingFactory) Histogram(opts Opts) Histogram { + name := metricName(opts) + f.mu.Lock() + defer f.mu.Unlock() + if h, ok := f.histograms[name]; ok { + return h + } + h := &recordingHistogram{} + f.histograms[name] = h + return h +} + +// HistogramVec creates an in-memory labelled histogram. +func (f *RecordingFactory) HistogramVec(opts Opts, _ []string) HistogramVec { + name := metricName(opts) + f.mu.Lock() + defer f.mu.Unlock() + if hv, ok := f.histogramVecs[name]; ok { + return hv + } + hv := &recordingHistogramVec{children: make(map[string]*recordingHistogram)} + f.histogramVecs[name] = hv + return hv +} + +// Close is a no-op; recording factories do not hold external resources. +func (f *RecordingFactory) Close() error { return nil } + +// CounterValue returns the current value of the counter registered under +// namespace/subsystem/name. Returns 0 if the counter was never created. +func (f *RecordingFactory) CounterValue(namespace, subsystem, name string) float64 { + key := metricName(Opts{Namespace: namespace, Subsystem: subsystem, Name: name}) + f.mu.Lock() + defer f.mu.Unlock() + if c, ok := f.counters[key]; ok { + return c.value() + } + return 0 +} + +// CounterVecValue returns the current value of a labelled counter. Labels +// are supplied as alternating key/value pairs (e.g. "operation", "check"). +// Returns 0 if the metric or label combination was never observed. +func (f *RecordingFactory) CounterVecValue(namespace, subsystem, name string, labelValues ...string) float64 { + key := metricName(Opts{Namespace: namespace, Subsystem: subsystem, Name: name}) + f.mu.Lock() + defer f.mu.Unlock() + cv, ok := f.counterVecs[key] + if !ok { + return 0 + } + return cv.value(labelValues...) +} + +// GaugeValue returns the current value of the gauge registered under +// namespace/subsystem/name. Returns 0 if the gauge was never created. +func (f *RecordingFactory) GaugeValue(namespace, subsystem, name string) float64 { + key := metricName(Opts{Namespace: namespace, Subsystem: subsystem, Name: name}) + f.mu.Lock() + defer f.mu.Unlock() + if g, ok := f.gauges[key]; ok { + return g.value() + } + return 0 +} + +// GaugeVecValue returns the current value of a labelled gauge. Returns 0 if the +// metric or label combination was never observed. +func (f *RecordingFactory) GaugeVecValue(namespace, subsystem, name string, labelValues ...string) float64 { + key := metricName(Opts{Namespace: namespace, Subsystem: subsystem, Name: name}) + f.mu.Lock() + defer f.mu.Unlock() + gv, ok := f.gaugeVecs[key] + if !ok { + return 0 + } + return gv.value(labelValues...) +} + +// HistogramCount returns the number of observations recorded for the histogram. +func (f *RecordingFactory) HistogramCount(namespace, subsystem, name string) uint64 { + key := metricName(Opts{Namespace: namespace, Subsystem: subsystem, Name: name}) + f.mu.Lock() + defer f.mu.Unlock() + if h, ok := f.histograms[key]; ok { + return h.count() + } + return 0 +} + +// HistogramSum returns the sum of observations recorded for the histogram. +func (f *RecordingFactory) HistogramSum(namespace, subsystem, name string) float64 { + key := metricName(Opts{Namespace: namespace, Subsystem: subsystem, Name: name}) + f.mu.Lock() + defer f.mu.Unlock() + if h, ok := f.histograms[key]; ok { + return h.sumValue() + } + return 0 +} + +// HistogramVecCount returns the number of observations recorded for a labelled histogram. +func (f *RecordingFactory) HistogramVecCount(namespace, subsystem, name string, labelValues ...string) uint64 { + key := metricName(Opts{Namespace: namespace, Subsystem: subsystem, Name: name}) + f.mu.Lock() + defer f.mu.Unlock() + hv, ok := f.histogramVecs[key] + if !ok { + return 0 + } + return hv.count(labelValues...) +} + +// HistogramVecSum returns the sum of observations recorded for a labelled histogram. +func (f *RecordingFactory) HistogramVecSum(namespace, subsystem, name string, labelValues ...string) float64 { + key := metricName(Opts{Namespace: namespace, Subsystem: subsystem, Name: name}) + f.mu.Lock() + defer f.mu.Unlock() + hv, ok := f.histogramVecs[key] + if !ok { + return 0 + } + return hv.sumValue(labelValues...) +} + +// ---- internal types ---- + +type recordingCounter struct { + mu sync.Mutex + v float64 // GUARDED_BY(mu) +} + +func (c *recordingCounter) Inc() { + c.Add(1) +} + +func (c *recordingCounter) Add(delta float64) { + c.mu.Lock() + defer c.mu.Unlock() + c.v += delta +} + +func (c *recordingCounter) value() float64 { + c.mu.Lock() + defer c.mu.Unlock() + return c.v +} + +type recordingCounterVec struct { + mu sync.Mutex + children map[string]*recordingCounter // GUARDED_BY(mu) +} + +func (cv *recordingCounterVec) WithLabelValues(lvs ...string) Counter { + key := strings.Join(lvs, "\x00") + cv.mu.Lock() + defer cv.mu.Unlock() + if c, ok := cv.children[key]; ok { + return c + } + c := &recordingCounter{} + cv.children[key] = c + return c +} + +func (cv *recordingCounterVec) value(lvs ...string) float64 { + key := strings.Join(lvs, "\x00") + cv.mu.Lock() + defer cv.mu.Unlock() + if c, ok := cv.children[key]; ok { + return c.value() + } + return 0 +} + +type recordingGauge struct { + mu sync.Mutex + v float64 // GUARDED_BY(mu) +} + +func (g *recordingGauge) Inc() { + g.Add(1) +} + +func (g *recordingGauge) Add(delta float64) { + g.mu.Lock() + defer g.mu.Unlock() + g.v += delta +} + +func (g *recordingGauge) Set(value float64) { + g.mu.Lock() + defer g.mu.Unlock() + g.v = value +} + +func (g *recordingGauge) value() float64 { + g.mu.Lock() + defer g.mu.Unlock() + return g.v +} + +type recordingGaugeVec struct { + mu sync.Mutex + children map[string]*recordingGauge // GUARDED_BY(mu) +} + +func (gv *recordingGaugeVec) WithLabelValues(lvs ...string) Gauge { + key := strings.Join(lvs, "\x00") + gv.mu.Lock() + defer gv.mu.Unlock() + if g, ok := gv.children[key]; ok { + return g + } + g := &recordingGauge{} + gv.children[key] = g + return g +} + +func (gv *recordingGaugeVec) value(lvs ...string) float64 { + key := strings.Join(lvs, "\x00") + gv.mu.Lock() + defer gv.mu.Unlock() + if g, ok := gv.children[key]; ok { + return g.value() + } + return 0 +} + +type recordingHistogram struct { + mu sync.Mutex + observationCount uint64 // GUARDED_BY(mu) + sum float64 // GUARDED_BY(mu) +} + +func (h *recordingHistogram) Observe(value float64) { + h.mu.Lock() + defer h.mu.Unlock() + h.observationCount++ + h.sum += value +} + +func (h *recordingHistogram) count() uint64 { + h.mu.Lock() + defer h.mu.Unlock() + return h.observationCount +} + +func (h *recordingHistogram) sumValue() float64 { + h.mu.Lock() + defer h.mu.Unlock() + return h.sum +} + +type recordingHistogramVec struct { + mu sync.Mutex + children map[string]*recordingHistogram // GUARDED_BY(mu) +} + +func (hv *recordingHistogramVec) WithLabelValues(lvs ...string) Histogram { + key := strings.Join(lvs, "\x00") + hv.mu.Lock() + defer hv.mu.Unlock() + if h, ok := hv.children[key]; ok { + return h + } + h := &recordingHistogram{} + hv.children[key] = h + return h +} + +func (hv *recordingHistogramVec) count(lvs ...string) uint64 { + key := strings.Join(lvs, "\x00") + hv.mu.Lock() + defer hv.mu.Unlock() + if h, ok := hv.children[key]; ok { + return h.count() + } + return 0 +} + +func (hv *recordingHistogramVec) sumValue(lvs ...string) float64 { + key := strings.Join(lvs, "\x00") + hv.mu.Lock() + defer hv.mu.Unlock() + if h, ok := hv.children[key]; ok { + return h.sumValue() + } + return 0 +} + +// metricName builds a canonical "namespace_subsystem_name" key, omitting +// empty parts so the result is stable regardless of which fields are set. +func metricName(opts Opts) string { + parts := make([]string, 0, 3) + for _, p := range []string{opts.Namespace, opts.Subsystem, opts.Name} { + if p != "" { + parts = append(parts, p) + } + } + return strings.Join(parts, "_") +} + +// NoopFactory discards all metrics. Use when metrics are disabled. +type NoopFactory struct{} + +func (NoopFactory) Counter(_ Opts) Counter { return noopCounter{} } +func (NoopFactory) CounterVec(_ Opts, _ []string) CounterVec { return noopCounterVec{} } +func (NoopFactory) Gauge(_ Opts) Gauge { return noopGauge{} } +func (NoopFactory) GaugeVec(_ Opts, _ []string) GaugeVec { return noopGaugeVec{} } +func (NoopFactory) Histogram(_ Opts) Histogram { return noopHistogram{} } +func (NoopFactory) HistogramVec(_ Opts, _ []string) HistogramVec { return noopHistogramVec{} } +func (NoopFactory) Close() error { return nil } + +type ( + noopCounter struct{} + noopCounterVec struct{} + noopGauge struct{} + noopGaugeVec struct{} + noopHistogram struct{} + noopHistogramVec struct{} +) + +func (noopCounter) Inc() {} +func (noopCounter) Add(_ float64) {} +func (noopCounterVec) WithLabelValues(_ ...string) Counter { return noopCounter{} } +func (noopGauge) Inc() {} +func (noopGauge) Add(_ float64) {} +func (noopGauge) Set(_ float64) {} +func (noopGaugeVec) WithLabelValues(_ ...string) Gauge { return noopGauge{} } +func (noopHistogram) Observe(_ float64) {} +func (noopHistogramVec) WithLabelValues(_ ...string) Histogram { return noopHistogram{} } diff --git a/internal/metrics/recording_test.go b/internal/metrics/recording_test.go new file mode 100644 index 0000000000..34bc3d9094 --- /dev/null +++ b/internal/metrics/recording_test.go @@ -0,0 +1,154 @@ +package metrics + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestRecordingFactoryRecordsCounterGaugeAndHistogramValues(t *testing.T) { + rf := NewRecordingFactory() + + counter := rf.Counter(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "recording_total", Help: "test counter"}) + counter.Inc() + counter.Add(2.5) + require.InEpsilon(t, 3.5, rf.CounterValue("spicedb", "metrics", "recording_total"), 1e-9) + + gaugeVec := rf.GaugeVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "recording_current", Help: "test gauge"}, []string{"kind"}) + gauge := gaugeVec.WithLabelValues("check") + gauge.Set(4) + gauge.Add(1.5) + gauge.Inc() + require.InEpsilon(t, 6.5, rf.GaugeVecValue("spicedb", "metrics", "recording_current", "check"), 1e-9) + + histogramVec := rf.HistogramVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "recording_seconds", Help: "test histogram", Buckets: []float64{0.1, 1, 10}}, []string{"kind"}) + histogram := histogramVec.WithLabelValues("check") + histogram.Observe(0.5) + histogram.Observe(1.5) + require.Equal(t, uint64(2), rf.HistogramVecCount("spicedb", "metrics", "recording_seconds", "check")) + require.InEpsilon(t, 2.0, rf.HistogramVecSum("spicedb", "metrics", "recording_seconds", "check"), 1e-9) +} + +func TestRecordingFactoryGaugeAndHistogramAndCounterVec(t *testing.T) { + rf := NewRecordingFactory() + + // Gauge (non-vec) + gauge := rf.Gauge(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "gauge_val", Help: "test gauge"}) + gauge.Inc() + gauge.Add(2) + gauge.Set(10) + require.InEpsilon(t, 10.0, rf.GaugeValue("spicedb", "metrics", "gauge_val"), 1e-9) + + // Histogram (non-vec) + hist := rf.Histogram(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "hist_seconds", Help: "test histogram"}) + hist.Observe(1.0) + hist.Observe(2.0) + require.Equal(t, uint64(2), rf.HistogramCount("spicedb", "metrics", "hist_seconds")) + require.InEpsilon(t, 3.0, rf.HistogramSum("spicedb", "metrics", "hist_seconds"), 1e-9) + + // CounterVec + cv := rf.CounterVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "cv_total", Help: "test counter vec"}, []string{"op"}) + cv.WithLabelValues("read").Inc() + cv.WithLabelValues("read").Add(4) + cv.WithLabelValues("write").Add(7) + require.InEpsilon(t, 5.0, rf.CounterVecValue("spicedb", "metrics", "cv_total", "read"), 1e-9) + require.InEpsilon(t, 7.0, rf.CounterVecValue("spicedb", "metrics", "cv_total", "write"), 1e-9) +} + +func TestRecordingFactoryReturnsZeroForNeverCreatedMetrics(t *testing.T) { + rf := NewRecordingFactory() + + require.Equal(t, 0.0, rf.CounterValue("a", "b", "c")) + require.Equal(t, 0.0, rf.CounterVecValue("a", "b", "c", "x")) + require.Equal(t, 0.0, rf.GaugeValue("a", "b", "c")) + require.Equal(t, 0.0, rf.GaugeVecValue("a", "b", "c", "x")) + require.Equal(t, uint64(0), rf.HistogramCount("a", "b", "c")) + require.Equal(t, 0.0, rf.HistogramSum("a", "b", "c")) + require.Equal(t, uint64(0), rf.HistogramVecCount("a", "b", "c", "x")) + require.Equal(t, 0.0, rf.HistogramVecSum("a", "b", "c", "x")) +} + +func TestRecordingFactoryIdempotentLookup(t *testing.T) { + rf := NewRecordingFactory() + + opts := Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "idem_total", Help: "test"} + + // Creating the same metric twice returns the same underlying instance. + c1 := rf.Counter(opts) + c1.Add(1) + c2 := rf.Counter(opts) + c2.Add(2) + require.InEpsilon(t, 3.0, rf.CounterValue("spicedb", "metrics", "idem_total"), 1e-9) + + // Same for GaugeVec. + gv1 := rf.GaugeVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "idem_gauge", Help: "test"}, []string{"k"}) + gv1.WithLabelValues("a").Set(5) + gv2 := rf.GaugeVec(Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "idem_gauge", Help: "test"}, []string{"k"}) + gv2.WithLabelValues("a").Add(3) + require.InEpsilon(t, 8.0, rf.GaugeVecValue("spicedb", "metrics", "idem_gauge", "a"), 1e-9) +} + +func TestRecordingFactoryExistingAndMissingVecLabelPaths(t *testing.T) { + rf := NewRecordingFactory() + + // Re-acquire gauge and histogram metrics to exercise the existing-instance branches. + gaugeOpts := Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "existing_gauge", Help: "test"} + g1 := rf.Gauge(gaugeOpts) + g2 := rf.Gauge(gaugeOpts) + require.Same(t, g1, g2) + + histOpts := Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "existing_hist", Help: "test"} + h1 := rf.Histogram(histOpts) + h2 := rf.Histogram(histOpts) + require.Same(t, h1, h2) + + histVecOpts := Opts{Namespace: "spicedb", Subsystem: "metrics", Name: "existing_hist_vec", Help: "test"} + hv1 := rf.HistogramVec(histVecOpts, []string{"kind"}) + hv2 := rf.HistogramVec(histVecOpts, []string{"kind"}) + require.Same(t, hv1, hv2) + + // Reuse an existing child and ensure missing labels on existing vecs return zero. + hv1.WithLabelValues("present").Observe(3) + hv2.WithLabelValues("present").Observe(2) + require.Equal(t, uint64(2), rf.HistogramVecCount("spicedb", "metrics", "existing_hist_vec", "present")) + require.InEpsilon(t, 5.0, rf.HistogramVecSum("spicedb", "metrics", "existing_hist_vec", "present"), 1e-9) + + // These calls exercise the "vec exists but label does not" zero paths. + require.Equal(t, 0.0, rf.CounterVecValue("spicedb", "metrics", "cv_total", "missing")) + require.Equal(t, 0.0, rf.GaugeVecValue("spicedb", "metrics", "idem_gauge", "missing")) + require.Equal(t, uint64(0), rf.HistogramVecCount("spicedb", "metrics", "existing_hist_vec", "missing")) + require.Equal(t, 0.0, rf.HistogramVecSum("spicedb", "metrics", "existing_hist_vec", "missing")) +} + +func TestRecordingFactoryClose(t *testing.T) { + rf := NewRecordingFactory() + require.NoError(t, rf.Close()) +} + +func TestNoopFactoryAllMethods(t *testing.T) { + nf := NoopFactory{} + + // All methods must be callable and not panic. + c := nf.Counter(Opts{Name: "noop_c"}) + c.Inc() + c.Add(1) + + cv := nf.CounterVec(Opts{Name: "noop_cv"}, []string{"k"}) + cv.WithLabelValues("v").Inc() + + g := nf.Gauge(Opts{Name: "noop_g"}) + g.Inc() + g.Add(1) + g.Set(0) + + gv := nf.GaugeVec(Opts{Name: "noop_gv"}, []string{"k"}) + gv.WithLabelValues("v").Set(1) + + h := nf.Histogram(Opts{Name: "noop_h"}) + h.Observe(1) + + hv := nf.HistogramVec(Opts{Name: "noop_hv"}, []string{"k"}) + hv.WithLabelValues("v").Observe(1) + + require.NoError(t, nf.Close()) +} diff --git a/internal/metrics/register.go b/internal/metrics/register.go new file mode 100644 index 0000000000..d951400413 --- /dev/null +++ b/internal/metrics/register.go @@ -0,0 +1,27 @@ +package metrics + +import ( + "errors" + + "github.com/prometheus/client_golang/prometheus" +) + +// RegisterOrReuse registers c with r. If c has already been registered, +// the existing collector is returned. +func RegisterOrReuse(r prometheus.Registerer, c prometheus.Collector) (prometheus.Collector, error) { + if err := r.Register(c); err != nil { + if are, ok := errors.AsType[prometheus.AlreadyRegisteredError](err); ok { + return are.ExistingCollector, nil + } + return nil, err + } + return c, nil +} + +// MustRegisterOrReuse registers c with r, panicking only on non-duplicate +// registration errors. +func MustRegisterOrReuse(r prometheus.Registerer, c prometheus.Collector) { + if _, err := RegisterOrReuse(r, c); err != nil { + panic(err) + } +} diff --git a/internal/metrics/register_test.go b/internal/metrics/register_test.go new file mode 100644 index 0000000000..c894e82872 --- /dev/null +++ b/internal/metrics/register_test.go @@ -0,0 +1,76 @@ +package metrics + +import ( + "errors" + "testing" + + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRegisterOrReuseReturnsExistingCollectorOnDuplicate(t *testing.T) { + registry := prometheus.NewRegistry() + + first := prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: "spicedb", + Subsystem: "metrics", + Name: "register_or_reuse_total", + Help: "test counter", + }) + + registeredFirst, err := RegisterOrReuse(registry, first) + require.NoError(t, err) + require.Same(t, first, registeredFirst) + + duplicate := prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: "spicedb", + Subsystem: "metrics", + Name: "register_or_reuse_total", + Help: "test counter", + }) + + registeredDuplicate, err := RegisterOrReuse(registry, duplicate) + require.NoError(t, err) + require.Same(t, registeredFirst, registeredDuplicate) +} + +func TestMustRegisterOrReuse(t *testing.T) { + t.Run("does not panic on duplicate registration", func(t *testing.T) { + registry := prometheus.NewRegistry() + + first := prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: "spicedb", + Subsystem: "metrics", + Name: "must_register_or_reuse_total", + Help: "test counter", + }) + MustRegisterOrReuse(registry, first) + + duplicate := prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: "spicedb", + Subsystem: "metrics", + Name: "must_register_or_reuse_total", + Help: "test counter", + }) + + assert.NotPanics(t, func() { + MustRegisterOrReuse(registry, duplicate) + }) + }) + + t.Run("panics on non-duplicate registration error", func(t *testing.T) { + assert.Panics(t, func() { + MustRegisterOrReuse(failingRegisterer{}, prometheus.NewCounter(prometheus.CounterOpts{ + Name: "failing_registerer_total", + Help: "test counter", + })) + }) + }) +} + +type failingRegisterer struct{} + +func (failingRegisterer) Register(prometheus.Collector) error { return errors.New("register failed") } +func (failingRegisterer) MustRegister(...prometheus.Collector) {} +func (failingRegisterer) Unregister(prometheus.Collector) bool { return false } diff --git a/internal/middleware/memoryprotection/memory_protection.go b/internal/middleware/memoryprotection/memory_protection.go index b7b6312256..b032b2cd51 100644 --- a/internal/middleware/memoryprotection/memory_protection.go +++ b/internal/middleware/memoryprotection/memory_protection.go @@ -7,33 +7,41 @@ import ( "strings" middleware "github.com/grpc-ecosystem/go-grpc-middleware/v2" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" "go.opentelemetry.io/otel" "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" log "github.com/authzed/spicedb/internal/logging" + "github.com/authzed/spicedb/internal/metrics" ) var tracer = otel.Tracer("spicedb/internal/middleware/memory_protection") -// RequestsProcessed tracks requests that were processed by this middleware. -var RequestsProcessed = promauto.NewCounterVec(prometheus.CounterOpts{ +var requestsProcessedOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "memory_middleware", Name: "requests_processed_total", Help: "Total requests processed by the memory protection middleware (flag --memory-protection-enabled)", -}, []string{"endpoint", "accepted"}) +} type MemoryProtectionMiddleware struct { currentMemoryUsageProvider MemoryUsageProvider + requestsProcessed metrics.CounterVec } func New(usageProvider MemoryUsageProvider, name string) *MemoryProtectionMiddleware { + return NewWithMetricsFactory(usageProvider, name, metrics.NewPrometheusFactory(nil)) +} + +func NewWithMetricsFactory(usageProvider MemoryUsageProvider, name string, factory metrics.Factory) *MemoryProtectionMiddleware { + if factory == nil { + factory = metrics.NoopFactory{} + } + am := MemoryProtectionMiddleware{ currentMemoryUsageProvider: usageProvider, + requestsProcessed: factory.CounterVec(requestsProcessedOpts, []string{"endpoint", "accepted"}), } log.Info(). @@ -94,6 +102,6 @@ func (am *MemoryProtectionMiddleware) recordMetric(fullMethod string, accepted b acceptedStr := strconv.FormatBool(accepted) - RequestsProcessed.WithLabelValues(endpointType, acceptedStr).Inc() + am.requestsProcessed.WithLabelValues(endpointType, acceptedStr).Inc() return endpointType } diff --git a/internal/middleware/memoryprotection/memory_protection_test.go b/internal/middleware/memoryprotection/memory_protection_test.go index 712fa7f410..b9a2df204d 100644 --- a/internal/middleware/memoryprotection/memory_protection_test.go +++ b/internal/middleware/memoryprotection/memory_protection_test.go @@ -7,14 +7,14 @@ import ( "testing" "github.com/grpc-ecosystem/go-grpc-middleware/v2/testing/testpb" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/testutil" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "go.uber.org/goleak" "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + + "github.com/authzed/spicedb/internal/metrics" ) func TestNew(t *testing.T) { @@ -63,28 +63,11 @@ func TestMemoryProtectionMiddleware_RecordRejection(t *testing.T) { goleak.VerifyNone(t) }) - // Create a custom registry for testing - registry := prometheus.NewRegistry() - - // Register our metrics with the test registry - testRequestsProcessed := prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: "spicedb", - Subsystem: "memory_middleware", - Name: "requests_processed_total", - Help: "Total requests processed by the memory protection middleware (flag --memory-protection-enabled)", - }, []string{"endpoint", "accepted"}) - registry.MustRegister(testRequestsProcessed) - - // Replace the global counter with our test counter for this test - originalCounter := RequestsProcessed - RequestsProcessed = testRequestsProcessed - defer func() { - RequestsProcessed = originalCounter - }() + rf := metrics.NewRecordingFactory() lp := &HarcodedMemoryUsageProvider{AcceptAllRequests: true} - am := New(lp, "test") + am := NewWithMetricsFactory(lp, "test", rf) // Test API endpoint endpointType := am.recordMetric("/authzed.api.v1.PermissionsService/CheckPermission", true) @@ -94,11 +77,11 @@ func TestMemoryProtectionMiddleware_RecordRejection(t *testing.T) { endpointType = am.recordMetric("/dispatch.v1.DispatchService/DispatchCheck", false) require.Equal(t, "dispatch", endpointType) - gaugeValue := testutil.ToFloat64(testRequestsProcessed.WithLabelValues("api", "true")) - require.Equal(t, float64(1), gaugeValue) //nolint:testifylint // these values aren't being operated on + apiAcceptedCount := rf.CounterVecValue("spicedb", "memory_middleware", "requests_processed_total", "api", "true") + require.Equal(t, float64(1), apiAcceptedCount) //nolint:testifylint // these values aren't being operated on - gaugeValue = testutil.ToFloat64(testRequestsProcessed.WithLabelValues("dispatch", "false")) - require.Equal(t, float64(1), gaugeValue) //nolint:testifylint // these values aren't being operated on + dispatchRejectedCount := rf.CounterVecValue("spicedb", "memory_middleware", "requests_processed_total", "dispatch", "false") + require.Equal(t, float64(1), dispatchRejectedCount) //nolint:testifylint // these values aren't being operated on } type memoryProtectionTestServer struct { diff --git a/internal/middleware/perfinsights/perfinsights.go b/internal/middleware/perfinsights/perfinsights.go index 0eef6229b8..d3a9079cc3 100644 --- a/internal/middleware/perfinsights/perfinsights.go +++ b/internal/middleware/perfinsights/perfinsights.go @@ -9,7 +9,6 @@ import ( "github.com/ccoveille/go-safecast/v2" "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors" "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" "github.com/rs/zerolog/log" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/trace" @@ -18,6 +17,7 @@ import ( "github.com/authzed/ctxkey" "github.com/authzed/grpcutil" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/spiceerrors" ) @@ -61,14 +61,29 @@ func NoLabels() APIShapeLabels { // // 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{ +var apiShapeLatencyOpts = metrics.Opts{ 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 APIShapeLatency metrics.HistogramVec + +func init() { + SetMetricsFactory(metrics.NewPrometheusFactory(nil)) +} + +// SetMetricsFactory configures which metrics factory is used by this package's +// interceptors and exported latency histogram. +func SetMetricsFactory(factory metrics.Factory) { + if factory == nil { + factory = metrics.NoopFactory{} + } + APIShapeLatency = factory.HistogramVec(apiShapeLatencyOpts, append([]string{"api_kind"}, allLabels...)) +} var tracer = otel.Tracer("spicedb/internal/middleware") @@ -81,7 +96,7 @@ func ObserveShapeLatency(ctx context.Context, methodName string, shape APIShapeL observeShapeLatency(ctx, APIShapeLatency, methodName, shape, duration) } -func observeShapeLatency(ctx context.Context, metric *prometheus.HistogramVec, methodName string, shape APIShapeLabels, duration time.Duration) { +func observeShapeLatency(ctx context.Context, metric metrics.HistogramVec, methodName string, shape APIShapeLabels, duration time.Duration) { ctx, span := tracer.Start(ctx, "perfInsights.observeShapeLatency") defer span.End() labels := buildLabels(methodName, shape) diff --git a/internal/middleware/perfinsights/perfinsights_test.go b/internal/middleware/perfinsights/perfinsights_test.go index 530434d300..b92724071f 100644 --- a/internal/middleware/perfinsights/perfinsights_test.go +++ b/internal/middleware/perfinsights/perfinsights_test.go @@ -16,6 +16,8 @@ import ( "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/metadata" "google.golang.org/grpc/test/bufconn" + + "github.com/authzed/spicedb/internal/metrics" ) func TestBuildLabels(t *testing.T) { @@ -65,8 +67,8 @@ func TestBuildLabels(t *testing.T) { func TestObserveShapeLatency(t *testing.T) { reg := prometheus.NewRegistry() - - metric := prometheus.NewHistogramVec(prometheus.HistogramOpts{ + factory := metrics.NewPrometheusFactory(reg) + metric := factory.HistogramVec(metrics.Opts{ Namespace: "spicedb", Subsystem: "perf_insights", Name: "api_shape_latency_seconds", @@ -74,7 +76,6 @@ func TestObserveShapeLatency(t *testing.T) { Buckets: nil, NativeHistogramBucketFactor: 1.1, }, append([]string{"api_kind"}, allLabels...)) - require.NoError(t, reg.Register(metric)) // Report some data. observeShapeLatency(t.Context(), metric, "testMethod", APIShapeLabels{ diff --git a/internal/middleware/usagemetrics/usagemetrics.go b/internal/middleware/usagemetrics/usagemetrics.go index 3f0e6fbe9a..84a9f07b75 100644 --- a/internal/middleware/usagemetrics/usagemetrics.go +++ b/internal/middleware/usagemetrics/usagemetrics.go @@ -8,7 +8,6 @@ import ( "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors" "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" "go.opentelemetry.io/otel" "google.golang.org/grpc" @@ -17,6 +16,7 @@ import ( "github.com/authzed/grpcutil" log "github.com/authzed/spicedb/internal/logging" + "github.com/authzed/spicedb/internal/metrics" dispatch "github.com/authzed/spicedb/pkg/proto/dispatch/v1" ) @@ -28,17 +28,40 @@ var ( // DispatchedCountHistogram is the metric that SpiceDB uses to keep track // of the number of downstream dispatches that are performed to answer a // single query. - DispatchedCountHistogram = promauto.NewHistogramVec(prometheus.HistogramOpts{ + DispatchedCountHistogram metrics.HistogramVec + + dispatchedCountOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "services", Name: "dispatches", Help: "Histogram of cluster dispatches performed by the instance.", Buckets: []float64{1, 5, 10, 25, 50, 100, 250}, - }, DispatchedCountLabels) + } tracer = otel.Tracer("spicedb/internal/middleware") ) +func init() { + SetMetricsFactory(metrics.NewPrometheusFactory(nil)) +} + +// SetMetricsFactory configures which metrics factory is used by this package's +// interceptors and exported histogram. +func SetMetricsFactory(factory metrics.Factory) { + if factory == nil { + factory = metrics.NoopFactory{} + } + DispatchedCountHistogram = factory.HistogramVec(dispatchedCountOpts, DispatchedCountLabels) +} + +// CollectDispatchedCountMetrics collects the dispatched-count histogram metrics +// when the configured histogram is backed by Prometheus. +func CollectDispatchedCountMetrics(ch chan<- prometheus.Metric) { + if collector, ok := metrics.AsPrometheusHistogramVec(DispatchedCountHistogram); ok { + collector.Collect(ch) + } +} + type reporter struct{} func (r *reporter) ServerReporter(ctx context.Context, callMeta interceptors.CallMeta) (interceptors.Reporter, context.Context) { diff --git a/internal/services/integrationtesting/benchmark_dispatchqueryplan_test.go b/internal/services/integrationtesting/benchmark_dispatchqueryplan_test.go index 7cfb93a9a3..c3c42bb287 100644 --- a/internal/services/integrationtesting/benchmark_dispatchqueryplan_test.go +++ b/internal/services/integrationtesting/benchmark_dispatchqueryplan_test.go @@ -180,7 +180,7 @@ func (h *dispatchQueryPlanHandle) newCachingDispatcher(b *testing.B) *caching.Di c, err := cache.NewStandardCache[keys.DispatchCacheKey, any](cacheConfig) require.NoError(b, err) - cd, err := caching.NewCachingDispatcher(c, false, "bench", &keys.DirectKeyHandler{}) + cd, err := caching.NewCachingDispatcher(c, nil, "bench", &keys.DirectKeyHandler{}) require.NoError(b, err) lpd := &localQueryPlanDispatcher{handle: h} diff --git a/internal/services/integrationtesting/consistencytestutil/clusteranddata.go b/internal/services/integrationtesting/consistencytestutil/clusteranddata.go index 8cd8ec47bb..587adb2854 100644 --- a/internal/services/integrationtesting/consistencytestutil/clusteranddata.go +++ b/internal/services/integrationtesting/consistencytestutil/clusteranddata.go @@ -75,7 +75,7 @@ func CreateDispatcherForTesting(t *testing.T, withCaching bool) dispatch.Dispatc dispatcher, err := graph.NewLocalOnlyDispatcher(params) require.NoError(err) if withCaching { - cachingDispatcher, err := caching.NewCachingDispatcher(nil, false, "", &keys.CanonicalKeyHandler{}) + cachingDispatcher, err := caching.NewCachingDispatcher(nil, nil, "", &keys.CanonicalKeyHandler{}) require.NoError(err) params2, err := graph.NewDefaultDispatcherParametersForTesting() diff --git a/internal/services/integrationtesting/queryconsistency/query_plan_grpc_test.go b/internal/services/integrationtesting/queryconsistency/query_plan_grpc_test.go index 0d64368566..a8287f6620 100644 --- a/internal/services/integrationtesting/queryconsistency/query_plan_grpc_test.go +++ b/internal/services/integrationtesting/queryconsistency/query_plan_grpc_test.go @@ -41,7 +41,9 @@ func TestQueryPlanConsistencyGRPC(t *testing.T) { // nolint:tparallel for _, filePath := range consistencyTestFiles { t.Run(path.Base(filePath), func(t *testing.T) { - t.Parallel() + // Each subtest spins up a full server via Config.Complete, which mutates + // package-level metrics singletons in several packages. Running these in + // parallel triggers races on those globals. runQueryPlanConsistencyGRPCForFile(t, filePath) }) } diff --git a/internal/services/v1/relationships.go b/internal/services/v1/relationships.go index a465ae5299..4026e31c18 100644 --- a/internal/services/v1/relationships.go +++ b/internal/services/v1/relationships.go @@ -9,8 +9,6 @@ import ( "buf.build/go/protovalidate" grpcvalidate "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/protovalidate" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" "go.opentelemetry.io/otel/trace" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/structpb" @@ -18,6 +16,7 @@ import ( v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" "github.com/authzed/spicedb/internal/dispatch" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/internal/middleware" "github.com/authzed/spicedb/internal/middleware/handwrittenvalidation" "github.com/authzed/spicedb/internal/middleware/interceptorwrapper" @@ -45,13 +44,28 @@ import ( "github.com/authzed/spicedb/pkg/zedtoken" ) -var writeUpdateCounter = promauto.NewHistogramVec(prometheus.HistogramOpts{ +var writeUpdateCounter metrics.HistogramVec + +var writeUpdateCounterOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "v1", Name: "write_relationships_updates", Help: "The update counts for the WriteRelationships calls", Buckets: []float64{0, 1, 2, 5, 10, 15, 25, 50, 100, 250, 500, 1000}, -}, []string{"kind"}) +} + +func init() { + SetMetricsFactory(metrics.NewPrometheusFactory(nil)) +} + +// SetMetricsFactory configures which metrics factory is used by this package's +// service-level metrics. +func SetMetricsFactory(factory metrics.Factory) { + if factory == nil { + factory = metrics.NoopFactory{} + } + writeUpdateCounter = factory.HistogramVec(writeUpdateCounterOpts, []string{"kind"}) +} const MaximumTransactionMetadataSize = 65000 // bytes. Limited by the BLOB size used in MySQL driver diff --git a/internal/telemetry/metrics.go b/internal/telemetry/metrics.go index 82a3d2c7f9..586bc93a5a 100644 --- a/internal/telemetry/metrics.go +++ b/internal/telemetry/metrics.go @@ -8,26 +8,58 @@ import ( "runtime" "runtime/debug" "strconv" + "sync" "time" "github.com/jzelinskie/cobrautil/v2" "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" dto "github.com/prometheus/client_model/go" "golang.org/x/sync/errgroup" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/internal/middleware/usagemetrics" "github.com/authzed/spicedb/pkg/datastore" "github.com/authzed/spicedb/pkg/promutil" ) -var LogicalChecks = promauto.NewCounter(prometheus.CounterOpts{ +var logicalChecksCounterOpts = internalmetrics.Opts{ Namespace: "spicedb", Subsystem: "services", Name: "logical_checks_total", Help: `Count of the number of "checks" made across all APIs (e.g. each item within a CheckBulk, each item returned from a Lookup).`, -}) +} + +var ( + logicalChecksMu sync.Mutex + LogicalChecks internalmetrics.Counter // GUARDED_BY(logicalChecksMu) + logicalChecksMetric prometheus.Metric // GUARDED_BY(logicalChecksMu) +) + +func init() { + SetMetricsFactory(internalmetrics.NewPrometheusFactory(nil)) +} + +// SetMetricsFactory configures which metrics factory is used by this package's +// logical checks counter. +func SetMetricsFactory(factory internalmetrics.Factory) { + if factory == nil { + factory = internalmetrics.NoopFactory{} + } + + logicalChecksMu.Lock() + defer logicalChecksMu.Unlock() + + LogicalChecks = factory.Counter(logicalChecksCounterOpts) + + metric, ok := LogicalChecks.(prometheus.Metric) + if !ok { + logicalChecksMetric = nil + return + } + + logicalChecksMetric = metric +} func SpiceDBClusterInfoCollector(ctx context.Context, subsystem, dsEngine string, ds datastore.Datastore) (promutil.CollectorFunc, error) { nodeID, err := os.Hostname() @@ -75,31 +107,39 @@ func RegisterTelemetryCollector(datastoreEngine string, ds datastore.Datastore) func registerTelemetryCollector(datastoreEngine string, ds datastore.Datastore) (*prometheus.Registry, *collector, error) { registry := prometheus.NewRegistry() + collector, err := registerTelemetryCollectorWithRegistry(registry, datastoreEngine, ds) + if err != nil { + return nil, nil, err + } + + return registry, collector, nil +} + +func registerTelemetryCollectorWithRegistry(registry *prometheus.Registry, datastoreEngine string, ds datastore.Datastore) (*collector, error) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() infoCollector, err := SpiceDBClusterInfoCollector(ctx, "telemetry", datastoreEngine, ds) if err != nil { - return nil, nil, fmt.Errorf("unable create info collector: %w", err) + return nil, fmt.Errorf("unable create info collector: %w", err) } - if err := registry.Register(infoCollector); err != nil { - return nil, nil, fmt.Errorf("unable to register telemetry collector: %w", err) + if _, err := internalmetrics.RegisterOrReuse(registry, infoCollector); err != nil { + return nil, fmt.Errorf("unable to register telemetry collector: %w", err) } nodeID, err := os.Hostname() if err != nil { - return nil, nil, fmt.Errorf("unable to get hostname: %w", err) + return nil, fmt.Errorf("unable to get hostname: %w", err) } dbStats, err := ds.Statistics(ctx) if err != nil { - return nil, nil, fmt.Errorf("unable to query DB stats: %w", err) + return nil, fmt.Errorf("unable to query DB stats: %w", err) } clusterID := dbStats.UniqueID - collector := &collector{ - ds: ds, + telemetryCollector := &collector{ objectDefsDesc: prometheus.NewDesc( prometheus.BuildFQName("spicedb", "telemetry", "object_definitions_total"), "Count of the number of objects defined by the schema.", @@ -137,11 +177,18 @@ func registerTelemetryCollector(datastoreEngine string, ds datastore.Datastore) }, ), } - if err := registry.Register(collector); err != nil { - return nil, collector, fmt.Errorf("unable to register telemetry collector: %w", err) + + registeredCollector, err := internalmetrics.RegisterOrReuse(registry, telemetryCollector) + if err != nil { + return telemetryCollector, fmt.Errorf("unable to register telemetry collector: %w", err) } - return registry, collector, nil + existingCollector, ok := registeredCollector.(*collector) + if ok { + telemetryCollector = existingCollector + } + + return telemetryCollector, nil } type collector struct { @@ -165,14 +212,26 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() - dsStats, err := c.ds.Statistics(ctx) - if err != nil { - log.Warn().Err(err).Msg("unable to collect datastore statistics") + dsStats := datastore.Stats{} + if c.ds != nil { + stats, err := c.ds.Statistics(ctx) + if err != nil { + log.Warn().Err(err).Msg("unable to collect datastore statistics") + } else { + dsStats = stats + } } ch <- prometheus.MustNewConstMetric(c.objectDefsDesc, prometheus.GaugeValue, float64(len(dsStats.ObjectTypeStatistics))) ch <- prometheus.MustNewConstMetric(c.relationshipsDesc, prometheus.GaugeValue, float64(dsStats.EstimatedRelationshipCount)) - ch <- prometheus.MustNewConstMetric(c.logicalChecksDec, prometheus.CounterValue, promutil.MustCounterValue(LogicalChecks)) + logicalChecksValue := 0.0 + logicalChecksMu.Lock() + metric := logicalChecksMetric + logicalChecksMu.Unlock() + if metric != nil { + logicalChecksValue = promutil.MustCounterValue(metric) + } + ch <- prometheus.MustNewConstMetric(c.logicalChecksDec, prometheus.CounterValue, logicalChecksValue) dispatchedCountMetrics := make(chan prometheus.Metric) g := errgroup.Group{} @@ -207,7 +266,7 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { return nil }) - usagemetrics.DispatchedCountHistogram.Collect(dispatchedCountMetrics) + usagemetrics.CollectDispatchedCountMetrics(dispatchedCountMetrics) close(dispatchedCountMetrics) if err := g.Wait(); err != nil { diff --git a/internal/telemetry/metrics_test.go b/internal/telemetry/metrics_test.go index b291f5cc75..06f19361d0 100644 --- a/internal/telemetry/metrics_test.go +++ b/internal/telemetry/metrics_test.go @@ -1,6 +1,7 @@ package telemetry import ( + "errors" "testing" "time" @@ -8,6 +9,9 @@ import ( "github.com/stretchr/testify/require" "github.com/authzed/spicedb/internal/datastore/dsfortesting" + proxymock "github.com/authzed/spicedb/internal/datastore/proxy/proxy_test" + internalmetrics "github.com/authzed/spicedb/internal/metrics" + "github.com/authzed/spicedb/pkg/datastore" ) func TestCollect(t *testing.T) { @@ -103,3 +107,152 @@ outer: require.Equal(t, 4, descCount, "Expected four metric descriptions") } + +func TestRegisterTelemetryCollectorRepeatedOnSameRegistry(t *testing.T) { + ds, err := dsfortesting.NewMemDBDatastoreForTesting(t, 100, 10*time.Hour, 10*time.Hour) + require.NoError(t, err) + + registry := prometheus.NewRegistry() + + firstCollector, err := registerTelemetryCollectorWithRegistry(registry, "memdb", ds) + require.NoError(t, err) + require.NotNil(t, firstCollector) + + secondCollector, err := registerTelemetryCollectorWithRegistry(registry, "memdb", ds) + require.NoError(t, err) + require.NotNil(t, secondCollector) + + // Re-registration should reuse the existing collector instance. + require.Same(t, firstCollector, secondCollector) + + metricFamilies, err := registry.Gather() + require.NoError(t, err) + require.NotEmpty(t, metricFamilies) +} + +func TestCollectWithRecordingFactory(t *testing.T) { + SetMetricsFactory(internalmetrics.NewRecordingFactory()) + t.Cleanup(func() { + SetMetricsFactory(internalmetrics.NewPrometheusFactory(nil)) + }) + + ds, err := dsfortesting.NewMemDBDatastoreForTesting(t, 100, 10*time.Hour, 10*time.Hour) + require.NoError(t, err) + + _, c, err := registerTelemetryCollector("memdb", ds) + require.NoError(t, err) + + ch := make(chan prometheus.Metric, 100) + require.NotPanics(t, func() { + c.Collect(ch) + }) +} + +func TestSetMetricsFactoryWithNoopFactory(t *testing.T) { + original := internalmetrics.NewPrometheusFactory(nil) + t.Cleanup(func() { + SetMetricsFactory(original) + }) + + SetMetricsFactory(internalmetrics.NoopFactory{}) + + ds, err := dsfortesting.NewMemDBDatastoreForTesting(t, 100, 10*time.Hour, 10*time.Hour) + require.NoError(t, err) + + _, c, err := registerTelemetryCollector("memdb", ds) + require.NoError(t, err) + + ch := make(chan prometheus.Metric, 100) + require.NotPanics(t, func() { + c.Collect(ch) + }) +} + +func TestSetMetricsFactoryWithNil(t *testing.T) { + original := internalmetrics.NewPrometheusFactory(nil) + t.Cleanup(func() { + SetMetricsFactory(original) + }) + + // Passing nil should default to NoopFactory + SetMetricsFactory(nil) + + ds, err := dsfortesting.NewMemDBDatastoreForTesting(t, 100, 10*time.Hour, 10*time.Hour) + require.NoError(t, err) + + _, c, err := registerTelemetryCollector("memdb", ds) + require.NoError(t, err) + + ch := make(chan prometheus.Metric, 100) + require.NotPanics(t, func() { + c.Collect(ch) + }) +} + +func TestRegisterTelemetryCollectorWithRegistryStatisticsError(t *testing.T) { + ds := &proxymock.MockDatastore{} + ds.On("Statistics").Return(datastore.Stats{}, errors.New("stats unavailable")).Once() + + registry := prometheus.NewRegistry() + collector, err := registerTelemetryCollectorWithRegistry(registry, "mock", ds) + require.Error(t, err) + require.Nil(t, collector) + require.Contains(t, err.Error(), "unable create info collector") + + ds.AssertExpectations(t) +} + +func TestCollectWithDatastoreStatisticsErrorUsesZeroStats(t *testing.T) { + ds := &proxymock.MockDatastore{} + ds.On("Statistics").Return(datastore.Stats{UniqueID: "cluster-1"}, nil).Twice() + + registry := prometheus.NewRegistry() + collector, err := registerTelemetryCollectorWithRegistry(registry, "mock", ds) + require.NoError(t, err) + require.NotNil(t, collector) + + collector.ds = ds + ds.On("Statistics").Return(datastore.Stats{}, errors.New("stats unavailable during collect")).Once() + + ch := make(chan prometheus.Metric, 100) + require.NotPanics(t, func() { + collector.Collect(ch) + }) + + // Collect should still emit metrics even if datastore statistics fail. + count := 0 + for { + select { + case metric := <-ch: + require.NotNil(t, metric) + count++ + default: + require.GreaterOrEqual(t, count, 3) + ds.AssertExpectations(t) + return + } + } +} + +func TestCollectUsesNilDatastoreWithoutPanicking(t *testing.T) { + ds, err := dsfortesting.NewMemDBDatastoreForTesting(t, 100, 10*time.Hour, 10*time.Hour) + require.NoError(t, err) + + _, collector, err := registerTelemetryCollector("memdb", ds) + require.NoError(t, err) + + collector.ds = nil + + ch := make(chan prometheus.Metric, 100) + require.NotPanics(t, func() { + collector.Collect(ch) + }) + + // Ensure something was emitted on the nil-datastore path. + select { + case metric := <-ch: + require.NotNil(t, metric) + default: + t.Fatal("expected at least one collected metric") + } +} diff --git a/pkg/cache/metrics.go b/pkg/cache/metrics.go index b9b110b233..71475612f4 100644 --- a/pkg/cache/metrics.go +++ b/pkg/cache/metrics.go @@ -5,10 +5,12 @@ import ( "github.com/jzelinskie/stringz" "github.com/prometheus/client_golang/prometheus" + + internalmetrics "github.com/authzed/spicedb/internal/metrics" ) func init() { - prometheus.MustRegister(defaultCollector) + internalmetrics.MustRegisterOrReuse(prometheus.DefaultRegisterer, defaultCollector) } const ( diff --git a/pkg/cmd/datastore/datastore.go b/pkg/cmd/datastore/datastore.go index 9c3c3c251c..2980870a6c 100644 --- a/pkg/cmd/datastore/datastore.go +++ b/pkg/cmd/datastore/datastore.go @@ -10,6 +10,7 @@ import ( "time" "github.com/ccoveille/go-safecast/v2" + "github.com/prometheus/client_golang/prometheus" "github.com/spf13/pflag" "github.com/authzed/spicedb/internal/datastore/common" @@ -20,6 +21,7 @@ import ( "github.com/authzed/spicedb/internal/datastore/proxy" "github.com/authzed/spicedb/internal/datastore/spanner" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/internal/sharederrors" caveattypes "github.com/authzed/spicedb/pkg/caveats/types" "github.com/authzed/spicedb/pkg/datalayer" @@ -114,12 +116,13 @@ type Config struct { FilterMaximumIDCount uint16 `debugmap:"hidden" default:"100"` // Options - ReadConnPool ConnPoolConfig `debugmap:"visible"` - WriteConnPool ConnPoolConfig `debugmap:"visible"` - ReadOnly bool `debugmap:"visible"` - EnableDatastoreMetrics bool `debugmap:"visible"` - DisableStats bool `debugmap:"visible"` - IncludeQueryParametersInTraces bool `debugmap:"visible"` + ReadConnPool ConnPoolConfig `debugmap:"visible"` + WriteConnPool ConnPoolConfig `debugmap:"visible"` + ReadOnly bool `debugmap:"visible"` + EnableDatastoreMetrics bool `debugmap:"visible"` + PrometheusRegisterer prometheus.Registerer `debugmap:"hidden"` + DisableStats bool `debugmap:"visible"` + IncludeQueryParametersInTraces bool `debugmap:"visible"` // Read Replicas ReadReplicaConnPool ConnPoolConfig `debugmap:"visible"` @@ -419,6 +422,8 @@ func NewDatastore(ctx context.Context, options ...ConfigOption) (datastore.Datas o(opts) } + proxy.SetMetricsFactory(internalmetrics.NewPrometheusFactory(opts.PrometheusRegisterer)) + if (opts.Engine == PostgresEngine || opts.Engine == MySQLEngine) && opts.FollowerReadDelay == DefaultFollowerReadDelay { // Set the default follower read delay for postgres and mysql to 0 - // this should only be set if read replicas are used. @@ -600,6 +605,7 @@ func newCRDBDatastore(ctx context.Context, opts Config) (datastore.Datastore, er crdb.WatchBufferWriteTimeout(opts.WatchBufferWriteTimeout), crdb.WatchConnectTimeout(opts.WatchConnectTimeout), crdb.WithEnablePrometheusStats(opts.EnableDatastoreMetrics), + crdb.WithPrometheusRegisterer(opts.PrometheusRegisterer), crdb.WithEnableConnectionBalancing(opts.EnableConnectionBalancing), crdb.ConnectRate(opts.ConnectRate), crdb.FilterMaximumIDCount(opts.FilterMaximumIDCount), @@ -651,6 +657,7 @@ func commonPostgresDatastoreOptions(opts Config) ([]postgres.Option, error) { return []postgres.Option{ postgres.EnableTracing(), postgres.WithEnablePrometheusStats(opts.EnableDatastoreMetrics), + postgres.WithPrometheusRegisterer(opts.PrometheusRegisterer), postgres.MaxRetries(maxRetries), postgres.FilterMaximumIDCount(opts.FilterMaximumIDCount), postgres.WithColumnOptimization(opts.ExperimentalColumnOptimization), @@ -799,6 +806,7 @@ func commonMySQLDatastoreOptions(opts Config) ([]mysql.Option, error) { mysql.MaxRetries(maxRetries), mysql.OverrideLockWaitTimeout(1), mysql.WithEnablePrometheusStats(opts.EnableDatastoreMetrics), + mysql.WithPrometheusRegisterer(opts.PrometheusRegisterer), mysql.WatchBufferLength(opts.WatchBufferLength), mysql.WatchBufferWriteTimeout(opts.WatchBufferWriteTimeout), mysql.WatchChangeBufferMaximumSize(watchChangeBufferMaximumSize), diff --git a/pkg/cmd/datastore/zz_generated.options.go b/pkg/cmd/datastore/zz_generated.options.go index 9d814ebf72..d25afaa945 100644 --- a/pkg/cmd/datastore/zz_generated.options.go +++ b/pkg/cmd/datastore/zz_generated.options.go @@ -5,6 +5,7 @@ import ( "fmt" types "github.com/authzed/spicedb/pkg/caveats/types" defaults "github.com/creasty/defaults" + prometheus "github.com/prometheus/client_golang/prometheus" "time" ) @@ -44,6 +45,7 @@ func (c *Config) ToOption() ConfigOption { to.WriteConnPool = c.WriteConnPool to.ReadOnly = c.ReadOnly to.EnableDatastoreMetrics = c.EnableDatastoreMetrics + to.PrometheusRegisterer = c.PrometheusRegisterer to.DisableStats = c.DisableStats to.IncludeQueryParametersInTraces = c.IncludeQueryParametersInTraces to.ReadReplicaConnPool = c.ReadReplicaConnPool @@ -438,6 +440,13 @@ func WithEnableDatastoreMetrics(enableDatastoreMetrics bool) ConfigOption { } } +// WithPrometheusRegisterer returns an option that can set PrometheusRegisterer on a Config +func WithPrometheusRegisterer(prometheusRegisterer prometheus.Registerer) ConfigOption { + return func(c *Config) { + c.PrometheusRegisterer = prometheusRegisterer + } +} + // WithDisableStats returns an option that can set DisableStats on a Config func WithDisableStats(disableStats bool) ConfigOption { return func(c *Config) { diff --git a/pkg/cmd/datastore_test.go b/pkg/cmd/datastore_test.go index 93831676e3..9b290b4989 100644 --- a/pkg/cmd/datastore_test.go +++ b/pkg/cmd/datastore_test.go @@ -19,7 +19,6 @@ func TestExecuteGC(t *testing.T) { name: "cockroachdb does not support garbage collection", cfgBuilder: func(t *testing.T) *datastore.Config { cfg := datastore.DefaultDatastoreConfig() - cfg.EnableDatastoreMetrics = false // avoid "duplicate metrics collector registration attempted" cfg.Engine = "cockroachdb" runningDatastore := datastoreTest.RunDatastoreEngine(t, cfg.Engine) db := runningDatastore.NewDatabase(t) @@ -49,7 +48,6 @@ func TestExecuteRepair(t *testing.T) { name: "cockroachdb does not support repair", cfgBuilder: func(t *testing.T) *datastore.Config { cfg := datastore.DefaultDatastoreConfig() - cfg.EnableDatastoreMetrics = false // avoid "duplicate metrics collector registration attempted" cfg.Engine = "cockroachdb" runningDatastore := datastoreTest.RunDatastoreEngine(t, cfg.Engine) db := runningDatastore.NewDatabase(t) @@ -62,7 +60,6 @@ func TestExecuteRepair(t *testing.T) { name: "postgres supports repair", cfgBuilder: func(t *testing.T) *datastore.Config { cfg := datastore.DefaultDatastoreConfig() - cfg.EnableDatastoreMetrics = false // avoid "duplicate metrics collector registration attempted" cfg.Engine = "postgres" runningDatastore := datastoreTest.RunDatastoreEngine(t, cfg.Engine) db := runningDatastore.NewDatabase(t) diff --git a/pkg/cmd/server/defaults.go b/pkg/cmd/server/defaults.go index febda4da7b..8e5d7609a6 100644 --- a/pkg/cmd/server/defaults.go +++ b/pkg/cmd/server/defaults.go @@ -6,7 +6,6 @@ import ( "fmt" "net/http" "net/http/pprof" - "sync" "time" "github.com/KimMachineGun/automemlimit/memlimit" @@ -33,6 +32,7 @@ import ( "github.com/authzed/spicedb/internal/dispatch" "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" dispatchmw "github.com/authzed/spicedb/internal/middleware/dispatcher" "github.com/authzed/spicedb/internal/middleware/memoryprotection" "github.com/authzed/spicedb/internal/middleware/servicespecific" @@ -182,10 +182,11 @@ const ( DefaultInternalMiddlewareServerSpecific = "servicespecific" ) -//go:generate go run github.com/ecordell/optgen -output zz_generated.middlewareoption.go . MiddlewareOption +//go:generate go run github.com/ecordell/optgen -prefix -output zz_generated.middlewareoption.go . MiddlewareOption type MiddlewareOption struct { Logger zerolog.Logger `debugmap:"hidden"` AuthFunc grpcauth.AuthFunc `debugmap:"hidden"` + PrometheusRegisterer prometheus.Registerer `debugmap:"hidden"` EnableVersionResponse bool `debugmap:"visible"` DispatcherForMiddleware dispatch.Dispatcher `debugmap:"hidden"` EnableRequestLog bool `debugmap:"visible"` @@ -244,21 +245,9 @@ func (m MiddlewareOption) WithDatastore(ds datastore.Datastore, dlOpts ...datala return m } -// gRPCMetricsUnaryInterceptor creates the default prometheus metrics interceptor for unary gRPCs -var gRPCMetricsUnaryInterceptor grpc.UnaryServerInterceptor - -// gRPCMetricsStreamingInterceptor creates the default prometheus metrics interceptor for streaming gRPCs -var gRPCMetricsStreamingInterceptor grpc.StreamServerInterceptor - -var serverMetricsOnce sync.Once - // GRPCMetrics returns the interceptors used for the default gRPC metrics from grpc-ecosystem/go-grpc-middleware -func GRPCMetrics(disableLatencyHistogram bool) (grpc.UnaryServerInterceptor, grpc.StreamServerInterceptor) { - serverMetricsOnce.Do(func() { - gRPCMetricsUnaryInterceptor, gRPCMetricsStreamingInterceptor = createServerMetrics(disableLatencyHistogram) - }) - - return gRPCMetricsUnaryInterceptor, gRPCMetricsStreamingInterceptor +func GRPCMetrics(disableLatencyHistogram bool, registerer prometheus.Registerer) (grpc.UnaryServerInterceptor, grpc.StreamServerInterceptor) { + return createServerMetrics(disableLatencyHistogram, registerer) } const healthCheckRoute = "/grpc.health.v1.Health/Check" @@ -277,8 +266,8 @@ func doesNotMatchRoute(route string) func(_ context.Context, c interceptors.Call // DefaultUnaryMiddleware generates the default middleware chain used for the public SpiceDB Unary gRPC methods func DefaultUnaryMiddleware(opts MiddlewareOption) (*MiddlewareChain[grpc.UnaryServerInterceptor], error) { - grpcMetricsUnaryInterceptor, _ := GRPCMetrics(opts.DisableGRPCHistogram) - memoryProtectionUnaryInterceptor := memoryprotection.New(opts.MemoryUsageProvider, "unary-middleware") + grpcMetricsUnaryInterceptor, _ := GRPCMetrics(opts.DisableGRPCHistogram, opts.PrometheusRegisterer) + memoryProtectionUnaryInterceptor := memoryprotection.NewWithMetricsFactory(opts.MemoryUsageProvider, "unary-middleware", internalmetrics.NewPrometheusFactory(opts.PrometheusRegisterer)) chain, err := NewMiddlewareChain([]ReferenceableMiddleware[grpc.UnaryServerInterceptor]{ NewUnaryMiddleware(). WithName(DefaultMiddlewareRequestID). @@ -352,8 +341,8 @@ func DefaultUnaryMiddleware(opts MiddlewareOption) (*MiddlewareChain[grpc.UnaryS // DefaultStreamingMiddleware generates the default middleware chain used for the public SpiceDB Streaming gRPC methods func DefaultStreamingMiddleware(opts MiddlewareOption) (*MiddlewareChain[grpc.StreamServerInterceptor], error) { - _, grpcMetricsStreamingInterceptor := GRPCMetrics(opts.DisableGRPCHistogram) - memoryProtectionStreamInterceptor := memoryprotection.New(opts.MemoryUsageProvider, "stream-middleware") + _, grpcMetricsStreamingInterceptor := GRPCMetrics(opts.DisableGRPCHistogram, opts.PrometheusRegisterer) + memoryProtectionStreamInterceptor := memoryprotection.NewWithMetricsFactory(opts.MemoryUsageProvider, "stream-middleware", internalmetrics.NewPrometheusFactory(opts.PrometheusRegisterer)) chain, err := NewMiddlewareChain([]ReferenceableMiddleware[grpc.StreamServerInterceptor]{ NewStreamMiddleware(). WithName(DefaultMiddlewareRequestID). @@ -437,9 +426,9 @@ func determineEventsToLog(opts MiddlewareOption) grpclog.Option { } // DefaultDispatchMiddleware generates the default middleware chain used for the internal dispatch SpiceDB gRPC API -func DefaultDispatchMiddleware(logger zerolog.Logger, authFunc grpcauth.AuthFunc, ds datastore.Datastore, disableGRPCLatencyHistogram bool, memoryUsageProvider memoryprotection.MemoryUsageProvider, dlOpts ...datalayer.DataLayerOption) ([]grpc.UnaryServerInterceptor, []grpc.StreamServerInterceptor) { - grpcMetricsUnaryInterceptor, grpcMetricsStreamingInterceptor := GRPCMetrics(disableGRPCLatencyHistogram) - dispatchMemoryProtection := memoryprotection.New(memoryUsageProvider, "dispatch-middleware") +func DefaultDispatchMiddleware(logger zerolog.Logger, authFunc grpcauth.AuthFunc, ds datastore.Datastore, disableGRPCLatencyHistogram bool, registerer prometheus.Registerer, memoryUsageProvider memoryprotection.MemoryUsageProvider, dlOpts ...datalayer.DataLayerOption) ([]grpc.UnaryServerInterceptor, []grpc.StreamServerInterceptor) { + grpcMetricsUnaryInterceptor, grpcMetricsStreamingInterceptor := GRPCMetrics(disableGRPCLatencyHistogram, registerer) + dispatchMemoryProtection := memoryprotection.NewWithMetricsFactory(memoryUsageProvider, "dispatch-middleware", internalmetrics.NewPrometheusFactory(registerer)) dl := datalayer.NewDataLayer(ds, dlOpts...) return []grpc.UnaryServerInterceptor{ @@ -484,7 +473,11 @@ func InterceptorLogger(l zerolog.Logger) grpclog.Logger { } // initializes prometheus grpc interceptors with exemplar support enabled -func createServerMetrics(disableHistogram bool) (grpc.UnaryServerInterceptor, grpc.StreamServerInterceptor) { +func createServerMetrics(disableHistogram bool, registerer prometheus.Registerer) (grpc.UnaryServerInterceptor, grpc.StreamServerInterceptor) { + if registerer == nil { + registerer = prometheus.DefaultRegisterer + } + var opts []grpcprom.ServerMetricsOption if !disableHistogram { opts = append(opts, grpcprom.WithServerHandlingTimeHistogram( @@ -497,10 +490,16 @@ func createServerMetrics(disableHistogram bool) (grpc.UnaryServerInterceptor, gr }), )) } - srvMetrics := grpcprom.NewServerMetrics(opts...) - // deliberately ignore if these metrics were already registered, so that - // custom builds of SpiceDB can register these metrics with custom labels - _ = prometheus.Register(srvMetrics) + createdMetrics := grpcprom.NewServerMetrics(opts...) + registeredCollector, err := internalmetrics.RegisterOrReuse(registerer, createdMetrics) + if err != nil { + registeredCollector = createdMetrics + } + + srvMetrics, ok := registeredCollector.(*grpcprom.ServerMetrics) + if !ok { + srvMetrics = createdMetrics + } exemplarFromContext := func(ctx context.Context) prometheus.Labels { if span := trace.SpanContextFromContext(ctx); span.IsSampled() { diff --git a/pkg/cmd/server/defaults_test.go b/pkg/cmd/server/defaults_test.go index d4e9d0ce3d..f5b873793e 100644 --- a/pkg/cmd/server/defaults_test.go +++ b/pkg/cmd/server/defaults_test.go @@ -25,18 +25,18 @@ func TestWithDatastore(t *testing.T) { var someDispatcher dispatch.Dispatcher opts := MiddlewareOption{ - someLogger, - someAuthFunc, - true, - someDispatcher, - true, - true, - false, - "service", - consistency.TreatMismatchingTokensAsError, - memoryprotection.NewNoopMemoryUsageProvider(), - nil, - nil, + Logger: someLogger, + AuthFunc: someAuthFunc, + EnableVersionResponse: true, + DispatcherForMiddleware: someDispatcher, + EnableRequestLog: true, + EnableResponseLog: true, + DisableGRPCHistogram: false, + MiddlewareServiceLabel: "service", + MismatchingZedTokenOption: consistency.TreatMismatchingTokensAsError, + MemoryUsageProvider: memoryprotection.NewNoopMemoryUsageProvider(), + unaryDatastoreMiddleware: nil, + streamDatastoreMiddleware: nil, } someDS, err := memdb.NewMemdbDatastore(0, time.Hour, time.Hour) @@ -68,18 +68,18 @@ func TestWithDatastoreMiddleware(t *testing.T) { var someDispatcher dispatch.Dispatcher opts := MiddlewareOption{ - someLogger, - someAuthFunc, - true, - someDispatcher, - true, - true, - false, - "service", - consistency.TreatMismatchingTokensAsError, - memoryprotection.NewNoopMemoryUsageProvider(), - nil, - nil, + Logger: someLogger, + AuthFunc: someAuthFunc, + EnableVersionResponse: true, + DispatcherForMiddleware: someDispatcher, + EnableRequestLog: true, + EnableResponseLog: true, + DisableGRPCHistogram: false, + MiddlewareServiceLabel: "service", + MismatchingZedTokenOption: consistency.TreatMismatchingTokensAsError, + MemoryUsageProvider: memoryprotection.NewNoopMemoryUsageProvider(), + unaryDatastoreMiddleware: nil, + streamDatastoreMiddleware: nil, } someMiddleware := pertoken.NewMiddleware(nil, caveattypes.Default.TypeSet) diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index ea102870da..a9dd4d6a7d 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -35,8 +35,12 @@ import ( "github.com/authzed/spicedb/internal/dispatch/graph" "github.com/authzed/spicedb/internal/dispatch/keys" "github.com/authzed/spicedb/internal/gateway" + graphmetrics "github.com/authzed/spicedb/internal/graph" log "github.com/authzed/spicedb/internal/logging" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/internal/middleware/memoryprotection" + "github.com/authzed/spicedb/internal/middleware/perfinsights" + "github.com/authzed/spicedb/internal/middleware/usagemetrics" "github.com/authzed/spicedb/internal/services" dispatchSvc "github.com/authzed/spicedb/internal/services/dispatch" "github.com/authzed/spicedb/internal/services/health" @@ -168,7 +172,8 @@ type Config struct { EnableResponseLogs bool `debugmap:"visible"` // Metrics - DisableGRPCLatencyHistogram bool `debugmap:"visible"` + DisableGRPCLatencyHistogram bool `debugmap:"visible"` + PrometheusRegisterer prometheus.Registerer `debugmap:"hidden"` } // Complete validates the config and fills out defaults. @@ -189,6 +194,12 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) { return nil, err } + serviceMetricsFactory := metrics.NewPrometheusFactory(c.PrometheusRegisterer) + proxy.SetMetricsFactory(serviceMetricsFactory) + schemacaching.SetMetricsFactory(serviceMetricsFactory) + graphmetrics.SetMetricsFactory(serviceMetricsFactory) + telemetry.SetMetricsFactory(serviceMetricsFactory) + nscc, err := CompleteCache[cache.StringKey, schemacaching.CacheEntry](&c.NamespaceCacheConfig) if err != nil { return nil, fmt.Errorf("failed to create namespace cache: %w", err) @@ -199,6 +210,7 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) { if ds == nil { var err error c.supportOldAndNewReadReplicaConnectionPoolFlags() + c.DatastoreConfig.PrometheusRegisterer = c.PrometheusRegisterer ds, err = datastorecfg.NewDatastore(ctx, c.DatastoreConfig.ToOption(), // Datastore's filter maximum ID count is set to the max size, since the number of elements to be dispatched // are at most the number of elements returned from a datastore query @@ -294,7 +306,7 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) { requestid.StreamClientInterceptor(), ), ), - combineddispatch.MetricsEnabled(c.DispatchClientMetricsEnabled), + combineddispatch.MetricsFactory(dispatchClientMetricsFactory(c)), combineddispatch.PrometheusSubsystem(c.DispatchClientMetricsPrefix), combineddispatch.Cache(cc), combineddispatch.ConcurrencyLimits(concurrencyLimits), @@ -326,7 +338,7 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) { cachingClusterDispatch, err = clusterdispatch.NewClusterDispatcher( dispatcher, - clusterdispatch.MetricsEnabled(c.DispatchClusterMetricsEnabled), + clusterdispatch.MetricsFactory(dispatchClusterMetricsFactory(c)), clusterdispatch.PrometheusSubsystem(c.DispatchClusterMetricsPrefix), clusterdispatch.Cache(cdcc), clusterdispatch.RemoteDispatchTimeout(c.DispatchUpstreamTimeout), @@ -385,6 +397,7 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) { opts := MiddlewareOption{ Logger: log.Logger, AuthFunc: c.GRPCAuthFunc, + PrometheusRegisterer: c.PrometheusRegisterer, EnableVersionResponse: !c.DisableVersionResponse, DispatcherForMiddleware: dispatcher, EnableRequestLog: c.EnableRequestLogs, @@ -468,6 +481,11 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) { QueryPlanMetadata: queryPlanMetadata, } + usagemetrics.SetMetricsFactory(serviceMetricsFactory) + perfinsights.SetMetricsFactory(serviceMetricsFactory) + v1svc.SetMetricsFactory(serviceMetricsFactory) + consistency.SetMetricsFactory(serviceMetricsFactory) + healthManager := health.NewHealthManager(dispatcher, ds) grpcServer, err := c.GRPCServer.Complete(zerolog.InfoLevel, func(server *grpc.Server) { @@ -501,7 +519,12 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) { if err != nil { log.Warn().Err(err).Msg("unable to initialize info collector") } else { - if err := prometheus.Register(infoCollector); err != nil { + registerer := c.PrometheusRegisterer + if registerer == nil { + registerer = prometheus.DefaultRegisterer + } + + if _, err := metrics.RegisterOrReuse(registerer, infoCollector); err != nil { log.Warn().Err(err).Msg("unable to initialize info collector") } } @@ -553,6 +576,26 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) { }, nil } +// dispatchClientMetricsFactory returns a metrics.Factory for the combined (client-side) +// caching dispatcher, respecting the DispatchClientMetricsEnabled flag and the +// PrometheusRegisterer injection point. +func dispatchClientMetricsFactory(c *Config) metrics.Factory { + if !c.DispatchClientMetricsEnabled { + return metrics.NoopFactory{} + } + return metrics.NewPrometheusFactory(c.PrometheusRegisterer) +} + +// dispatchClusterMetricsFactory returns a metrics.Factory for the cluster (server-side) +// caching dispatcher, respecting the DispatchClusterMetricsEnabled flag and the +// PrometheusRegisterer injection point. +func dispatchClusterMetricsFactory(c *Config) metrics.Factory { + if !c.DispatchClusterMetricsEnabled { + return metrics.NoopFactory{} + } + return metrics.NewPrometheusFactory(c.PrometheusRegisterer) +} + func (c *Config) handleGrpcAuthn(ctx context.Context) error { if len(c.PresharedSecureKey) < 1 && c.GRPCAuthFunc == nil { return errors.New("a preshared key must be provided to authenticate API requests") @@ -610,9 +653,9 @@ func (c *Config) BuildMemoryUsageProvider() memoryprotection.MemoryUsageProvider func (c *Config) buildDispatchServer(memoryUsageProvider memoryprotection.MemoryUsageProvider, ds datastore.Datastore, cachingClusterDispatch dispatch.Dispatcher, otelOpts []otelgrpc.Option, dlOpts []datalayer.DataLayerOption) (util.RunnableGRPCServer, error) { if len(c.DispatchUnaryMiddleware) == 0 && len(c.DispatchStreamingMiddleware) == 0 { if c.GRPCAuthFunc == nil { - c.DispatchUnaryMiddleware, c.DispatchStreamingMiddleware = DefaultDispatchMiddleware(log.Logger, auth.MustRequirePresharedKey(c.PresharedSecureKey), ds, c.DisableGRPCLatencyHistogram, memoryUsageProvider, dlOpts...) + c.DispatchUnaryMiddleware, c.DispatchStreamingMiddleware = DefaultDispatchMiddleware(log.Logger, auth.MustRequirePresharedKey(c.PresharedSecureKey), ds, c.DisableGRPCLatencyHistogram, c.PrometheusRegisterer, memoryUsageProvider, dlOpts...) } else { - c.DispatchUnaryMiddleware, c.DispatchStreamingMiddleware = DefaultDispatchMiddleware(log.Logger, c.GRPCAuthFunc, ds, c.DisableGRPCLatencyHistogram, memoryUsageProvider, dlOpts...) + c.DispatchUnaryMiddleware, c.DispatchStreamingMiddleware = DefaultDispatchMiddleware(log.Logger, c.GRPCAuthFunc, ds, c.DisableGRPCLatencyHistogram, c.PrometheusRegisterer, memoryUsageProvider, dlOpts...) } } @@ -723,7 +766,7 @@ func (c *Config) initializeGateway(ctx context.Context) (util.RunnableHTTPServer } var gatewayHandler http.Handler - closeableGatewayHandler, err := gateway.NewHandler(ctx, c.HTTPGatewayUpstreamAddr, c.HTTPGatewayUpstreamTLSCertPath) + closeableGatewayHandler, err := gateway.NewHandlerWithMetricsFactory(ctx, c.HTTPGatewayUpstreamAddr, c.HTTPGatewayUpstreamTLSCertPath, metrics.NewPrometheusFactory(c.PrometheusRegisterer)) if err != nil { return nil, nil, fmt.Errorf("failed to initialize rest gateway: %w", err) } diff --git a/pkg/cmd/server/server_test.go b/pkg/cmd/server/server_test.go index f604fb277b..b6cda6fa16 100644 --- a/pkg/cmd/server/server_test.go +++ b/pkg/cmd/server/server_test.go @@ -528,7 +528,20 @@ func TestModifyUnaryMiddleware(t *testing.T) { }, }} - opt := MiddlewareOption{logging.Logger, nil, false, nil, false, false, false, "testing", consistency.TreatMismatchingTokensAsFullConsistency, memoryprotection.NewNoopMemoryUsageProvider(), nil, nil} + opt := MiddlewareOption{ + Logger: logging.Logger, + AuthFunc: nil, + EnableVersionResponse: false, + DispatcherForMiddleware: nil, + EnableRequestLog: false, + EnableResponseLog: false, + DisableGRPCHistogram: false, + MiddlewareServiceLabel: "testing", + MismatchingZedTokenOption: consistency.TreatMismatchingTokensAsFullConsistency, + MemoryUsageProvider: memoryprotection.NewNoopMemoryUsageProvider(), + unaryDatastoreMiddleware: nil, + streamDatastoreMiddleware: nil, + } opt = opt.WithDatastore(nil) defaultMw, err := DefaultUnaryMiddleware(opt) @@ -556,7 +569,20 @@ func TestModifyStreamingMiddleware(t *testing.T) { }, }} - opt := MiddlewareOption{logging.Logger, nil, false, nil, false, false, false, "testing", consistency.TreatMismatchingTokensAsFullConsistency, memoryprotection.NewNoopMemoryUsageProvider(), nil, nil} + opt := MiddlewareOption{ + Logger: logging.Logger, + AuthFunc: nil, + EnableVersionResponse: false, + DispatcherForMiddleware: nil, + EnableRequestLog: false, + EnableResponseLog: false, + DisableGRPCHistogram: false, + MiddlewareServiceLabel: "testing", + MismatchingZedTokenOption: consistency.TreatMismatchingTokensAsFullConsistency, + MemoryUsageProvider: memoryprotection.NewNoopMemoryUsageProvider(), + unaryDatastoreMiddleware: nil, + streamDatastoreMiddleware: nil, + } opt = opt.WithDatastore(nil) defaultMw, err := DefaultStreamingMiddleware(opt) diff --git a/pkg/cmd/server/zz_generated.middlewareoption.go b/pkg/cmd/server/zz_generated.middlewareoption.go index c4ac6fd639..3c22be50a1 100644 --- a/pkg/cmd/server/zz_generated.middlewareoption.go +++ b/pkg/cmd/server/zz_generated.middlewareoption.go @@ -7,6 +7,7 @@ import ( consistency "github.com/authzed/spicedb/pkg/middleware/consistency" defaults "github.com/creasty/defaults" auth "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/auth" + prometheus "github.com/prometheus/client_golang/prometheus" zerolog "github.com/rs/zerolog" ) @@ -36,6 +37,7 @@ func (m *MiddlewareOption) ToOption() MiddlewareOptionOption { return func(to *MiddlewareOption) { to.Logger = m.Logger to.AuthFunc = m.AuthFunc + to.PrometheusRegisterer = m.PrometheusRegisterer to.EnableVersionResponse = m.EnableVersionResponse to.DispatcherForMiddleware = m.DispatcherForMiddleware to.EnableRequestLog = m.EnableRequestLog @@ -106,71 +108,78 @@ func (m *MiddlewareOption) WithOptions(opts ...MiddlewareOptionOption) *Middlewa return m } -// WithLogger returns an option that can set Logger on a MiddlewareOption -func WithLogger(logger zerolog.Logger) MiddlewareOptionOption { +// WithMiddlewareOptionLogger returns an option that can set Logger on a MiddlewareOption +func WithMiddlewareOptionLogger(logger zerolog.Logger) MiddlewareOptionOption { return func(m *MiddlewareOption) { m.Logger = logger } } -// WithAuthFunc returns an option that can set AuthFunc on a MiddlewareOption -func WithAuthFunc(authFunc auth.AuthFunc) MiddlewareOptionOption { +// WithMiddlewareOptionAuthFunc returns an option that can set AuthFunc on a MiddlewareOption +func WithMiddlewareOptionAuthFunc(authFunc auth.AuthFunc) MiddlewareOptionOption { return func(m *MiddlewareOption) { m.AuthFunc = authFunc } } -// WithEnableVersionResponse returns an option that can set EnableVersionResponse on a MiddlewareOption -func WithEnableVersionResponse(enableVersionResponse bool) MiddlewareOptionOption { +// WithMiddlewareOptionPrometheusRegisterer returns an option that can set PrometheusRegisterer on a MiddlewareOption +func WithMiddlewareOptionPrometheusRegisterer(prometheusRegisterer prometheus.Registerer) MiddlewareOptionOption { + return func(m *MiddlewareOption) { + m.PrometheusRegisterer = prometheusRegisterer + } +} + +// WithMiddlewareOptionEnableVersionResponse returns an option that can set EnableVersionResponse on a MiddlewareOption +func WithMiddlewareOptionEnableVersionResponse(enableVersionResponse bool) MiddlewareOptionOption { return func(m *MiddlewareOption) { m.EnableVersionResponse = enableVersionResponse } } -// WithDispatcherForMiddleware returns an option that can set DispatcherForMiddleware on a MiddlewareOption -func WithDispatcherForMiddleware(dispatcherForMiddleware dispatch.Dispatcher) MiddlewareOptionOption { +// WithMiddlewareOptionDispatcherForMiddleware returns an option that can set DispatcherForMiddleware on a MiddlewareOption +func WithMiddlewareOptionDispatcherForMiddleware(dispatcherForMiddleware dispatch.Dispatcher) MiddlewareOptionOption { return func(m *MiddlewareOption) { m.DispatcherForMiddleware = dispatcherForMiddleware } } -// WithEnableRequestLog returns an option that can set EnableRequestLog on a MiddlewareOption -func WithEnableRequestLog(enableRequestLog bool) MiddlewareOptionOption { +// WithMiddlewareOptionEnableRequestLog returns an option that can set EnableRequestLog on a MiddlewareOption +func WithMiddlewareOptionEnableRequestLog(enableRequestLog bool) MiddlewareOptionOption { return func(m *MiddlewareOption) { m.EnableRequestLog = enableRequestLog } } -// WithEnableResponseLog returns an option that can set EnableResponseLog on a MiddlewareOption -func WithEnableResponseLog(enableResponseLog bool) MiddlewareOptionOption { +// WithMiddlewareOptionEnableResponseLog returns an option that can set EnableResponseLog on a MiddlewareOption +func WithMiddlewareOptionEnableResponseLog(enableResponseLog bool) MiddlewareOptionOption { return func(m *MiddlewareOption) { m.EnableResponseLog = enableResponseLog } } -// WithDisableGRPCHistogram returns an option that can set DisableGRPCHistogram on a MiddlewareOption -func WithDisableGRPCHistogram(disableGRPCHistogram bool) MiddlewareOptionOption { +// WithMiddlewareOptionDisableGRPCHistogram returns an option that can set DisableGRPCHistogram on a MiddlewareOption +func WithMiddlewareOptionDisableGRPCHistogram(disableGRPCHistogram bool) MiddlewareOptionOption { return func(m *MiddlewareOption) { m.DisableGRPCHistogram = disableGRPCHistogram } } -// WithMiddlewareServiceLabel returns an option that can set MiddlewareServiceLabel on a MiddlewareOption -func WithMiddlewareServiceLabel(middlewareServiceLabel string) MiddlewareOptionOption { +// WithMiddlewareOptionMiddlewareServiceLabel returns an option that can set MiddlewareServiceLabel on a MiddlewareOption +func WithMiddlewareOptionMiddlewareServiceLabel(middlewareServiceLabel string) MiddlewareOptionOption { return func(m *MiddlewareOption) { m.MiddlewareServiceLabel = middlewareServiceLabel } } -// WithMismatchingZedTokenOption returns an option that can set MismatchingZedTokenOption on a MiddlewareOption -func WithMismatchingZedTokenOption(mismatchingZedTokenOption consistency.MismatchingTokenOption) MiddlewareOptionOption { +// WithMiddlewareOptionMismatchingZedTokenOption returns an option that can set MismatchingZedTokenOption on a MiddlewareOption +func WithMiddlewareOptionMismatchingZedTokenOption(mismatchingZedTokenOption consistency.MismatchingTokenOption) MiddlewareOptionOption { return func(m *MiddlewareOption) { m.MismatchingZedTokenOption = mismatchingZedTokenOption } } -// WithMemoryUsageProvider returns an option that can set MemoryUsageProvider on a MiddlewareOption -func WithMemoryUsageProvider(memoryUsageProvider memoryprotection.MemoryUsageProvider) MiddlewareOptionOption { +// WithMiddlewareOptionMemoryUsageProvider returns an option that can set MemoryUsageProvider on a MiddlewareOption +func WithMiddlewareOptionMemoryUsageProvider(memoryUsageProvider memoryprotection.MemoryUsageProvider) MiddlewareOptionOption { return func(m *MiddlewareOption) { m.MemoryUsageProvider = memoryUsageProvider } diff --git a/pkg/cmd/server/zz_generated.options.go b/pkg/cmd/server/zz_generated.options.go index 4b0cab41a6..aaf47e87ab 100644 --- a/pkg/cmd/server/zz_generated.options.go +++ b/pkg/cmd/server/zz_generated.options.go @@ -11,6 +11,7 @@ import ( query "github.com/authzed/spicedb/pkg/query" defaults "github.com/creasty/defaults" auth "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/auth" + prometheus "github.com/prometheus/client_golang/prometheus" grpc "google.golang.org/grpc" "time" ) @@ -114,6 +115,7 @@ func (c *Config) ToOption() ConfigOption { to.EnableRequestLogs = c.EnableRequestLogs to.EnableResponseLogs = c.EnableResponseLogs to.DisableGRPCLatencyHistogram = c.DisableGRPCLatencyHistogram + to.PrometheusRegisterer = c.PrometheusRegisterer } } @@ -1019,3 +1021,10 @@ func WithDisableGRPCLatencyHistogram(disableGRPCLatencyHistogram bool) ConfigOpt c.DisableGRPCLatencyHistogram = disableGRPCLatencyHistogram } } + +// WithPrometheusRegisterer returns an option that can set PrometheusRegisterer on a Config +func WithPrometheusRegisterer(prometheusRegisterer prometheus.Registerer) ConfigOption { + return func(c *Config) { + c.PrometheusRegisterer = prometheusRegisterer + } +} diff --git a/pkg/cmd/testserver/testserver.go b/pkg/cmd/testserver/testserver.go index 611beb156b..7013d8dc5c 100644 --- a/pkg/cmd/testserver/testserver.go +++ b/pkg/cmd/testserver/testserver.go @@ -115,13 +115,13 @@ func (c *Config) Complete(ctx context.Context) (RunnableTestServer, error) { ) } - noAuth := server.WithAuthFunc(func(ctx context.Context) (context.Context, error) { + noAuth := server.WithMiddlewareOptionAuthFunc(func(ctx context.Context) (context.Context, error) { // Turn off the default auth system. return ctx, nil }) opts := *server.NewMiddlewareOptionWithOptions(noAuth, - server.WithLogger(log.Logger), - server.WithMemoryUsageProvider(&memoryprotection.HarcodedMemoryUsageProvider{AcceptAllRequests: true})) + server.WithMiddlewareOptionLogger(log.Logger), + server.WithMiddlewareOptionMemoryUsageProvider(&memoryprotection.HarcodedMemoryUsageProvider{AcceptAllRequests: true})) opts = opts.WithDatastoreMiddleware(datastoreMiddleware) unaryMiddleware, err := server.DefaultUnaryMiddleware(opts) diff --git a/pkg/datastore/gc.go b/pkg/datastore/gc.go index f9a9433504..7b0f7a2cbb 100644 --- a/pkg/datastore/gc.go +++ b/pkg/datastore/gc.go @@ -3,6 +3,7 @@ package datastore import ( "context" "fmt" + "sync" "time" "github.com/cenkalti/backoff/v5" @@ -11,70 +12,173 @@ import ( "go.opentelemetry.io/otel" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" ) var gcTracer = otel.Tracer("spicedb/pkg/datastore") +var gcMetricsMu sync.Mutex + var ( - gcDurationHistogram = prometheus.NewHistogram(prometheus.HistogramOpts{ + gcDurationHistogram internalmetrics.Histogram // GUARDED_BY(gcMetricsMu) + gcRelationshipsCounter internalmetrics.Counter // GUARDED_BY(gcMetricsMu) + gcExpiredRelationshipsCounter internalmetrics.Counter // GUARDED_BY(gcMetricsMu) + gcTransactionsCounter internalmetrics.Counter // GUARDED_BY(gcMetricsMu) + gcNamespacesCounter internalmetrics.Counter // GUARDED_BY(gcMetricsMu) + gcFailureCounter internalmetrics.Counter // GUARDED_BY(gcMetricsMu) + + gcDurationHistogramOpts = internalmetrics.Opts{ Namespace: "spicedb", Subsystem: "datastore", Name: "gc_duration_seconds", Help: "The duration of datastore garbage collection.", Buckets: []float64{0.01, 0.1, 0.5, 1, 5, 10, 25, 60, 120}, - }) + } - gcRelationshipsCounter = prometheus.NewCounter(prometheus.CounterOpts{ + gcRelationshipsCounterOpts = internalmetrics.Opts{ Namespace: "spicedb", Subsystem: "datastore", Name: "gc_relationships_total", Help: "The number of stale relationships deleted by the datastore garbage collection.", - }) + } - gcExpiredRelationshipsCounter = prometheus.NewCounter(prometheus.CounterOpts{ + gcExpiredRelationshipsCounterOpts = internalmetrics.Opts{ Namespace: "spicedb", Subsystem: "datastore", Name: "gc_expired_relationships_total", Help: "The number of expired relationships deleted by the datastore garbage collection.", - }) + } - gcTransactionsCounter = prometheus.NewCounter(prometheus.CounterOpts{ + gcTransactionsCounterOpts = internalmetrics.Opts{ Namespace: "spicedb", Subsystem: "datastore", Name: "gc_transactions_total", Help: "The number of stale transactions deleted by the datastore garbage collection.", - }) + } - gcNamespacesCounter = prometheus.NewCounter(prometheus.CounterOpts{ + gcNamespacesCounterOpts = internalmetrics.Opts{ Namespace: "spicedb", Subsystem: "datastore", Name: "gc_namespaces_total", Help: "The number of stale namespaces deleted by the datastore garbage collection.", - }) + } - gcFailureCounterConfig = prometheus.CounterOpts{ + gcFailureCounterOpts = internalmetrics.Opts{ Namespace: "spicedb", Subsystem: "datastore", Name: "gc_failure_total", Help: "The number of failed runs of the datastore garbage collection.", } - gcFailureCounter = prometheus.NewCounter(gcFailureCounterConfig) ) -// RegisterGCMetrics registers garbage collection metrics to the default -// registry and returns them (so that they be unregistered). -func RegisterGCMetrics() ([]prometheus.Collector, error) { - collectors := []prometheus.Collector{ - gcDurationHistogram, - gcRelationshipsCounter, - gcTransactionsCounter, - gcNamespacesCounter, - gcFailureCounter, +func init() { + SetMetricsFactory(internalmetrics.NewPrometheusFactory(nil)) +} + +// SetMetricsFactory configures which metrics factory is used by this package's +// runtime garbage collection metrics. +func SetMetricsFactory(factory internalmetrics.Factory) { + if factory == nil { + factory = internalmetrics.NoopFactory{} + } + + gcMetricsMu.Lock() + defer gcMetricsMu.Unlock() + + gcDurationHistogram = factory.Histogram(gcDurationHistogramOpts) + gcRelationshipsCounter = factory.Counter(gcRelationshipsCounterOpts) + gcExpiredRelationshipsCounter = factory.Counter(gcExpiredRelationshipsCounterOpts) + gcTransactionsCounter = factory.Counter(gcTransactionsCounterOpts) + gcNamespacesCounter = factory.Counter(gcNamespacesCounterOpts) + gcFailureCounter = factory.Counter(gcFailureCounterOpts) +} + +func gcPrometheusCollectors() []prometheus.Collector { + return []prometheus.Collector{ + prometheus.NewHistogram(prometheus.HistogramOpts{ + Namespace: gcDurationHistogramOpts.Namespace, + Subsystem: gcDurationHistogramOpts.Subsystem, + Name: gcDurationHistogramOpts.Name, + Help: gcDurationHistogramOpts.Help, + Buckets: gcDurationHistogramOpts.Buckets, + }), + prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: gcRelationshipsCounterOpts.Namespace, + Subsystem: gcRelationshipsCounterOpts.Subsystem, + Name: gcRelationshipsCounterOpts.Name, + Help: gcRelationshipsCounterOpts.Help, + }), + prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: gcExpiredRelationshipsCounterOpts.Namespace, + Subsystem: gcExpiredRelationshipsCounterOpts.Subsystem, + Name: gcExpiredRelationshipsCounterOpts.Name, + Help: gcExpiredRelationshipsCounterOpts.Help, + }), + prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: gcTransactionsCounterOpts.Namespace, + Subsystem: gcTransactionsCounterOpts.Subsystem, + Name: gcTransactionsCounterOpts.Name, + Help: gcTransactionsCounterOpts.Help, + }), + prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: gcNamespacesCounterOpts.Namespace, + Subsystem: gcNamespacesCounterOpts.Subsystem, + Name: gcNamespacesCounterOpts.Name, + Help: gcNamespacesCounterOpts.Help, + }), + prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: gcFailureCounterOpts.Namespace, + Subsystem: gcFailureCounterOpts.Subsystem, + Name: gcFailureCounterOpts.Name, + Help: gcFailureCounterOpts.Help, + }), } - for _, metric := range collectors { - if err := prometheus.Register(metric); err != nil { - return nil, fmt.Errorf("failed to register GC metric: %w", err) +} + +// RegisterGCMetrics registers garbage collection metrics to the supplied +// registerer and returns the registered collectors so they can be +// unregistered when the datastore is closed. +// +// If registerer is nil, prometheus.DefaultRegisterer is used. +// +// AlreadyRegisteredError is tolerated: when another caller has already +// registered one of these metrics the existing collector is reused. This +// keeps the function idempotent when, for example, a primary and a replica +// datastore are created in the same process. +func RegisterGCMetrics(registerer prometheus.Registerer) ([]prometheus.Collector, error) { + if registerer == nil { + registerer = prometheus.DefaultRegisterer + } + candidates := gcPrometheusCollectors() + collectors := make([]prometheus.Collector, 0, len(candidates)) + for _, metric := range candidates { + registered, err := internalmetrics.RegisterOrReuse(registerer, metric) + if err != nil { + return collectors, fmt.Errorf("failed to register GC metric: %w", err) } + collectors = append(collectors, registered) + } + + gcMetricsMu.Lock() + defer gcMetricsMu.Unlock() + + if h, ok := collectors[0].(prometheus.Histogram); ok { + gcDurationHistogram = h + } + if c, ok := collectors[1].(prometheus.Counter); ok { + gcRelationshipsCounter = c + } + if c, ok := collectors[2].(prometheus.Counter); ok { + gcExpiredRelationshipsCounter = c + } + if c, ok := collectors[3].(prometheus.Counter); ok { + gcTransactionsCounter = c + } + if c, ok := collectors[4].(prometheus.Counter); ok { + gcNamespacesCounter = c + } + if c, ok := collectors[5].(prometheus.Counter); ok { + gcFailureCounter = c } return collectors, nil @@ -162,7 +266,7 @@ func StartGarbageCollector(ctx context.Context, collectable GarbageCollectableDa }, interval, timeout, gcFailureCounter) } -func runGCOnIntervalWithBackoff(ctx context.Context, taskFn func() error, interval, timeout time.Duration, failureCounter prometheus.Counter) error { +func runGCOnIntervalWithBackoff(ctx context.Context, taskFn func() error, interval, timeout time.Duration, failureCounter internalmetrics.Counter) error { backoffInterval := backoff.NewExponentialBackOff() backoffInterval.InitialInterval = interval backoffInterval.MaxInterval = max(MaxGCInterval, interval) diff --git a/pkg/datastore/gc_test.go b/pkg/datastore/gc_test.go index 3f23885a70..f9fc45948d 100644 --- a/pkg/datastore/gc_test.go +++ b/pkg/datastore/gc_test.go @@ -6,6 +6,7 @@ import ( "fmt" "slices" "sync" + "sync/atomic" "testing" "testing/synctest" "time" @@ -15,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/goleak" + internalmetrics "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/pkg/testutil" ) @@ -167,23 +169,26 @@ func alwaysErr() error { func TestGCFailureBackoff(t *testing.T) { t.Cleanup(func() { - goleak.VerifyNone(t, testutil.GoLeakIgnores()...) + goleak.VerifyNone(t, append(testutil.GoLeakIgnores(), goleak.IgnoreCurrent())...) + }) + localCounter := prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: gcFailureCounterOpts.Namespace, + Subsystem: gcFailureCounterOpts.Subsystem, + Name: gcFailureCounterOpts.Name, + Help: gcFailureCounterOpts.Help, }) - localCounter := prometheus.NewCounter(gcFailureCounterConfig) reg := prometheus.NewRegistry() require.NoError(t, reg.Register(localCounter)) errCh := make(chan error, 1) synctest.Test(t, func(t *testing.T) { duration := 1000 * time.Second - ctx, cancel := context.WithTimeout(t.Context(), duration) - t.Cleanup(func() { - cancel() - }) + ctx, cancel := context.WithCancel(t.Context()) go func() { errCh <- runGCOnIntervalWithBackoff(ctx, alwaysErr, 100*time.Second, 1*time.Minute, localCounter) }() time.Sleep(duration) + cancel() synctest.Wait() }) require.Error(t, <-errCh) @@ -241,6 +246,142 @@ func TestGCFailureBackoffReset(t *testing.T) { require.Greater(t, gc.markedCompleteCount, 20, "Next interval was not reset with backoff") } +func TestRunGCOnIntervalWithBackoffResetsAfterSuccess(t *testing.T) { + factory := internalmetrics.NewRecordingFactory() + failureCounter := factory.Counter(internalmetrics.Opts{ + Namespace: "spicedb", + Subsystem: "datastore", + Name: "gc_failure_reset_test_total", + Help: "test", + }) + + var callCount atomic.Int32 + errCh := make(chan error, 1) + + synctest.Test(t, func(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + go func() { + errCh <- runGCOnIntervalWithBackoff(ctx, func() error { + if callCount.Add(1) == 1 { + return errors.New("first call fails") + } + return nil + }, 10*time.Millisecond, 1*time.Minute, failureCounter) + }() + + // Allow enough virtual time for one failure and several successful runs. + time.Sleep(100 * time.Millisecond) + cancel() + synctest.Wait() + }) + + require.Error(t, <-errCh) + require.GreaterOrEqual(t, callCount.Load(), int32(3), "expected repeated runs after a successful retry") + require.InEpsilon(t, 1.0, factory.CounterValue("spicedb", "datastore", "gc_failure_reset_test_total"), 1e-9) +} + +func TestRegisterGCMetricsRepeatedRegistration(t *testing.T) { + registry := prometheus.NewRegistry() + + first, err := RegisterGCMetrics(registry) + require.NoError(t, err) + require.NotEmpty(t, first) + + second, err := RegisterGCMetrics(registry) + require.NoError(t, err) + require.Len(t, second, len(first)) + + for index := range first { + require.Same(t, first[index], second[index]) + } +} + +func TestRegisterGCMetricsWithNilRegisterer(t *testing.T) { + // Passing nil should use DefaultRegisterer + collectors, err := RegisterGCMetrics(nil) + // Should succeed without error + require.NoError(t, err) + require.NotEmpty(t, collectors) + require.Len(t, collectors, 6) +} + +func TestRegisterGCMetricsReturnsCollectors(t *testing.T) { + registry := prometheus.NewRegistry() + collectors, err := RegisterGCMetrics(registry) + + require.NoError(t, err) + require.Len(t, collectors, 6) + + // Verify we got the expected collector types + _, isHistogram := collectors[0].(prometheus.Histogram) + require.True(t, isHistogram, "First collector should be a Histogram") + + for i := 1; i < len(collectors); i++ { + _, isCounter := collectors[i].(prometheus.Counter) + require.True(t, isCounter, "Collector %d should be a Counter", i) + } +} + +func TestRegisterGCMetricsAssignsGlobalMetricsFromRegisteredCollectors(t *testing.T) { + gcMetricsMu.Lock() + originalHistogram := gcDurationHistogram + originalRelationships := gcRelationshipsCounter + originalExpired := gcExpiredRelationshipsCounter + originalTransactions := gcTransactionsCounter + originalNamespaces := gcNamespacesCounter + originalFailure := gcFailureCounter + gcMetricsMu.Unlock() + + t.Cleanup(func() { + gcMetricsMu.Lock() + defer gcMetricsMu.Unlock() + gcDurationHistogram = originalHistogram + gcRelationshipsCounter = originalRelationships + gcExpiredRelationshipsCounter = originalExpired + gcTransactionsCounter = originalTransactions + gcNamespacesCounter = originalNamespaces + gcFailureCounter = originalFailure + }) + + registry := prometheus.NewRegistry() + collectors, err := RegisterGCMetrics(registry) + require.NoError(t, err) + require.Len(t, collectors, 6) + + gcMetricsMu.Lock() + registeredHistogram, ok := gcDurationHistogram.(prometheus.Histogram) + require.True(t, ok) + registeredRelationships, ok := gcRelationshipsCounter.(prometheus.Counter) + require.True(t, ok) + registeredExpired, ok := gcExpiredRelationshipsCounter.(prometheus.Counter) + require.True(t, ok) + registeredTransactions, ok := gcTransactionsCounter.(prometheus.Counter) + require.True(t, ok) + registeredNamespaces, ok := gcNamespacesCounter.(prometheus.Counter) + require.True(t, ok) + registeredFailure, ok := gcFailureCounter.(prometheus.Counter) + require.True(t, ok) + gcMetricsMu.Unlock() + + require.Equal(t, collectors[0], registeredHistogram) + require.Equal(t, collectors[1], registeredRelationships) + require.Equal(t, collectors[2], registeredExpired) + require.Equal(t, collectors[3], registeredTransactions) + require.Equal(t, collectors[4], registeredNamespaces) + require.Equal(t, collectors[5], registeredFailure) + + registeredHistogram.Observe(0.25) + registeredRelationships.Add(1) + registeredExpired.Add(1) + registeredTransactions.Add(1) + registeredNamespaces.Add(1) + registeredFailure.Add(1) + + metricFamilies, err := registry.Gather() + require.NoError(t, err) + require.NotEmpty(t, metricFamilies) +} + func TestGCUnlockOnTimeout(t *testing.T) { gc := newFakeGCStore(alwaysErrorDeleter{}) @@ -269,3 +410,62 @@ func TestGCUnlockOnTimeout(t *testing.T) { require.True(t, gc.fakeGC.wasLocked, "GC should have been locked") require.True(t, gc.fakeGC.wasUnlocked, "GC should have been unlocked") } + +func TestSetMetricsFactoryWithRecordingFactory(t *testing.T) { + original := internalmetrics.NewPrometheusFactory(nil) + t.Cleanup(func() { + SetMetricsFactory(original) + }) + + recordingFactory := internalmetrics.NewRecordingFactory() + SetMetricsFactory(recordingFactory) + + // Verify that the metrics are set to the recording factory's implementations + require.NotNil(t, gcDurationHistogram) + require.NotNil(t, gcRelationshipsCounter) + require.NotNil(t, gcFailureCounter) + + // Should not panic when using recording factory metrics + gcDurationHistogram.Observe(1.0) + gcRelationshipsCounter.Add(1) + gcFailureCounter.Add(1) +} + +func TestSetMetricsFactoryWithNoopFactory(t *testing.T) { + original := internalmetrics.NewPrometheusFactory(nil) + t.Cleanup(func() { + SetMetricsFactory(original) + }) + + SetMetricsFactory(internalmetrics.NoopFactory{}) + + // Verify that the metrics are set (even if they're no-op) + require.NotNil(t, gcDurationHistogram) + require.NotNil(t, gcRelationshipsCounter) + require.NotNil(t, gcFailureCounter) + + // Should not panic when using noop factory metrics + gcDurationHistogram.Observe(1.0) + gcRelationshipsCounter.Add(1) + gcFailureCounter.Add(1) +} + +func TestSetMetricsFactoryWithNil(t *testing.T) { + original := internalmetrics.NewPrometheusFactory(nil) + t.Cleanup(func() { + SetMetricsFactory(original) + }) + + // Passing nil should default to NoopFactory + SetMetricsFactory(nil) + + // Verify that the metrics are set (to noop implementations) + require.NotNil(t, gcDurationHistogram) + require.NotNil(t, gcRelationshipsCounter) + require.NotNil(t, gcFailureCounter) + + // Should not panic when using noop factory metrics + gcDurationHistogram.Observe(1.0) + gcRelationshipsCounter.Add(1) + gcFailureCounter.Add(1) +} diff --git a/pkg/middleware/consistency/consistency.go b/pkg/middleware/consistency/consistency.go index cc1d42098f..e306f201bc 100644 --- a/pkg/middleware/consistency/consistency.go +++ b/pkg/middleware/consistency/consistency.go @@ -5,8 +5,6 @@ import ( "errors" "strings" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" "github.com/rs/zerolog/log" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -14,6 +12,7 @@ import ( v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" + "github.com/authzed/spicedb/internal/metrics" "github.com/authzed/spicedb/internal/services/shared" "github.com/authzed/spicedb/pkg/cursor" "github.com/authzed/spicedb/pkg/datalayer" @@ -22,12 +21,27 @@ import ( "github.com/authzed/spicedb/pkg/zedtoken" ) -var ConsistencyCounter = promauto.NewCounterVec(prometheus.CounterOpts{ +var ConsistencyCounter metrics.CounterVec + +var consistencyCounterOpts = metrics.Opts{ Namespace: "spicedb", Subsystem: "middleware", Name: "consistency_assigned_total", Help: "Count of the consistencies used per request", -}, []string{"method", "source", "service"}) +} + +func init() { + SetMetricsFactory(metrics.NewPrometheusFactory(nil)) +} + +// SetMetricsFactory configures which metrics factory is used by consistency +// middleware counters. +func SetMetricsFactory(factory metrics.Factory) { + if factory == nil { + factory = metrics.NoopFactory{} + } + ConsistencyCounter = factory.CounterVec(consistencyCounterOpts, []string{"method", "source", "service"}) +} // MismatchingTokenOption is the option specifying the behavior of the consistency middleware // when a ZedToken provided references a different datastore instance than the current diff --git a/pkg/runtime/metrics.go b/pkg/runtime/metrics.go index 85f0f8a98f..48bd86d2cc 100644 --- a/pkg/runtime/metrics.go +++ b/pkg/runtime/metrics.go @@ -3,6 +3,8 @@ package runtime import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/collectors" + + internalmetrics "github.com/authzed/spicedb/internal/metrics" ) // prometheus client_golang by default registers a collector that collects all metrics, except scheduler metrics @@ -11,7 +13,7 @@ import ( // in order to register this, the package must be imported anonymously func init() { prometheus.DefaultRegisterer.Unregister(collectors.NewGoCollector()) - prometheus.MustRegister(collectors.NewGoCollector( + internalmetrics.MustRegisterOrReuse(prometheus.DefaultRegisterer, collectors.NewGoCollector( collectors.WithGoCollectorRuntimeMetrics(collectors.MetricsAll), )) } diff --git a/pkg/x509util/certwatcher.go b/pkg/x509util/certwatcher.go index df3efc3efe..c05ef1fe3f 100644 --- a/pkg/x509util/certwatcher.go +++ b/pkg/x509util/certwatcher.go @@ -29,6 +29,7 @@ import ( "github.com/prometheus/client_golang/prometheus" log "github.com/authzed/spicedb/internal/logging" + internalmetrics "github.com/authzed/spicedb/internal/metrics" ) var ( @@ -69,6 +70,7 @@ type CertWatcher struct { // metrics ReadCertificateTotal prometheus.Counter ReadCertificateErrors prometheus.Counter + registerer prometheus.Registerer } // NewTLSCertWatcher returns a new CertWatcher watching the given certificate and key. @@ -84,11 +86,16 @@ func NewTLSCertWatcher(certPath, keyPath string) (*CertWatcher, error) { started: make(chan error), ReadCertificateTotal: ReadTotal, ReadCertificateErrors: ReadErrors, + registerer: prometheus.DefaultRegisterer, } - // ignore "duplicate metric registration" errors - _ = prometheus.Register(cw.ReadCertificateTotal) - _ = prometheus.Register(cw.ReadCertificateErrors) + if _, err := internalmetrics.RegisterOrReuse(cw.registerer, cw.ReadCertificateTotal); err != nil { + return nil, err + } + if _, err := internalmetrics.RegisterOrReuse(cw.registerer, cw.ReadCertificateErrors); err != nil { + cw.unregisterMetrics() + return nil, err + } // Initial read of certificate and key. if err := cw.ReadCertificate(); err != nil { @@ -107,8 +114,11 @@ func NewTLSCertWatcher(certPath, keyPath string) (*CertWatcher, error) { // unregisterMetrics removes the prometheus metrics registered by this CertWatcher. func (cw *CertWatcher) unregisterMetrics() { - prometheus.Unregister(cw.ReadCertificateTotal) - prometheus.Unregister(cw.ReadCertificateErrors) + if cw.registerer == nil { + cw.registerer = prometheus.DefaultRegisterer + } + cw.registerer.Unregister(cw.ReadCertificateTotal) + cw.registerer.Unregister(cw.ReadCertificateErrors) } // WithWatchInterval sets the watch interval and returns the CertWatcher pointer diff --git a/pkg/x509util/certwatcher_test.go b/pkg/x509util/certwatcher_test.go index 97790aa7f1..4abefbc1a9 100644 --- a/pkg/x509util/certwatcher_test.go +++ b/pkg/x509util/certwatcher_test.go @@ -72,6 +72,31 @@ func TestCertWatcherSequentialMetricRegistration(t *testing.T) { cancel2() } +func TestCertWatcherRepeatedConstructionMetricRegistration(t *testing.T) { + dir := t.TempDir() + certPath := dir + "/tls.crt" + keyPath := dir + "/tls.key" + + require.NoError(t, writeCerts(certPath, keyPath, "127.0.0.1")) + + watcher1, err := NewTLSCertWatcher(certPath, keyPath) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, watcher1.watcher.Close()) + watcher1.unregisterMetrics() + }) + + watcher2, err := NewTLSCertWatcher(certPath, keyPath) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, watcher2.watcher.Close()) + watcher2.unregisterMetrics() + }) + + require.Same(t, watcher1.ReadCertificateTotal, watcher2.ReadCertificateTotal) + require.Same(t, watcher1.ReadCertificateErrors, watcher2.ReadCertificateErrors) +} + // setupWatcher creates a temporary certificate/key pair for the given IP address, // constructs a CertWatcher, and returns a startWatcher function that launches the // watcher in a background goroutine.