Skip to content

Commit a8858d8

Browse files
miparnisarimiparnisari
authored
refactor: now, the server object owns the caches (#3167)
Solves #3156 (comment) Right now, the `caches sync.Map` is a global variable, which means that if you try to spin up 2 spicedb servers within the same process (as our tests do) and both have caching turned on, a panic will occur. This PR fixes that.
1 parent 221681b commit a8858d8

7 files changed

Lines changed: 115 additions & 120 deletions

File tree

‎pkg/cache/cache_otter.go‎

Lines changed: 80 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,34 +3,50 @@
33
package cache
44

55
import (
6+
"fmt"
67
"math"
78
"sync/atomic"
89
"time"
910

1011
"github.com/ccoveille/go-safecast/v2"
1112
"github.com/maypok86/otter/v2"
1213
"github.com/maypok86/otter/v2/stats"
14+
"github.com/prometheus/client_golang/prometheus"
1315
"github.com/rs/zerolog"
1416
)
1517

16-
func NewOtterCacheWithMetrics[K KeyString, V any](name string, config *Config) (Cache[K, V], error) {
17-
cache, err := NewOtterCache[K, V](name, config)
18-
if err != nil {
19-
return nil, err
20-
}
21-
// NOTE: this is the difference between `WithMetrics` and not -
22-
// the counters are instantiated either way, but they're only registered
23-
// in this variant.
24-
mustRegisterCache(name, cache)
25-
return cache, nil
26-
}
18+
const (
19+
promNamespace = "spicedb"
20+
promSubsystem = "cache"
21+
)
2722

2823
type valueAndCost[V any] struct {
2924
value V
3025
cost uint32
3126
}
3227

28+
// NewOtterCache creates an Otter-backed cache. It tracks its own metrics (see
29+
// Cache.GetMetrics) but does not export them; use NewOtterCacheWithMetrics to
30+
// also register those metrics with a Prometheus registerer.
3331
func NewOtterCache[K KeyString, V any](name string, config *Config) (Cache[K, V], error) {
32+
return newOtterCache[K, V](name, config)
33+
}
34+
35+
// NewOtterCacheWithMetrics creates an Otter-backed cache and registers its
36+
// metrics (labeled by name) with the given registerer. The metrics are
37+
// unregistered when the cache is Closed.
38+
func NewOtterCacheWithMetrics[K KeyString, V any](registerer prometheus.Registerer, name string, config *Config) (Cache[K, V], error) {
39+
cache, err := newOtterCache[K, V](name, config)
40+
if err != nil {
41+
return nil, err
42+
}
43+
if err := cache.registerMetrics(registerer); err != nil {
44+
return nil, err
45+
}
46+
return cache, nil
47+
}
48+
49+
func newOtterCache[K KeyString, V any](name string, config *Config) (*otterCache[K, V], error) {
3450
uintCost, err := safecast.Convert[uint64](config.MaxCost)
3551
if err != nil {
3652
return nil, err
@@ -50,10 +66,10 @@ func NewOtterCache[K KeyString, V any](name string, config *Config) (Cache[K, V]
5066

5167
cache, err := otter.New(opts)
5268
return &otterCache[K, V]{
53-
name,
54-
cache,
55-
otterMetrics{atomic.Uint64{}, counter},
56-
config.DefaultTTL,
69+
name: name,
70+
cache: cache,
71+
metrics: otterMetrics{atomic.Uint64{}, counter},
72+
ttl: config.DefaultTTL,
5773
}, err
5874
}
5975

@@ -62,6 +78,49 @@ type otterCache[K KeyString, V any] struct {
6278
cache *otter.Cache[string, valueAndCost[V]]
6379
metrics otterMetrics
6480
ttl time.Duration
81+
82+
// registerer and collectors are set when metrics are registered (via
83+
// NewOtterCacheWithMetrics) and used to unregister them on Close.
84+
registerer prometheus.Registerer
85+
collectors []prometheus.Collector
86+
}
87+
88+
// registerMetrics registers this cache's metrics, labeled by its name, with the
89+
// given registerer. The metrics are read from the cache at scrape time. On any
90+
// registration failure, already-registered metrics are rolled back.
91+
func (wtc *otterCache[K, V]) registerMetrics(registerer prometheus.Registerer) error {
92+
labels := prometheus.Labels{"cache": wtc.name}
93+
collectors := []prometheus.Collector{
94+
prometheus.NewCounterFunc(prometheus.CounterOpts{
95+
Namespace: promNamespace, Subsystem: promSubsystem, Name: "hits_total",
96+
Help: "Number of cache hits", ConstLabels: labels,
97+
}, func() float64 { return float64(wtc.metrics.Hits()) }),
98+
prometheus.NewCounterFunc(prometheus.CounterOpts{
99+
Namespace: promNamespace, Subsystem: promSubsystem, Name: "misses_total",
100+
Help: "Number of cache misses", ConstLabels: labels,
101+
}, func() float64 { return float64(wtc.metrics.Misses()) }),
102+
prometheus.NewCounterFunc(prometheus.CounterOpts{ //nolint:promlinter // don't add _total
103+
Namespace: promNamespace, Subsystem: promSubsystem, Name: "cost_added_bytes",
104+
Help: "Cost of entries added to the cache", ConstLabels: labels,
105+
}, func() float64 { return float64(wtc.metrics.CostAdded()) }),
106+
prometheus.NewCounterFunc(prometheus.CounterOpts{ //nolint:promlinter // don't add _total
107+
Namespace: promNamespace, Subsystem: promSubsystem, Name: "cost_evicted_bytes",
108+
Help: "Cost of entries evicted from the cache", ConstLabels: labels,
109+
}, func() float64 { return float64(wtc.metrics.CostEvicted()) }),
110+
}
111+
112+
for i, c := range collectors {
113+
if err := registerer.Register(c); err != nil {
114+
for _, registered := range collectors[:i] {
115+
registerer.Unregister(registered)
116+
}
117+
return fmt.Errorf("could not register metrics for cache %q: %w", wtc.name, err)
118+
}
119+
}
120+
121+
wtc.registerer = registerer
122+
wtc.collectors = collectors
123+
return nil
65124
}
66125

67126
func (wtc *otterCache[K, V]) GetTTL() time.Duration {
@@ -101,7 +160,12 @@ func (wtc *otterCache[K, V]) Wait() {}
101160
func (wtc *otterCache[K, V]) Close() {
102161
// Stops the pending goroutine that Otter spins off
103162
wtc.cache.StopAllGoroutines()
104-
unregisterCache(wtc.name)
163+
164+
// Unregister any metrics this cache registered with the registerer.
165+
for _, c := range wtc.collectors {
166+
wtc.registerer.Unregister(c)
167+
}
168+
wtc.collectors = nil
105169
}
106170

107171
type otterMetrics struct {

‎pkg/cache/cache_test.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"testing"
55
"time"
66

7+
"github.com/prometheus/client_golang/prometheus"
78
"github.com/stretchr/testify/require"
89
)
910

@@ -14,7 +15,7 @@ func TestCacheWithMetrics(t *testing.T) {
1415
}
1516

1617
t.Run("Set and Get", func(t *testing.T) {
17-
cache, err := NewOtterCacheWithMetrics[StringKey, string]("test-otter", config)
18+
cache, err := NewOtterCacheWithMetrics[StringKey, string](prometheus.NewRegistry(), "test-otter", config)
1819
require.NoError(t, err)
1920
defer cache.Close()
2021

@@ -45,7 +46,7 @@ func TestCacheWithMetrics(t *testing.T) {
4546
})
4647

4748
t.Run("Set same key with diff values", func(t *testing.T) {
48-
cache, err := NewOtterCacheWithMetrics[StringKey, string]("test-otter", config)
49+
cache, err := NewOtterCacheWithMetrics[StringKey, string](prometheus.NewRegistry(), "test-otter", config)
4950
require.NoError(t, err)
5051
defer cache.Close()
5152

@@ -66,7 +67,7 @@ func TestCacheWithMetrics(t *testing.T) {
6667
})
6768

6869
t.Run("Close multiple times", func(t *testing.T) {
69-
cache, err := NewOtterCacheWithMetrics[StringKey, string]("test-otter", config)
70+
cache, err := NewOtterCacheWithMetrics[StringKey, string](prometheus.NewRegistry(), "test-otter", config)
7071
require.NoError(t, err)
7172

7273
for range 10 {
@@ -75,15 +76,15 @@ func TestCacheWithMetrics(t *testing.T) {
7576
})
7677

7778
t.Run("GetTTL", func(t *testing.T) {
78-
cache, err := NewOtterCacheWithMetrics[StringKey, string]("test-otter", config)
79+
cache, err := NewOtterCacheWithMetrics[StringKey, string](prometheus.NewRegistry(), "test-otter", config)
7980
require.NoError(t, err)
8081
defer cache.Close()
8182

8283
require.Equal(t, 10*time.Hour, cache.GetTTL())
8384
})
8485

8586
t.Run("GetMetrics", func(t *testing.T) {
86-
cache, err := NewOtterCacheWithMetrics[StringKey, string]("test-otter", config)
87+
cache, err := NewOtterCacheWithMetrics[StringKey, string](prometheus.NewRegistry(), "test-otter", config)
8788
require.NoError(t, err)
8889
defer cache.Close()
8990

‎pkg/cache/cache_wasm.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ package cache
44

55
import (
66
"fmt"
7+
8+
"github.com/prometheus/client_golang/prometheus"
79
)
810

911
// NewStandardCache returns an error for caching.
@@ -12,10 +14,14 @@ func NewStandardCache[K KeyString, V any](config *Config) (Cache[K, V], error) {
1214
return nil, fmt.Errorf("caching is currently unsupported in WASM")
1315
}
1416

15-
func NewStandardCacheWithMetrics[K KeyString, V any](name string, config *Config) (Cache[K, V], error) {
17+
func NewStandardCacheWithMetrics[K KeyString, V any](registerer prometheus.Registerer, name string, config *Config) (Cache[K, V], error) {
1618
return nil, fmt.Errorf("caching is currently unsupported in WASM")
1719
}
1820

1921
func NewOtterCache[K KeyString, V any](name string, config *Config) (Cache[K, V], error) {
2022
return nil, fmt.Errorf("caching is currently unsupported in WASM")
2123
}
24+
25+
func NewOtterCacheWithMetrics[K KeyString, V any](registerer prometheus.Registerer, name string, config *Config) (Cache[K, V], error) {
26+
return nil, fmt.Errorf("caching is currently unsupported in WASM")
27+
}

‎pkg/cache/metrics.go‎

Lines changed: 0 additions & 87 deletions
This file was deleted.

‎pkg/cache/standard.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,16 @@ wasm.
99

1010
package cache
1111

12-
// NewStandardCache creates a new cache with the given configuration.
12+
import "github.com/prometheus/client_golang/prometheus"
13+
14+
// NewStandardCache creates a new cache with the given configuration. The cache
15+
// tracks its own metrics (see Cache.GetMetrics) but does not export them.
1316
func NewStandardCache[K KeyString, V any](config *Config) (Cache[K, V], error) {
1417
return NewOtterCache[K, V]("", config)
1518
}
1619

17-
func NewStandardCacheWithMetrics[K KeyString, V any](name string, config *Config) (Cache[K, V], error) {
18-
return NewOtterCacheWithMetrics[K, V](name, config)
20+
// NewStandardCacheWithMetrics creates a new cache and registers its metrics,
21+
// labeled by name, with the given registerer.
22+
func NewStandardCacheWithMetrics[K KeyString, V any](registerer prometheus.Registerer, name string, config *Config) (Cache[K, V], error) {
23+
return NewOtterCacheWithMetrics[K, V](registerer, name, config)
1924
}

‎pkg/cmd/server/cacheconfig.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
"github.com/ccoveille/go-safecast/v2"
1212
"github.com/dustin/go-humanize"
13+
"github.com/prometheus/client_golang/prometheus"
1314
"github.com/spf13/pflag"
1415

1516
"github.com/authzed/spicedb/pkg/cache"
@@ -46,8 +47,9 @@ func (cc *CacheConfig) WithRevisionParameters(
4647
return cc
4748
}
4849

49-
// CompleteCache translates the CLI cache config into a cache config.
50-
func CompleteCache[K cache.KeyString, V any](cc *CacheConfig) (cache.Cache[K, V], error) {
50+
// CompleteCache translates the CLI cache config into a cache. If metrics are
51+
// enabled for the cache, its metrics are registered with the given registerer.
52+
func CompleteCache[K cache.KeyString, V any](registerer prometheus.Registerer, cc *CacheConfig) (cache.Cache[K, V], error) {
5153
if !cc.Enabled || cc.MaxCost == "" || cc.MaxCost == "0%" {
5254
return cache.NoopCache[K, V](), nil
5355
}
@@ -72,7 +74,7 @@ func CompleteCache[K cache.KeyString, V any](cc *CacheConfig) (cache.Cache[K, V]
7274
}
7375

7476
if cc.Metrics {
75-
return cache.NewStandardCacheWithMetrics[K, V](cc.Name, &cache.Config{
77+
return cache.NewStandardCacheWithMetrics[K, V](registerer, cc.Name, &cache.Config{
7678
MaxCost: intMaxCost,
7779
DefaultTTL: cc.defaultTTL,
7880
})

0 commit comments

Comments
 (0)