Remove global metrics registry and global logger in cache package #2903

yeya24 · 2020-07-20T20:00:24Z

What this PR does:

This pr removes global prometheus registries and util.logger in the cache package. This is good for reusing this package in other project like Thanos.

Which issue(s) this PR fixes:
Fixes #

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci

Epic job! Well done and thanks! ❤️

I left few comments, but overall the direction you're going looks good. Let's do it! 🚀

pkg/chunk/cache/background.go

pkg/chunk/cache/fifo_cache.go

pkg/chunk/cache/memcached.go

pkg/chunk/cache/memcached_client.go

pracucci · 2020-07-22T14:11:56Z

pkg/chunk/storage/factory.go

@@ -117,19 +118,20 @@ func (cfg *Config) Validate() error {
 func NewStore(cfg Config, storeCfg chunk.StoreConfig, schemaCfg chunk.SchemaConfig, limits StoreLimits, reg prometheus.Registerer, cacheGenNumLoader chunk.CacheGenNumLoader) (chunk.Store, error) {
 	chunkMetrics := newChunkClientMetrics(reg)

-	indexReadCache, err := cache.New(cfg.IndexQueriesCacheConfig)
+	logger := log.NewNopLogger()


Uhm, no, we should pick it in input, not using the nop logger :)

Thanks, yes what I am doing here is totally wrong :(.

But the logger is initialized in the service module, I need to do some work to propagate that logger here

pkg/chunk/cache/instrumented.go

yeya24 · 2020-07-22T14:30:13Z

Thanks for the review! @pracucci
One problem I meet is that we are using util.WarnExperimentalUse in the cache package, for example https://github.com/cortexproject/cortex/blob/ac796657c651e399e7e37f3e1cadbeed5777d21b/pkg/chunk/cache/redis_cache.go.

Is it possible to get rid of this? This relates to the counter metric here

cortex/pkg/util/experimental.go

Line 9 in dd23765

var experimentalFeaturesInUse = promauto.NewCounter(

pracucci · 2020-07-22T15:17:59Z

WarnExperimentalUse

I think we wanna keep it. If the problem is the logger, you could just pass the logger to it (in callers outside the pkg/chunk/cache, where logger is unavailable, you can just use util.Logger). If the problem is the metric, is it really a big deal in Thanos? Doesn't look dramatic exporting cortex_experimental_features_in_use_total.

Signed-off-by: Ben Ye <[email protected]>

yeya24 · 2020-07-24T03:18:25Z

WarnExperimentalUse

I think we wanna keep it. If the problem is the logger, you could just pass the logger to it (in callers outside the pkg/chunk/cache, where logger is unavailable, you can just use util.Logger). If the problem is the metric, is it really a big deal in Thanos? Doesn't look dramatic exporting cortex_experimental_features_in_use_total.

Thanks. I think it is okay. Any thoughts? @bwplotka

I addressed all the comments. PTAL when you get a chance 😄 @pracucci

Signed-off-by: Ben Ye <[email protected]>

pracucci

Thanks @yeya24 for this amazing work! LGTM 🚀 @pstibrany could take a second look, please?

bwplotka

Nice, it looks good from my side as well 👍

pstibrany

Thank you! While I've left some nit-picking comments, nothing that would prevent from merging.

pkg/chunk/cache/fifo_cache.go

pstibrany · 2020-07-27T11:48:49Z

pkg/chunk/cache/fifo_cache_test.go

@@ -37,7 +39,7 @@ func TestFifoCacheEviction(t *testing.T) {
 	}

 	for _, test := range tests {
-		c := NewFifoCache(test.name, test.cfg)
+		c := NewFifoCache(test.name, test.cfg, prometheus.NewRegistry(), log.NewNopLogger())


Nit: I would prefer to pass nil as registerer, if we're not testing metrics.

pstibrany · 2020-07-27T11:53:48Z

pkg/chunk/cache/memcached.go

-// TODO(bwplotka): Remove globals & util packages from cache package entirely (e.g util.Logger).
-func NewMemcached(cfg MemcachedConfig, client MemcachedClient, name string) *Memcached {
+func NewMemcached(cfg MemcachedConfig, client MemcachedClient, name string, reg prometheus.Registerer, logger log.Logger) *Memcached {
+	memcacheRequestDuration := promauto.With(reg).NewHistogramVec(prometheus.HistogramOpts{


Nit: this could be inlined.

pstibrany · 2020-07-27T11:55:02Z

pkg/chunk/cache/instrumented.go

-	prometheus.MustRegister(hits)
-	prometheus.MustRegister(valueSize)
-}
+	requestDuration := promauto.With(reg).NewHistogramVec(prometheus.HistogramOpts{


Nit: This can be inlined.

Signed-off-by: Ben Ye <[email protected]>

pull-request-size bot added the size/L label Jul 20, 2020

yeya24 mentioned this pull request Jul 20, 2020

Action Plan for Thanos query-frontend (Response Caching, Splitting queries) thanos-io/thanos#2454

Closed

yeya24 changed the title ~~Remove global metrics registry and util in cache package~~ WIP: Remove global metrics registry and util in cache package Jul 20, 2020

yeya24 force-pushed the no-global-registry-cache branch from f6f4540 to c7cd9b2 Compare July 20, 2020 23:07

pracucci reviewed Jul 22, 2020

View reviewed changes

remove global metrics registry and util.Logger in cache package

9214545

Signed-off-by: Ben Ye <[email protected]>

yeya24 force-pushed the no-global-registry-cache branch from c7cd9b2 to 780e430 Compare July 24, 2020 03:13

yeya24 marked this pull request as ready for review July 24, 2020 03:14

address comments and update changelog

b64980a

Signed-off-by: Ben Ye <[email protected]>

yeya24 force-pushed the no-global-registry-cache branch from 780e430 to b64980a Compare July 24, 2020 03:17

pull-request-size bot added size/XL and removed size/L labels Jul 27, 2020

yeya24 changed the title ~~WIP: Remove global metrics registry and util in cache package~~ Remove global metrics registry and util in cache package Jul 27, 2020

use util.Logger instead of noop logger

8ae218c

Signed-off-by: Ben Ye <[email protected]>

yeya24 force-pushed the no-global-registry-cache branch from 2b41b66 to 8ae218c Compare July 27, 2020 02:00

yeya24 changed the title ~~Remove global metrics registry and util in cache package~~ Remove global metrics registry and global logger in cache package Jul 27, 2020

pracucci approved these changes Jul 27, 2020

View reviewed changes

bwplotka approved these changes Jul 27, 2020

View reviewed changes

pstibrany approved these changes Jul 27, 2020

View reviewed changes

use nil prometheus registry and address some nits

fff97b9

Signed-off-by: Ben Ye <[email protected]>

pstibrany merged commit d439006 into cortexproject:master Jul 27, 2020

yeya24 deleted the no-global-registry-cache branch July 27, 2020 13:27

yeya24 mentioned this pull request Aug 5, 2020

Bump vendored cortex version thanos-io/thanos#2981

Merged

2 tasks

bboreham mentioned this pull request Sep 6, 2021

Chore: Upgrade weaveworks/common #4462

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove global metrics registry and global logger in cache package #2903

Remove global metrics registry and global logger in cache package #2903

yeya24 commented Jul 20, 2020 •

edited

Loading

pracucci left a comment

pracucci Jul 22, 2020

yeya24 Jul 22, 2020

yeya24 commented Jul 22, 2020

pracucci commented Jul 22, 2020

yeya24 commented Jul 24, 2020 •

edited

Loading

pracucci left a comment

bwplotka left a comment

pstibrany left a comment

pstibrany Jul 27, 2020

pstibrany Jul 27, 2020

pstibrany Jul 27, 2020

Remove global metrics registry and global logger in cache package #2903

Remove global metrics registry and global logger in cache package #2903

Conversation

yeya24 commented Jul 20, 2020 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

pracucci Jul 22, 2020

Choose a reason for hiding this comment

yeya24 Jul 22, 2020

Choose a reason for hiding this comment

yeya24 commented Jul 22, 2020

pracucci commented Jul 22, 2020

yeya24 commented Jul 24, 2020 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany Jul 27, 2020

Choose a reason for hiding this comment

pstibrany Jul 27, 2020

Choose a reason for hiding this comment

pstibrany Jul 27, 2020

Choose a reason for hiding this comment

yeya24 commented Jul 20, 2020 •

edited

Loading

yeya24 commented Jul 24, 2020 •

edited

Loading