cache: Enable embedded cache if no other cache is configured. #10620

ashwanthgoli · 2023-09-18T05:40:16Z

What this PR does / why we need it:

FIFO cache is replaced by embedded-cache in #6821
This PR enables embedded cache for query range results, chunks cache, index_stats_results and volume_results if no other cache is explicitly configured (redis, memcache).

Special notes for your reviewer:
Also removes a deprecated flag cache-split-interval

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated
- If the change is worth mentioning in the release notes, add add-to-release-notes label
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR

dannykopping · 2023-09-18T06:07:50Z

pkg/loki/config_wrapper.go

-func applyFIFOCacheConfig(r *ConfigWrapper) {
+// applyEmbeddedCacheConfig turns on Embedded cache for the chunk store, query range results,
+// index stats and volume results only if no other cache storage is configured (redis or memcache).
+// Not applicable for the index queries cache or for the write dedupe cache.


write dedupe cache

Possibly not for this PR, but I think we can remove this cache; it's been deprecated for ages.

pkg/loki/config_wrapper.go

dannykopping · 2023-09-18T06:12:52Z

pkg/storage/chunk/cache/cache.go

-				PurgeInterval: cfg.EmbeddedCache.PurgeInterval,
-			}
+	if cfg.EmbeddedCache.IsEnabled() {
+		fifocfg := FifoCacheConfig{


Shouldn't this be EmbeddedCacheConfig?

it internally uses fifo cache. I think the idea was to abstract away the implementation details; embedded cache would initialize fifo or group cache based on the distributed flag but the former is now removed.

Not always, it seems:

loki/pkg/loki/modules.go

Lines 587 to 598 in e5c425e

t.Cfg.StorageConfig.IndexQueriesCacheConfig = cache.Config{

EmbeddedCache: cache.EmbeddedCacheConfig{

Enabled: true,

MaxSizeMB: 200,

// This is a small hack to save some CPU cycles.

// We check if the object is still valid after pulling it from cache using the IndexCacheValidity value

// however it has to be deserialized to do so, setting the cache validity to some arbitrary amount less than the

// IndexCacheValidity guarantees the FIFO cache will expire the object first which can be done without

// having to deserialize the object.

TTL: t.Cfg.StorageConfig.IndexCacheValidity - 1*time.Minute,

},

}

Things are getting pretty weird in this space. I think we need to standardise on a single, clear approach.
I don't think we want to keep groupcache at all

but the former is now removed
sorry correction. I meant to say groupcache is now removed.

This is still a bit weird for me. EmbeddedCacheConfig and FifoCacheConfig are almost identical, so we should prefer EmbeddedCacheConfig to reduce complexity. We're nominally removing fifocache in this PR but by still using its configs and internal structs.

Maybe for another PR, but I think we should rename FifoCache to EmbeddedCache and simplify this part of the code.

agree with this. pushed commits to remove/rename fifocache

JStickler

[docs team] Docs LGTM

dannykopping

Approving to unblock, left a comment about still using FifoCache internally which feels weird.

ashwanthgoli · 2023-09-22T12:27:58Z

pkg/querier/queryrange/roundtrip_test.go

-
-			if tc.numCaches == 2 {
-				if tc.equalCaches {
-					require.Equal(t, caches[0], caches[1])


cannot compare them as they are bound to be different instances of embedded cache.
what we want to compare are configs passed down to the cache

results cache enabled, stats cache enabled but different
I'll raise a separate pr to improve this test

ashwanthgoli · 2023-09-22T12:33:48Z

pkg/storage/chunk/cache/embeddedcache.go

 }

 func (cfg *EmbeddedCacheConfig) IsEnabled() bool {
 	return cfg.Enabled
 }
+
+// NewEmbeddedCache returns a new initialised EmbeddedCache.
+func NewEmbeddedCache(name string, cfg EmbeddedCacheConfig, reg prometheus.Registerer, logger log.Logger, cacheType stats.CacheType) *EmbeddedCache {


a few changes to wire things after renaming fifo_cache.go/embeddedcache.go

dannykopping

LGTM, thanks a lot @ashwanthgoli!

dannykopping · 2023-09-22T12:48:38Z

pkg/storage/chunk/cache/embeddedcache.go

+		}),
+
+		entriesAddedNew: promauto.With(reg).NewCounter(prometheus.CounterOpts{
+			Namespace:   "querier",


I wonder if we should take this opportunity to correct these metrics, too.
All our metrics start with loki_ (well... except the ones that start with cortex_), but these don't. It also feels weird to limit the scope of this to the querier since many components can use this cache. Again, not necessarily for this PR 👍

agree, was talking to @chaudum about renaming these. i'll raise a separate pr

docs/sources/configure/query-frontend.md

Co-authored-by: Christian Haudum <christian.haudum@gmail.com>

…a#10620) **What this PR does / why we need it**: FIFO cache is replaced by embedded-cache in grafana#6821 This PR enables embedded cache for query range results, chunks cache, index_stats_results and volume_results if no other cache is explicitly configured (redis, memcache). **Special notes for your reviewer**: Also removes a deprecated flag `cache-split-interval` **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [x] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [x] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e) --------- Co-authored-by: Christian Haudum <christian.haudum@gmail.com>

embedded cache: enabled if not other cache is configured.

6005fdb

pull-request-size bot added the size/L label Sep 18, 2023

add changelog

a274bd5

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Sep 18, 2023

disable embedded cache for migrate tool

76e1dd3

ashwanthgoli marked this pull request as ready for review September 18, 2023 05:59

ashwanthgoli requested a review from a team as a code owner September 18, 2023 05:59

fix Test_ApplyDynamicConfig

81a074f

ashwanthgoli requested review from kavirajk, chaudum and shantanualsi September 18, 2023 06:08

dannykopping reviewed Sep 18, 2023

View reviewed changes

ashwanthgoli added 4 commits September 18, 2023 12:11

fix TestQueryTSDB_WithCachedPostings

339a134

make doc

ddd8866

add a note about the change of results cache size

bdb20e6

fixup! add a note about the change of results cache size

6f91513

JStickler approved these changes Sep 18, 2023

View reviewed changes

dannykopping approved these changes Sep 21, 2023

View reviewed changes

ashwanthgoli added 2 commits September 22, 2023 12:57

replace MaxSizeBytes with MaxSizeMB

80ebaa7

rename fifocache to embedded cache

0f75932

pull-request-size bot added size/XXL and removed size/L labels Sep 22, 2023

ashwanthgoli added 4 commits September 22, 2023 15:01

fixup! rename fifocache to embedded cache

7b29a65

remove fifo cache from frontend configure docs

896bfa4

Merge branch 'main' into ashwanth/apply-embedded-cache-defaults

7bc777f

fix broken TestNewTripperware_Caches. needs improvement (in a follow-up)

38c32ee

ashwanthgoli commented Sep 22, 2023

View reviewed changes

dannykopping approved these changes Sep 22, 2023

View reviewed changes

chaudum approved these changes Sep 22, 2023

View reviewed changes

docs/sources/configure/query-frontend.md Outdated Show resolved Hide resolved

Update docs/sources/configure/query-frontend.md

6941b11

Co-authored-by: Christian Haudum <christian.haudum@gmail.com>

ashwanthgoli merged commit 2902f98 into main Sep 22, 2023
3 checks passed

ashwanthgoli deleted the ashwanth/apply-embedded-cache-defaults branch September 22, 2023 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache: Enable embedded cache if no other cache is configured. #10620

cache: Enable embedded cache if no other cache is configured. #10620

ashwanthgoli commented Sep 18, 2023 •

edited

Loading

dannykopping Sep 18, 2023

dannykopping Sep 18, 2023

ashwanthgoli Sep 18, 2023

dannykopping Sep 18, 2023

ashwanthgoli Sep 18, 2023

ashwanthgoli Sep 18, 2023

dannykopping Sep 21, 2023

ashwanthgoli Sep 22, 2023

JStickler left a comment

dannykopping left a comment

ashwanthgoli Sep 22, 2023

ashwanthgoli Sep 22, 2023 •

edited

Loading

dannykopping left a comment

dannykopping Sep 22, 2023

ashwanthgoli Sep 22, 2023

	t.Cfg.StorageConfig.IndexQueriesCacheConfig = cache.Config{
	EmbeddedCache: cache.EmbeddedCacheConfig{
	Enabled: true,
	MaxSizeMB: 200,
	// This is a small hack to save some CPU cycles.
	// We check if the object is still valid after pulling it from cache using the IndexCacheValidity value
	// however it has to be deserialized to do so, setting the cache validity to some arbitrary amount less than the
	// IndexCacheValidity guarantees the FIFO cache will expire the object first which can be done without
	// having to deserialize the object.
	TTL: t.Cfg.StorageConfig.IndexCacheValidity - 1*time.Minute,
	},
	}

cache: Enable embedded cache if no other cache is configured. #10620

cache: Enable embedded cache if no other cache is configured. #10620

Conversation

ashwanthgoli commented Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JStickler left a comment

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwanthgoli Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwanthgoli commented Sep 18, 2023 •

edited

Loading

ashwanthgoli Sep 22, 2023 •

edited

Loading