Skip to content

Commit

Permalink
Remove global metrics registry and global logger in cache package (gr…
Browse files Browse the repository at this point in the history 
…afana#2903)

* remove global metrics registry and util.Logger in cache package

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* address comments and update changelog

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* use util.Logger instead of noop logger

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* use nil prometheus registry and address some nits

Signed-off-by: Ben Ye <yb532204897@gmail.com>
  • Loading branch information
@yeya24
yeya24 authored Jul 27, 2020
1 parent 32b5a9b commit e85298e
Show file tree
Hide file tree
Showing 20 changed files with 252 additions and 225 deletions.
38 changes: 18 additions & 20 deletions cache/background.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,6 @@ import (
"github.com/prometheus/client_golang/prometheus/promauto"
)

var (
droppedWriteBack = promauto.NewCounterVec(prometheus.CounterOpts{
Namespace: "cortex",
Name: "cache_dropped_background_writes_total",
Help: "Total count of dropped write backs to cache.",
}, []string{"name"})
queueLength = promauto.NewGaugeVec(prometheus.GaugeOpts{
Namespace: "cortex",
Name: "cache_background_queue_length",
Help: "Length of the cache background write queue.",
}, []string{"name"})
)

// BackgroundConfig is config for a Background Cache.
type BackgroundConfig struct {
WriteBackGoroutines int `yaml:"writeback_goroutines"`
Expand Down Expand Up @@ -54,14 +41,25 @@ type backgroundWrite struct {
}

// NewBackground returns a new Cache that does stores on background goroutines.
func NewBackground(name string, cfg BackgroundConfig, cache Cache) Cache {
func NewBackground(name string, cfg BackgroundConfig, cache Cache, reg prometheus.Registerer) Cache {
c := &backgroundCache{
Cache: cache,
quit: make(chan struct{}),
bgWrites: make(chan backgroundWrite, cfg.WriteBackBuffer),
name: name,
droppedWriteBack: droppedWriteBack.WithLabelValues(name),
queueLength: queueLength.WithLabelValues(name),
Cache: cache,
quit: make(chan struct{}),
bgWrites: make(chan backgroundWrite, cfg.WriteBackBuffer),
name: name,
droppedWriteBack: promauto.With(reg).NewCounter(prometheus.CounterOpts{
Namespace: "cortex",
Name: "cache_dropped_background_writes_total",
Help: "Total count of dropped write backs to cache.",
ConstLabels: prometheus.Labels{"name": name},
}),

queueLength: promauto.With(reg).NewGauge(prometheus.GaugeOpts{
Namespace: "cortex",
Name: "cache_background_queue_length",
Help: "Length of the cache background write queue.",
ConstLabels: prometheus.Labels{"name": name},
}),
}

c.wg.Add(cfg.WriteBackGoroutines)
Expand Down
2 changes: 1 addition & 1 deletion cache/background_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ func TestBackground(t *testing.T) {
c := cache.NewBackground("mock", cache.BackgroundConfig{
WriteBackGoroutines: 1,
WriteBackBuffer: 100,
}, cache.NewMockCache())
}, cache.NewMockCache(), nil)

keys, chunks := fillCache(t, c)
cache.Flush(c)
Expand Down
19 changes: 10 additions & 9 deletions cache/cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"flag"
"time"

"github.com/go-kit/kit/log"
"github.com/prometheus/client_golang/prometheus"
)

Expand Down Expand Up @@ -60,7 +61,7 @@ func (cfg *Config) Validate() error {
}

// New creates a new Cache using Config.
func New(cfg Config) (Cache, error) {
func New(cfg Config, reg prometheus.Registerer, logger log.Logger) (Cache, error) {
if cfg.Cache != nil {
return cfg.Cache, nil
}
Expand All @@ -72,8 +73,8 @@ func New(cfg Config) (Cache, error) {
cfg.Fifocache.Validity = cfg.DefaultValidity
}

if cache := NewFifoCache(cfg.Prefix+"fifocache", cfg.Fifocache); cache != nil {
caches = append(caches, Instrument(cfg.Prefix+"fifocache", cache))
if cache := NewFifoCache(cfg.Prefix+"fifocache", cfg.Fifocache, reg, logger); cache != nil {
caches = append(caches, Instrument(cfg.Prefix+"fifocache", cache, reg))
}
}

Expand All @@ -86,25 +87,25 @@ func New(cfg Config) (Cache, error) {
cfg.Memcache.Expiration = cfg.DefaultValidity
}

client := NewMemcachedClient(cfg.MemcacheClient, cfg.Prefix, prometheus.DefaultRegisterer)
cache := NewMemcached(cfg.Memcache, client, cfg.Prefix)
client := NewMemcachedClient(cfg.MemcacheClient, cfg.Prefix, reg, logger)
cache := NewMemcached(cfg.Memcache, client, cfg.Prefix, reg, logger)

cacheName := cfg.Prefix + "memcache"
caches = append(caches, NewBackground(cacheName, cfg.Background, Instrument(cacheName, cache)))
caches = append(caches, NewBackground(cacheName, cfg.Background, Instrument(cacheName, cache, reg), reg))
}

if cfg.Redis.Endpoint != "" {
if cfg.Redis.Expiration == 0 && cfg.DefaultValidity != 0 {
cfg.Redis.Expiration = cfg.DefaultValidity
}
cacheName := cfg.Prefix + "redis"
cache := NewRedisCache(cfg.Redis, cacheName, nil)
caches = append(caches, NewBackground(cacheName, cfg.Background, Instrument(cacheName, cache)))
cache := NewRedisCache(cfg.Redis, cacheName, nil, logger)
caches = append(caches, NewBackground(cacheName, cfg.Background, Instrument(cacheName, cache, reg), reg))
}

cache := NewTiered(caches)
if len(caches) > 1 {
cache = Instrument(cfg.Prefix+"tiered", cache)
cache = Instrument(cfg.Prefix+"tiered", cache, reg)
}
return cache, nil
}
11 changes: 7 additions & 4 deletions cache/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"
"time"

"github.com/go-kit/kit/log"
"github.com/prometheus/common/model"
"github.com/prometheus/prometheus/pkg/labels"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -158,25 +159,27 @@ func testCache(t *testing.T, cache cache.Cache) {

func TestMemcache(t *testing.T) {
t.Run("Unbatched", func(t *testing.T) {
cache := cache.NewMemcached(cache.MemcachedConfig{}, newMockMemcache(), "test")
cache := cache.NewMemcached(cache.MemcachedConfig{}, newMockMemcache(),
"test", nil, log.NewNopLogger())
testCache(t, cache)
})

t.Run("Batched", func(t *testing.T) {
cache := cache.NewMemcached(cache.MemcachedConfig{
BatchSize: 10,
Parallelism: 3,
}, newMockMemcache(), "test")
}, newMockMemcache(), "test", nil, log.NewNopLogger())
testCache(t, cache)
})
}

func TestFifoCache(t *testing.T) {
cache := cache.NewFifoCache("test", cache.FifoCacheConfig{MaxSizeItems: 1e3, Validity: 1 * time.Hour})
cache := cache.NewFifoCache("test", cache.FifoCacheConfig{MaxSizeItems: 1e3, Validity: 1 * time.Hour},
nil, log.NewNopLogger())
testCache(t, cache)
}

func TestSnappyCache(t *testing.T) {
cache := cache.NewSnappy(cache.NewMockCache())
cache := cache.NewSnappy(cache.NewMockCache(), log.NewNopLogger())
testCache(t, cache)
}
138 changes: 67 additions & 71 deletions cache/fifo_cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"unsafe"

"github.com/dustin/go-humanize"
"github.com/go-kit/kit/log"
"github.com/go-kit/kit/log/level"
"github.com/pkg/errors"
"github.com/prometheus/client_golang/prometheus"
Expand All @@ -18,64 +19,6 @@ import (
"github.com/cortexproject/cortex/pkg/util/flagext"
)

var (
cacheEntriesAdded = promauto.NewCounterVec(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "added_total",
Help: "The total number of Put calls on the cache",
}, []string{"cache"})

cacheEntriesAddedNew = promauto.NewCounterVec(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "added_new_total",
Help: "The total number of new entries added to the cache",
}, []string{"cache"})

cacheEntriesEvicted = promauto.NewCounterVec(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "evicted_total",
Help: "The total number of evicted entries",
}, []string{"cache"})

cacheEntriesCurrent = promauto.NewGaugeVec(prometheus.GaugeOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "entries",
Help: "The total number of entries",
}, []string{"cache"})

cacheTotalGets = promauto.NewCounterVec(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "gets_total",
Help: "The total number of Get calls",
}, []string{"cache"})

cacheTotalMisses = promauto.NewCounterVec(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "misses_total",
Help: "The total number of Get calls that had no valid entry",
}, []string{"cache"})

cacheStaleGets = promauto.NewCounterVec(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "stale_gets_total",
Help: "The total number of Get calls that had an entry which expired",
}, []string{"cache"})

cacheMemoryBytes = promauto.NewGaugeVec(prometheus.GaugeOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "memory_bytes",
Help: "The current cache size in bytes",
}, []string{"cache"})
)

const (
elementSize = int(unsafe.Sizeof(list.Element{}))
elementPrtSize = int(unsafe.Sizeof(&list.Element{}))
Expand Down Expand Up @@ -149,20 +92,19 @@ type cacheEntry struct {
}

// NewFifoCache returns a new initialised FifoCache of size.
// TODO(bwplotka): Fix metrics, get them out of globals, separate or allow prefixing.
func NewFifoCache(name string, cfg FifoCacheConfig) *FifoCache {
func NewFifoCache(name string, cfg FifoCacheConfig, reg prometheus.Registerer, logger log.Logger) *FifoCache {
util.WarnExperimentalUse("In-memory (FIFO) cache")

if cfg.DeprecatedSize > 0 {
flagext.DeprecatedFlagsUsed.Inc()
level.Warn(util.Logger).Log("msg", "running with DEPRECATED flag fifocache.size, use fifocache.max-size-items or fifocache.max-size-bytes instead", "cache", name)
level.Warn(logger).Log("msg", "running with DEPRECATED flag fifocache.size, use fifocache.max-size-items or fifocache.max-size-bytes instead", "cache", name)
cfg.MaxSizeItems = cfg.DeprecatedSize
}
maxSizeBytes, _ := parsebytes(cfg.MaxSizeBytes)

if maxSizeBytes == 0 && cfg.MaxSizeItems == 0 {
// zero cache capacity - no need to create cache
level.Warn(util.Logger).Log("msg", "neither fifocache.max-size-bytes nor fifocache.max-size-items is set", "cache", name)
level.Warn(logger).Log("msg", "neither fifocache.max-size-bytes nor fifocache.max-size-items is set", "cache", name)
return nil
}
return &FifoCache{
Expand All @@ -172,15 +114,69 @@ func NewFifoCache(name string, cfg FifoCacheConfig) *FifoCache {
entries: make(map[string]*list.Element),
lru: list.New(),

// TODO(bwplotka): There might be simple cache.Cache wrapper for those.
entriesAdded: cacheEntriesAdded.WithLabelValues(name),
entriesAddedNew: cacheEntriesAddedNew.WithLabelValues(name),
entriesEvicted: cacheEntriesEvicted.WithLabelValues(name),
entriesCurrent: cacheEntriesCurrent.WithLabelValues(name),
totalGets: cacheTotalGets.WithLabelValues(name),
totalMisses: cacheTotalMisses.WithLabelValues(name),
staleGets: cacheStaleGets.WithLabelValues(name),
memoryBytes: cacheMemoryBytes.WithLabelValues(name),
entriesAdded: promauto.With(reg).NewCounter(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "added_total",
Help: "The total number of Put calls on the cache",
ConstLabels: prometheus.Labels{"cache": name},
}),

entriesAddedNew: promauto.With(reg).NewCounter(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "added_new_total",
Help: "The total number of new entries added to the cache",
ConstLabels: prometheus.Labels{"cache": name},
}),

entriesEvicted: promauto.With(reg).NewCounter(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "evicted_total",
Help: "The total number of evicted entries",
ConstLabels: prometheus.Labels{"cache": name},
}),

entriesCurrent: promauto.With(reg).NewGauge(prometheus.GaugeOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "entries",
Help: "The total number of entries",
ConstLabels: prometheus.Labels{"cache": name},
}),

totalGets: promauto.With(reg).NewCounter(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "gets_total",
Help: "The total number of Get calls",
ConstLabels: prometheus.Labels{"cache": name},
}),

totalMisses: promauto.With(reg).NewCounter(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "misses_total",
Help: "The total number of Get calls that had no valid entry",
ConstLabels: prometheus.Labels{"cache": name},
}),

staleGets: promauto.With(reg).NewCounter(prometheus.CounterOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "stale_gets_total",
Help: "The total number of Get calls that had an entry which expired",
ConstLabels: prometheus.Labels{"cache": name},
}),

memoryBytes: promauto.With(reg).NewGauge(prometheus.GaugeOpts{
Namespace: "querier",
Subsystem: "cache",
Name: "memory_bytes",
Help: "The current cache size in bytes",
ConstLabels: prometheus.Labels{"cache": name},
}),
}
}

Expand Down
5 changes: 3 additions & 2 deletions cache/fifo_cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"testing"
"time"

"github.com/go-kit/kit/log"
"github.com/prometheus/client_golang/prometheus/testutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -37,7 +38,7 @@ func TestFifoCacheEviction(t *testing.T) {
}

for _, test := range tests {
c := NewFifoCache(test.name, test.cfg)
c := NewFifoCache(test.name, test.cfg, nil, log.NewNopLogger())
ctx := context.Background()

// Check put / get works
Expand Down Expand Up @@ -185,7 +186,7 @@ func TestFifoCacheExpiry(t *testing.T) {
}

for _, test := range tests {
c := NewFifoCache(test.name, test.cfg)
c := NewFifoCache(test.name, test.cfg, nil, log.NewNopLogger())
ctx := context.Background()

c.Store(ctx,
Expand Down
Loading

0 comments on commit e85298e

Please sign in to comment.