Loki: Enable FIFO cache by default

[DylanGuedes]

Co-Authored-By: Mauro Stettler mauro.stettler@gmail.com

What this PR does / why we need it:

• Enable FIFO cache by default for chunks store and for query results if no other cache storage is set
• Sets defaults for the cache validity and for the FIFO cache's max-size-bytes

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:

• This was based on this draft PR

Checklist

• Documentation added
• Tests updated

[KMiller-Grafana]

Docs change of the default values LGTM.

[DylanGuedes]

Here I'm moving this block to a separate function to reuse it in other tests.

edit: I've also moved the returned error from first to last param

[owen-d]

Still thinking about this one. I don't think we'll want write_dedupe_cache to automatically be enabled: it's unused with boltdb shipper and is only helpful coordinating which chunks have been flushed between multiple nodes (local fifo cache isn't helpful here). I'm leaning towards just enabling this for chunks,index, and query-results caches. Then again, enabling a local write-dedupe-cache won't do anything aside take some cpu cycles and memory, so it may just be simpler to eat the performance hit for config simplicity. WDYT @slim-bean ?

[owen-d]

If the structs are identical, can we instead just call lokicache.IsRedisSet(cortexcache.RedisConfig(config))? That would also be less likely to cause errors in the future as we extend the code. Instead, it would cause compiler errors which we could fix.

[DylanGuedes]

Wow that looks cool. I'll try it, let's see.

[DylanGuedes]

Oh, it didn't work for a similar reason :(
It conflicts because "github.com/cortexproject/cortex/pkg/chunk/cache".RedisConfig) != "github.com/grafana/loki/pkg/storage/chunk/cache".RedisConfig.

edit: In short: even if the structs are exactly the same, you can't just cast them because in essence they are different types (for being in different packages). One solution that I can think of is expecting a interface{} and then casting to cortexcache.Config and if it doesn't work, casting to lokicache.Config, but that looks weird/too much.

edit2: snippet:

func IsRedisSet(genericCfg interface{}) bool {
	if cortexCfg, ok := genericCfg.(cortexcache.Config); ok {
		return cortexCfg.RedisConfig.Address != ""
        }
        if lokiCfg, ok := genericCfg.(lokicache.Config); ok {
		return lokiCfg.RedisConfig.Address != ""
	}
	return false
}

[DylanGuedes]

Still thinking about this one. I don't think we'll want write_dedupe_cache to automatically be enabled: it's unused with boltdb shipper and is only helpful coordinating which chunks have been flushed between multiple nodes (local fifo cache isn't helpful here). I'm leaning towards just enabling this for chunks,index, and query-results caches. Then again, enabling a local write-dedupe-cache won't do anything aside take some cpu cycles and memory, so it may just be simpler to eat the performance hit for config simplicity. WDYT @slim-bean ?

The local cache is set to only be enabled by default for chunk store and for query results (so yeah, no default cache for dedupe_writes). That said, looks like you think index cache should be enabled by default. Do you mind sharing why?

[owen-d]

Upon further thinking, we don't want the index cache to be enabled by default because we don't want the ingester to cache these queries when it's configured to query the store. Let's add a couple tests to ensure we're not setting FIFO caches for the index or write_dedupe caches even when no other cache is set for those, then LGTM!

[DylanGuedes]

Upon further thinking, we don't want the index cache to be enabled by default because we don't want the ingester to cache these queries when it's configured to query the store. Let's add a couple tests to ensure we're not setting FIFO caches for the index or write_dedupe caches even when no other cache is set for those, then LGTM!

WDYT of the tests at lines 716 and 748 from pkg/loki/config_wrapper_test.go? They are covering that enableFifoCache isn't affected for the index and dedupe caches (although I agree that I'm not really checking if the final cache being used is the local/FIFO or not).

[owen-d]

Ah, I didn't catch that case. Nice work :)