Loki: Enable FIFO cache by default (#4519)

* Set default FIFO cache with reasonable defaults.

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

* Remove FIFO cache feature flag.

- We noticed that we always want that behavior, so the logic is better
  implemented in the `ApplyDinamicConfig` method
- Update documentation

* Move FIFO cache default behavior.

- Move FIFO cache default behavior implementation to ApplyDinamicConfig

* Add tests for the new FIFO cache default behavior.

- The tests tests the behavior of the four existing caches (query results, query
index, write dedupe, and chunk results)
- Add changelog entry
- Fix lint issues regarding import order
- Revert moving of EnableFifoCache check

* Apply suggestions from code review

- Change `EnableFifoCache` description to mention that it is only applied for chunk and query results cache

Co-authored-by: Owen Diehl <ow.diehl@gmail.com>

Co-authored-by: Mauro Stettler <mauro.stettler@gmail.com>
Co-authored-by: Owen Diehl <ow.diehl@gmail.com>

CHANGELOG.md

@@ -6,6 +6,7 @@
 * [4415](https://github.com/grafana/loki/pull/4415) **DylanGuedes**: Change default limits to common values
 * [4473](https://github.com/grafana/loki/pull/4473) **trevorwhitney**: Config: add object storage configuration to common config
 * [4425](https://github.com/grafana/loki/pull/4425) **trevorwhitney** and **slim-bean**: Add a ring for the query scheduler
+* [4519](https://github.com/grafana/loki/pull/4519) **DylanGuedes** and **replay**: Loki: Enable FIFO cache by default
 
 # 2.3.0 (2021/08/06)
 

docs/sources/configuration/_index.md

@@ -1794,7 +1794,7 @@ fifocache:
   # Maximum memory size of the cache in bytes. A unit suffix (KB, MB, GB) may be
   # applied.
   # CLI flag: -<prefix>.fifocache.max-size-bytes
-  [max_size_bytes: <string> | default = ""]
+  [max_size_bytes: <string> | default = "1GB"]
 
   # Maximum number of entries in the cache.
   # CLI flag: -<prefix>.fifocache.max-size-items
@@ -1802,7 +1802,7 @@ fifocache:
 
   # The expiry duration for the cache.
   # CLI flag: -<prefix>.fifocache.duration
-  [validity: <duration> | default = 0s]
+  [validity: <duration> | default = 1h]
 ```
 
 ## schema_config

pkg/loki/config_wrapper.go

@@ -5,13 +5,15 @@ import (
 	"fmt"
 	"reflect"
 
+	cortexcache "github.com/cortexproject/cortex/pkg/chunk/cache"
 	"github.com/grafana/dskit/flagext"
 	"github.com/pkg/errors"
 
+	"github.com/grafana/loki/pkg/storage/chunk/cache"
+	"github.com/grafana/loki/pkg/util/cfg"
+
 	loki_storage "github.com/grafana/loki/pkg/storage"
 	chunk_storage "github.com/grafana/loki/pkg/storage/chunk/storage"
-
-	"github.com/grafana/loki/pkg/util/cfg"
 )
 
 // ConfigWrapper is a struct containing the Loki config along with other values that can be set on the command line
@@ -84,8 +86,8 @@ func (c *ConfigWrapper) ApplyDynamicConfig() cfg.Source {
 		}
 
 		applyMemberlistConfig(r)
-		err := applyStorageConfig(r, &defaults)
-		if err != nil {
+
+		if err := applyStorageConfig(r, &defaults); err != nil {
 			return err
 		}
 
@@ -93,6 +95,8 @@ func (c *ConfigWrapper) ApplyDynamicConfig() cfg.Source {
 			betterBoltdbShipperDefaults(r, &defaults)
 		}
 
+		applyFIFOCacheConfig(r)
+
 		return nil
 	}
 }
@@ -229,3 +233,36 @@ func betterBoltdbShipperDefaults(cfg, defaults *ConfigWrapper) {
 		cfg.CompactorConfig.SharedStoreType = currentSchema.ObjectType
 	}
 }
+
+// applyFIFOCacheConfig turns on FIFO cache for the chunk store and for the query range results,
+// but only if no other cache storage is configured (redis or memcache).
+//
+// This behavior is only applied for the chunk store cache and for the query range results cache
+// (i.e: not applicable for the index queries cache or for the write dedupe cache).
+func applyFIFOCacheConfig(r *ConfigWrapper) {
+	chunkCacheConfig := r.ChunkStoreConfig.ChunkCacheConfig
+	if !cache.IsRedisSet(chunkCacheConfig) && !cache.IsMemcacheSet(chunkCacheConfig) {
+		r.ChunkStoreConfig.ChunkCacheConfig.EnableFifoCache = true
+	}
+
+	resultsCacheConfig := r.QueryRange.ResultsCacheConfig.CacheConfig
+	if !isRedisSet(resultsCacheConfig) && !isMemcacheSet(resultsCacheConfig) {
+		r.QueryRange.ResultsCacheConfig.CacheConfig.EnableFifoCache = true
+	}
+}
+
+// isRedisSet is a duplicate of cache.IsRedisSet.
+//
+// We had to duplicate this implementation because we have code relying on
+// loki/pkg/storage/chunk/cache and cortex/pkg/chunk/cache at the same time.
+func isRedisSet(cfg cortexcache.Config) bool {
+	return cfg.Redis.Endpoint != ""
+}
+
+// isMemcacheSet is a duplicate of cache.IsMemcacheSet.
+//
+// We had to duplicate this implementation because we have code relying on
+// loki/pkg/storage/chunk/cache and cortex/pkg/chunk/cache at the same time.
+func isMemcacheSet(cfg cortexcache.Config) bool {
+	return cfg.MemcacheClient.Addresses != "" || cfg.MemcacheClient.Host != ""
+}

pkg/loki/config_wrapper_test.go

@@ -21,41 +21,41 @@ import (
 	"github.com/grafana/loki/pkg/util/cfg"
 )
 
-func Test_ApplyDynamicConfig(t *testing.T) {
-	testContextExposingErrs := func(configFileString string, args []string) (error, ConfigWrapper, ConfigWrapper) {
-		config := ConfigWrapper{}
-		fs := flag.NewFlagSet(t.Name(), flag.PanicOnError)
-
-		file, err := ioutil.TempFile("", "config.yaml")
-		defer func() {
-			os.Remove(file.Name())
-		}()
-
-		require.NoError(t, err)
-		_, err = file.WriteString(configFileString)
-		require.NoError(t, err)
+func configWrapperFromYAML(t *testing.T, configFileString string, args []string) (ConfigWrapper, ConfigWrapper, error) {
+	config := ConfigWrapper{}
+	fs := flag.NewFlagSet(t.Name(), flag.PanicOnError)
+
+	file, err := ioutil.TempFile("", "config.yaml")
+	defer func() {
+		os.Remove(file.Name())
+	}()
+
+	require.NoError(t, err)
+	_, err = file.WriteString(configFileString)
+	require.NoError(t, err)
+
+	configFileArgs := []string{"-config.file", file.Name()}
+	if args == nil {
+		args = configFileArgs
+	} else {
+		args = append(args, configFileArgs...)
+	}
+	err = cfg.DynamicUnmarshal(&config, args, fs)
+	if err != nil {
+		return ConfigWrapper{}, ConfigWrapper{}, err
+	}
 
-		configFileArgs := []string{"-config.file", file.Name()}
-		if args == nil {
-			args = configFileArgs
-		} else {
-			args = append(args, configFileArgs...)
-		}
-		err = cfg.DynamicUnmarshal(&config, args, fs)
-		if err != nil {
-			return err, ConfigWrapper{}, ConfigWrapper{}
-		}
-
-		defaults := ConfigWrapper{}
-		freshFlags := flag.NewFlagSet(t.Name(), flag.PanicOnError)
-		err = cfg.DefaultUnmarshal(&defaults, args, freshFlags)
-		require.NoError(t, err)
+	defaults := ConfigWrapper{}
+	freshFlags := flag.NewFlagSet(t.Name(), flag.PanicOnError)
+	err = cfg.DefaultUnmarshal(&defaults, args, freshFlags)
+	require.NoError(t, err)
 
-		return nil, config, defaults
-	}
+	return config, defaults, nil
+}
 
+func Test_ApplyDynamicConfig(t *testing.T) {
 	testContext := func(configFileString string, args []string) (ConfigWrapper, ConfigWrapper) {
-		err, config, defaults := testContextExposingErrs(configFileString, args)
+		config, defaults, err := configWrapperFromYAML(t, configFileString, args)
 		require.NoError(t, err)
 
 		return config, defaults
@@ -208,7 +208,7 @@ memberlist:
       chunk_buffer_size: 27
       request_timeout: 5m`
 
-			err, _, _ := testContextExposingErrs(multipleConfig, nil)
+			_, _, err := configWrapperFromYAML(t, multipleConfig, nil)
 			assert.ErrorIs(t, err, ErrTooManyStorageConfigs)
 		})
 
@@ -656,7 +656,136 @@ schema_config:
 	})
 }
 
-// Can't use a totally empty yaml file or it causes weird behavior in the unmarhsalling
+func TestDefaultFIFOCacheBehavior(t *testing.T) {
+	t.Run("for the chunk cache config", func(t *testing.T) {
+		t.Run("no FIFO cache enabled by default if Redis is set", func(t *testing.T) {
+			configFileString := `---
+chunk_store_config:
+  chunk_cache_config: 
+    redis:
+      endpoint: endpoint.redis.org`
+
+			config, _, _ := configWrapperFromYAML(t, configFileString, nil)
+			assert.EqualValues(t, "endpoint.redis.org", config.ChunkStoreConfig.ChunkCacheConfig.Redis.Endpoint)
+			assert.False(t, config.ChunkStoreConfig.ChunkCacheConfig.EnableFifoCache)
+		})
+
+		t.Run("no FIFO cache enabled by default if Memcache is set", func(t *testing.T) {
+			configFileString := `---
+chunk_store_config:
+  chunk_cache_config: 
+    memcached_client:
+      host: host.memcached.org`
+
+			config, _, _ := configWrapperFromYAML(t, configFileString, nil)
+			assert.EqualValues(t, "host.memcached.org", config.ChunkStoreConfig.ChunkCacheConfig.MemcacheClient.Host)
+			assert.False(t, config.ChunkStoreConfig.ChunkCacheConfig.EnableFifoCache)
+		})
+
+		t.Run("FIFO cache is enabled by default if no other cache is set", func(t *testing.T) {
+			config, _, _ := configWrapperFromYAML(t, minimalConfig, nil)
+			assert.True(t, config.ChunkStoreConfig.ChunkCacheConfig.EnableFifoCache)
+		})
+	})
+
+	t.Run("for the write dedupe cache config", func(t *testing.T) {
+		t.Run("no FIFO cache enabled by default if Redis is set", func(t *testing.T) {
+			configFileString := `---
+chunk_store_config:
+  write_dedupe_cache_config:
+    redis:
+      endpoint: endpoint.redis.org`
+
+			config, _, _ := configWrapperFromYAML(t, configFileString, nil)
+			assert.EqualValues(t, "endpoint.redis.org", config.ChunkStoreConfig.WriteDedupeCacheConfig.Redis.Endpoint)
+			assert.False(t, config.ChunkStoreConfig.WriteDedupeCacheConfig.EnableFifoCache)
+		})
+
+		t.Run("no FIFO cache enabled by default if Memcache is set", func(t *testing.T) {
+			configFileString := `---
+chunk_store_config:
+  write_dedupe_cache_config:
+    memcached_client:
+      host: host.memcached.org`
+
+			config, _, _ := configWrapperFromYAML(t, configFileString, nil)
+			assert.EqualValues(t, "host.memcached.org", config.ChunkStoreConfig.WriteDedupeCacheConfig.MemcacheClient.Host)
+			assert.False(t, config.ChunkStoreConfig.WriteDedupeCacheConfig.EnableFifoCache)
+		})
+
+		t.Run("no FIFO cache is enabled by default even if no other cache is set", func(t *testing.T) {
+			config, _, _ := configWrapperFromYAML(t, minimalConfig, nil)
+			assert.False(t, config.ChunkStoreConfig.WriteDedupeCacheConfig.EnableFifoCache)
+		})
+	})
+
+	t.Run("for the index queries cache config", func(t *testing.T) {
+		t.Run("no FIFO cache enabled by default if Redis is set", func(t *testing.T) {
+			configFileString := `---
+storage_config:
+  index_queries_cache_config:
+    redis:
+      endpoint: endpoint.redis.org`
+
+			config, _, _ := configWrapperFromYAML(t, configFileString, nil)
+			assert.EqualValues(t, "endpoint.redis.org", config.StorageConfig.IndexQueriesCacheConfig.Redis.Endpoint)
+			assert.False(t, config.StorageConfig.IndexQueriesCacheConfig.EnableFifoCache)
+		})
+
+		t.Run("no FIFO cache enabled by default if Memcache is set", func(t *testing.T) {
+			configFileString := `---
+storage_config:
+  index_queries_cache_config:
+    memcached_client:
+      host: host.memcached.org`
+
+			config, _, _ := configWrapperFromYAML(t, configFileString, nil)
+
+			assert.EqualValues(t, "host.memcached.org", config.StorageConfig.IndexQueriesCacheConfig.MemcacheClient.Host)
+			assert.False(t, config.StorageConfig.IndexQueriesCacheConfig.EnableFifoCache)
+		})
+
+		t.Run("no FIFO cache is enabled by default even if no other cache is set", func(t *testing.T) {
+			config, _, _ := configWrapperFromYAML(t, minimalConfig, nil)
+			assert.False(t, config.StorageConfig.IndexQueriesCacheConfig.EnableFifoCache)
+		})
+	})
+
+	t.Run("for the query range results cache config", func(t *testing.T) {
+		t.Run("no FIFO cache enabled by default if Redis is set", func(t *testing.T) {
+			configFileString := `---
+query_range:
+  results_cache: 
+    cache:
+      redis:
+        endpoint: endpoint.redis.org`
+
+			config, _, _ := configWrapperFromYAML(t, configFileString, nil)
+			assert.EqualValues(t, config.QueryRange.CacheConfig.Redis.Endpoint, "endpoint.redis.org")
+			assert.False(t, config.QueryRange.CacheConfig.EnableFifoCache)
+		})
+
+		t.Run("no FIFO cache enabled by default if Memcache is set", func(t *testing.T) {
+			configFileString := `---
+query_range:
+  results_cache: 
+    cache:
+      memcached_client:
+        host: memcached.host.org`
+
+			config, _, _ := configWrapperFromYAML(t, configFileString, nil)
+			assert.EqualValues(t, "memcached.host.org", config.QueryRange.CacheConfig.MemcacheClient.Host)
+			assert.False(t, config.QueryRange.CacheConfig.EnableFifoCache)
+		})
+
+		t.Run("FIFO cache is enabled by default if no other cache is set", func(t *testing.T) {
+			config, _, _ := configWrapperFromYAML(t, minimalConfig, nil)
+			assert.True(t, config.QueryRange.CacheConfig.EnableFifoCache)
+		})
+	})
+}
+
+// Can't use a totally empty yaml file or it causes weird behavior in the unmarhsalling.
 const minimalConfig = `---
 schema_config:
   configs:

pkg/storage/chunk/cache/cache.go

@@ -49,9 +49,8 @@ func (cfg *Config) RegisterFlagsWithPrefix(prefix string, description string, f
 	cfg.MemcacheClient.RegisterFlagsWithPrefix(prefix, description, f)
 	cfg.Redis.RegisterFlagsWithPrefix(prefix, description, f)
 	cfg.Fifocache.RegisterFlagsWithPrefix(prefix, description, f)
-
-	f.BoolVar(&cfg.EnableFifoCache, prefix+"cache.enable-fifocache", false, description+"Enable in-memory cache.")
-	f.DurationVar(&cfg.DefaultValidity, prefix+"default-validity", 0, description+"The default validity of entries for caches unless overridden.")
+	f.DurationVar(&cfg.DefaultValidity, prefix+"default-validity", time.Hour, description+"The default validity of entries for caches unless overridden.")
+	f.BoolVar(&cfg.EnableFifoCache, prefix+"cache.enable-fifocache", false, description+"Enable in-memory cache (auto-enabled for the chunks & query results cache if no other cache is configured).")
 
 	cfg.Prefix = prefix
 }
@@ -60,6 +59,21 @@ func (cfg *Config) Validate() error {
 	return cfg.Fifocache.Validate()
 }
 
+// IsMemcacheSet returns whether a non empty Memcache config is set or not, based on the configured
+// host or addresses.
+//
+// Internally, this function is used to set Memcache as the cache storage to be used.
+func IsMemcacheSet(cfg Config) bool {
+	return cfg.MemcacheClient.Host != "" || cfg.MemcacheClient.Addresses != ""
+}
+
+// IsRedisSet returns whether a non empty Redis config is set or not, based on the configured endpoint.
+//
+// Internally, this function is used to set Redis as the cache storage to be used.
+func IsRedisSet(cfg Config) bool {
+	return cfg.Redis.Endpoint != ""
+}
+
 // New creates a new Cache using Config.
 func New(cfg Config, reg prometheus.Registerer, logger log.Logger) (Cache, error) {
 	if cfg.Cache != nil {
@@ -78,11 +92,11 @@ func New(cfg Config, reg prometheus.Registerer, logger log.Logger) (Cache, error
 		}
 	}
 
-	if (cfg.MemcacheClient.Host != "" || cfg.MemcacheClient.Addresses != "") && cfg.Redis.Endpoint != "" {
+	if IsMemcacheSet(cfg) && IsRedisSet(cfg) {
 		return nil, errors.New("use of multiple cache storage systems is not supported")
 	}
 
-	if cfg.MemcacheClient.Host != "" || cfg.MemcacheClient.Addresses != "" {
+	if IsMemcacheSet(cfg) {
 		if cfg.Memcache.Expiration == 0 && cfg.DefaultValidity != 0 {
 			cfg.Memcache.Expiration = cfg.DefaultValidity
 		}
@@ -94,7 +108,7 @@ func New(cfg Config, reg prometheus.Registerer, logger log.Logger) (Cache, error
 		caches = append(caches, NewBackground(cacheName, cfg.Background, Instrument(cacheName, cache, reg), reg))
 	}
 
-	if cfg.Redis.Endpoint != "" {
+	if IsRedisSet(cfg) {
 		if cfg.Redis.Expiration == 0 && cfg.DefaultValidity != 0 {
 			cfg.Redis.Expiration = cfg.DefaultValidity
 		}

pkg/storage/chunk/cache/fifo_cache.go

@@ -39,9 +39,9 @@ type FifoCacheConfig struct {
 
 // RegisterFlagsWithPrefix adds the flags required to config this to the given FlagSet
 func (cfg *FifoCacheConfig) RegisterFlagsWithPrefix(prefix, description string, f *flag.FlagSet) {
-	f.StringVar(&cfg.MaxSizeBytes, prefix+"fifocache.max-size-bytes", "", description+"Maximum memory size of the cache in bytes. A unit suffix (KB, MB, GB) may be applied.")
+	f.StringVar(&cfg.MaxSizeBytes, prefix+"fifocache.max-size-bytes", "1GB", description+"Maximum memory size of the cache in bytes. A unit suffix (KB, MB, GB) may be applied.")
 	f.IntVar(&cfg.MaxSizeItems, prefix+"fifocache.max-size-items", 0, description+"Maximum number of entries in the cache.")
-	f.DurationVar(&cfg.Validity, prefix+"fifocache.duration", 0, description+"The expiry duration for the cache.")
+	f.DurationVar(&cfg.Validity, prefix+"fifocache.duration", time.Hour, description+"The expiry duration for the cache.")
 
 	f.IntVar(&cfg.DeprecatedSize, prefix+"fifocache.size", 0, "Deprecated (use max-size-items or max-size-bytes instead): "+description+"The number of entries to cache. ")
 }

pkg/storage/chunk/chunk_store.go

@@ -72,7 +72,7 @@ type StoreConfig struct {
 func (cfg *StoreConfig) RegisterFlags(f *flag.FlagSet) {
 	cfg.ChunkCacheConfig.RegisterFlagsWithPrefix("store.chunks-cache.", "Cache config for chunks. ", f)
 	f.BoolVar(&cfg.chunkCacheStubs, "store.chunks-cache.cache-stubs", false, "If true, don't write the full chunk to cache, just a stub entry.")
-	cfg.WriteDedupeCacheConfig.RegisterFlagsWithPrefix("store.index-cache-write.", "Cache config for index entry writing. ", f)
+	cfg.WriteDedupeCacheConfig.RegisterFlagsWithPrefix("store.index-cache-write.", "Cache config for index entry writing.", f)
 
 	f.Var(&cfg.CacheLookupsOlderThan, "store.cache-lookups-older-than", "Cache index entries older than this period. 0 to disable.")
 }

pkg/storage/chunk/chunk_store_test.go

@@ -890,9 +890,7 @@ func TestStore_DeleteChunk(t *testing.T) {
 	nonExistentChunk := dummyChunkForEncoding(model.Now(), metric3, encoding.Varbit, 200)
 
 	fooMetricNameMatcher, err := parser.ParseMetricSelector(`foo`)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
 	for _, tc := range []struct {
 		name                           string

pkg/storage/chunk/storage/factory.go

@@ -114,7 +114,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	cfg.GrpcConfig.RegisterFlags(f)
 
 	f.StringVar(&cfg.Engine, "store.engine", "chunks", "The storage engine to use: chunks or blocks.")
-	cfg.IndexQueriesCacheConfig.RegisterFlagsWithPrefix("store.index-cache-read.", "Cache config for index entry reading. ", f)
+	cfg.IndexQueriesCacheConfig.RegisterFlagsWithPrefix("store.index-cache-read.", "Cache config for index entry reading.", f)
 	f.DurationVar(&cfg.IndexCacheValidity, "store.index-cache-validity", 5*time.Minute, "Cache validity for active index entries. Should be no higher than -ingester.max-chunk-idle.")
 	f.BoolVar(&cfg.DisableBroadIndexQueries, "store.disable-broad-index-queries", false, "Disable broad index queries which results in reduced cache usage and faster query performance at the expense of somewhat higher QPS on the index store.")
 }