embedded-cache: Bring `fifocache` and `groupcache` into single tent. #6821

kavirajk · 2022-08-01T14:58:05Z

Signed-off-by: Kaviraj kavirajkanagaraj@gmail.com

What this PR does / why we need it:
This PR introduces have following changes

new cache type called "embedded-cache"
Now we bring both fifocache and groupcache into single tent called embedded-cache.
old fifocache is depreicated. New config is embedded-cache.enabled=true && embedded-cache.distributed=false
replaces groupcache references to embedded cache accordingly.
distributed embedded cache is enabled only for results cache.

Usage: Embedded Cache.

embedded_cache:
    enabled: true
    distributed: true|false

Usage: Distributed Embedded cache (supported only for results cache for now)

query_range:
  results_cache:
    cache:
      embedded_cache:
        enabled: true
        distributed: true`

or via flag

-frontend.embedded-cache.distributed -frontend.embedded-cache.enabled

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
This is part of config cleanup before shipping groupcache feature for results cache.

Checklist

Documentation added
Tests updated
Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

1. Introduced new config called memorycache config. 2. It has flag `distributed`. By enabling it, we use groupcache Going forward. We will bring fifocache into memcache config(with distributed:false) This PR is just a POC. To show how we can simplify in-built memory cache config and bring it into single config. Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

MasslessParticle

~~I'm not clear: what happens when we set distributed = false?~~

Nevemind read the description

pkg/storage/chunk/cache/cache.go

pkg/storage/chunk/cache/groupcache.go

dannykopping

Great start! I was expecting to see some code which applies fifocache when memorycache is set with distributed: false. Did I miss it?

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

grafanabot · 2022-08-03T11:23:02Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.1%

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

dannykopping

Great work Kavi! Minor nits, mostly

CHANGELOG.md

pkg/loki/config_wrapper.go

pkg/loki/modules.go

pkg/storage/chunk/cache/cache.go

MasslessParticle

Overall this looks awesome. I think we need to think through how the ring config works and think about enabling this for the chunks cache.

pkg/storage/chunk/cache/cache.go

pkg/storage/chunk/cache/embeddedcache.go

pkg/storage/chunk/cache/groupcache.go

pkg/loki/modules.go

pkg/loki/config_wrapper.go

pkg/loki/modules.go

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

1. update changelog and upgrade guide 2. s/Embeddedcache/EmbeddedCache/g 3. /groupcache/ handler -> /embedded-cache/ 4. some typos 5. Fix some cache prefixes Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

kavirajk · 2022-08-04T10:08:16Z

NOTE: @slim-bean suggested an idea of instead of have boolean flag distributed, we could have type: local|distributed that has the more leverage to add different types of caches in the future.

I like the idea. I will take that up in the follow up PR.

grafanabot · 2022-08-04T10:11:29Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.3%

dannykopping

Looks great Kavi! We should test it out a bit and wait for Travis to approve before merging, but this looks promising. Left a couple very minor wording nits.

dannykopping · 2022-08-04T10:12:30Z

docs/sources/upgrading/_index.md

+We introduce new cache called `embedded-cache` which is an in-process cache system that make it possible to run loki without the need for an external cache (like memcached, redis, etc). It can be run in two modes `distributed: false` (default, and same as old `fifocache`) and `distributed: true` which runs cache in distributed fashion sharding keys across peers if Loki is run in microservices or SSD mode.
+
+Currently `embedde-cache` with `distributed: true` can be enabled only for results cache.


Suggested change

We introduce new cache called `embedded-cache` which is an in-process cache system that make it possible to run loki without the need for an external cache (like memcached, redis, etc). It can be run in two modes `distributed: false` (default, and same as old `fifocache`) and `distributed: true` which runs cache in distributed fashion sharding keys across peers if Loki is run in microservices or SSD mode.

Currently `embedde-cache` with `distributed: true` can be enabled only for results cache.

We introduced a new cache called `embedded-cache` which is an in-process cache system that make it possible to run loki without the need for an external cache (like memcached, redis, etc). It can be run in two modes `distributed: false` (default, and same as old `fifocache`) and `distributed: true` which runs cache in distributed fashion sharding keys across peers if Loki is run in microservices or SSD mode.

Currently `embedded-cache` with `distributed: true` can be enabled only for results cache.

dannykopping · 2022-08-04T10:14:01Z

pkg/storage/chunk/cache/embeddedcache.go

 	Distributed bool          `yaml:"distributed,omitempty"`
 	Enabled     bool          `yaml:"enabled,omitempty"`
 	MaxSizeMB   int64         `yaml:"max_size_mb"`
-	MaxItems    int           `yaml:"max_items"`
+	MaxItems    int           `yaml:"max_items"` // TODO(kavi): should we stop supporting it?


dannykopping · 2022-08-04T10:14:41Z

pkg/storage/chunk/cache/embeddedcache.go

+}
+
+func (cfg *EmbeddedCacheSingletonConfig) RegisterFlagsWithPrefix(prefix, description string, f *flag.FlagSet) {
+	f.IntVar(&cfg.ListenPort, prefix+"embedded-cache.listen_port", 4100, "The port to use for groupcache communication")


Let's avoid mentioning the underlying technology

…buted cache Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

grafanabot · 2022-08-04T11:02:33Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.9%

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

grafanabot · 2022-08-04T14:49:53Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.3%

docs/sources/upgrading/_index.md

KMiller-Grafana · 2022-08-04T18:17:08Z

Should the new configuration parameters be added to this doc section: https://grafana.com/docs/loki/latest/configuration/#cache_config, and should the fifocache configuration parameters be labeled as deprecated? If yes, an example of how the docs identify deprecated configuration parameters is split_queries_by_day.

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

grafanabot · 2022-08-08T11:59:59Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.3%

kavirajk · 2022-08-08T12:02:44Z

Should the new configuration parameters be added to this doc section: https://grafana.com/docs/loki/latest/configuration/#cache_config, and should the fifocache configuration parameters be labeled as deprecated? If yes, an example of how the docs identify deprecated configuration parameters is split_queries_by_day.

Thanks for pointer @KMiller-Grafana. Will definitely do it in the follow up PR. When I fix the distributed bool flag as mentioned in this comment.
#6821 (comment)

09jvilla · 2022-08-08T14:45:19Z

Addresses
#6785

KMiller-Grafana

Thank you for the changes.

…rafana#6821) * chore(groupcache): Rename groupcache into memcache{distributed: true} 1. Introduced new config called memorycache config. 2. It has flag `distributed`. By enabling it, we use groupcache Going forward. We will bring fifocache into memcache config(with distributed:false) This PR is just a POC. To show how we can simplify in-built memory cache config and bring it into single config. Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Memorycache -> EmbeddedCache * Bring fifocache into embeddedcache tent Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Fix edge case with enabling fifocache Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Fix missing flags for embedded cache Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Add changelog Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Warning message for fifocache config Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Make linter happy Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Fix listenport issue Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * idk. review remarks Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * PR remarks 1. update changelog and upgrade guide 2. s/Embeddedcache/EmbeddedCache/g 3. /groupcache/ handler -> /embedded-cache/ 4. some typos 5. Fix some cache prefixes Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Fixing corresponding tests Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Support extra timeouts on embedded-cache Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Remove unwanted single struct field Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * fix(stats-collector): Let cache.New() wrap stats collector for distributed cache Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * PR remarks Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Deprecate max_size_items from fifocache Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * PR remarks about the flag description Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Update docs/sources/upgrading/_index.md Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

**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** - [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](d10549e) --------- 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>

pull-request-size bot added the size/L label Aug 1, 2022

kavirajk changed the title ~~chore(groupcache): Rename groupcache into memcache{distributed: true}~~ chore(groupcache): Rename groupcache into memorycache{distributed: true} Aug 1, 2022

MasslessParticle reviewed Aug 1, 2022

View reviewed changes

pkg/storage/chunk/cache/cache.go Outdated Show resolved Hide resolved

pkg/storage/chunk/cache/groupcache.go Outdated Show resolved Hide resolved

dannykopping reviewed Aug 1, 2022

View reviewed changes

kavirajk added 7 commits August 3, 2022 11:12

Memorycache -> EmbeddedCache

2e2f3d4

Merge branch 'main' into kaviraj/memorycache-unification-config

f587e7a

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Bring fifocache into embeddedcache tent

0c0ae42

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Fix edge case with enabling fifocache

570b370

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Fix missing flags for embedded cache

d1b93cc

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Add changelog

4035664

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Warning message for fifocache config

7e6bed9

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

kavirajk changed the title ~~chore(groupcache): Rename groupcache into memorycache{distributed: true}~~ embedded-cache: Bring fifocache and groupcache into single tent. Aug 3, 2022

kavirajk marked this pull request as ready for review August 3, 2022 10:50

kavirajk requested a review from a team as a code owner August 3, 2022 10:50

Make linter happy

d4976b3

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Fix listenport issue

7c476ba

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

kavirajk requested review from dannykopping and MasslessParticle August 3, 2022 16:22

dannykopping reviewed Aug 3, 2022

View reviewed changes

MasslessParticle requested changes Aug 3, 2022

View reviewed changes

kavirajk added 3 commits August 4, 2022 11:13

idk. review remarks

993b870

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

PR remarks

0c16ab8

1. update changelog and upgrade guide 2. s/Embeddedcache/EmbeddedCache/g 3. /groupcache/ handler -> /embedded-cache/ 4. some typos 5. Fix some cache prefixes Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Fixing corresponding tests

314e938

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

kavirajk requested a review from KMiller-Grafana as a code owner August 4, 2022 09:37

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

kavirajk added 3 commits August 4, 2022 11:44

Merge branch 'main' into kaviraj/memorycache-unification-config

86f0b7e

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Support extra timeouts on embedded-cache

f6ba193

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Remove unwanted single struct field

fa076e4

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

kavirajk requested review from dannykopping and MasslessParticle and removed request for MasslessParticle August 4, 2022 10:05

dannykopping approved these changes Aug 4, 2022

View reviewed changes

kavirajk added 3 commits August 4, 2022 12:49

fix(stats-collector): Let cache.New() wrap stats collector for distri…

5c087cc

…buted cache Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

PR remarks

b71e47e

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Deprecate max_size_items from fifocache

1d9c80f

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

PR remarks about the flag description

eeb8d77

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

KMiller-Grafana reviewed Aug 4, 2022

View reviewed changes

docs/sources/upgrading/_index.md Outdated Show resolved Hide resolved

Update docs/sources/upgrading/_index.md

800fbdf

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

MasslessParticle approved these changes Aug 8, 2022

View reviewed changes

09jvilla mentioned this pull request Aug 8, 2022

Remove references to groupcache in config & metrics #6785

Closed

KMiller-Grafana approved these changes Aug 8, 2022

View reviewed changes

kavirajk merged commit 7e3d243 into main Aug 9, 2022

kavirajk deleted the kaviraj/memorycache-unification-config branch August 9, 2022 10:49

ashwanthgoli mentioned this pull request Sep 18, 2023

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

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

embedded-cache: Bring `fifocache` and `groupcache` into single tent. #6821

embedded-cache: Bring `fifocache` and `groupcache` into single tent. #6821

kavirajk commented Aug 1, 2022 •

edited

Loading

MasslessParticle left a comment •

edited

Loading

dannykopping left a comment

grafanabot commented Aug 3, 2022

dannykopping left a comment

MasslessParticle left a comment

kavirajk commented Aug 4, 2022

grafanabot commented Aug 4, 2022

dannykopping left a comment

dannykopping Aug 4, 2022

dannykopping Aug 4, 2022

dannykopping Aug 4, 2022

grafanabot commented Aug 4, 2022

grafanabot commented Aug 4, 2022

KMiller-Grafana commented Aug 4, 2022

grafanabot commented Aug 8, 2022

kavirajk commented Aug 8, 2022

09jvilla commented Aug 8, 2022

KMiller-Grafana left a comment

		We introduce new cache called `embedded-cache` which is an in-process cache system that make it possible to run loki without the need for an external cache (like memcached, redis, etc). It can be run in two modes `distributed: false` (default, and same as old `fifocache`) and `distributed: true` which runs cache in distributed fashion sharding keys across peers if Loki is run in microservices or SSD mode.

		Currently `embedde-cache` with `distributed: true` can be enabled only for results cache.

embedded-cache: Bring fifocache and groupcache into single tent. #6821

embedded-cache: Bring fifocache and groupcache into single tent. #6821

Conversation

kavirajk commented Aug 1, 2022 • edited Loading

MasslessParticle left a comment • edited Loading

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

grafanabot commented Aug 3, 2022

dannykopping left a comment

Choose a reason for hiding this comment

MasslessParticle left a comment

Choose a reason for hiding this comment

kavirajk commented Aug 4, 2022

grafanabot commented Aug 4, 2022

dannykopping left a comment

Choose a reason for hiding this comment

dannykopping Aug 4, 2022

Choose a reason for hiding this comment

dannykopping Aug 4, 2022

Choose a reason for hiding this comment

dannykopping Aug 4, 2022

Choose a reason for hiding this comment

grafanabot commented Aug 4, 2022

grafanabot commented Aug 4, 2022

KMiller-Grafana commented Aug 4, 2022

grafanabot commented Aug 8, 2022

kavirajk commented Aug 8, 2022

09jvilla commented Aug 8, 2022

KMiller-Grafana left a comment

Choose a reason for hiding this comment

embedded-cache: Bring `fifocache` and `groupcache` into single tent. #6821

embedded-cache: Bring `fifocache` and `groupcache` into single tent. #6821

kavirajk commented Aug 1, 2022 •

edited

Loading

MasslessParticle left a comment •

edited

Loading