Add cache usage statistics #6317

dannykopping · 2022-06-06T13:27:08Z

What this PR does / why we need it:
This PR adds cache statistics for the chunk, index, and result caches. The statistics are collected in the same mechanism that is used currently for ingester/querier stats. The PR also adds some pertinent info to metrics.go which shows cache usage on a per-query, per-tenant basis.

This PR should be read with #6289.

Running logcli query <other args> --stats will now return extra stats:

Cache.Chunk.Requests 		 24
Cache.Chunk.EntriesRequested 	 36
Cache.Chunk.EntriesFound 	 36
Cache.Chunk.EntriesStored 	 0
Cache.Chunk.BytesSent 		 0 B
Cache.Chunk.BytesReceived 	 50 MB
Cache.Index.Requests 		 22
Cache.Index.EntriesRequested 	 22
Cache.Index.EntriesFound 	 22
Cache.Index.EntriesStored 	 0
Cache.Index.BytesSent 		 0 B
Cache.Index.BytesReceived 	 20 kB
Cache.Result.Requests 		 6
Cache.Result.EntriesRequested 	 6
Cache.Result.EntriesFound 	 6
Cache.Result.EntriesStored 	 0
Cache.Result.BytesSent 		 0 B
Cache.Result.BytesReceived 	 23 kB

Which issue(s) this PR fixes:
#6318

Special notes for your reviewer:
Statistics were not being collected correctly in the queryrange.StatsCollectorMiddleware. I believe the statistics were being discarded by the pkg/querier/queryrange/queryrangebase/results_cache.go middleware, but creating a new stats context in the StatsCollectorMiddleware seems to have fixed the issue. This code is pretty hard for me to reason about, so I would appreciate @cyriltovena's review on that since he was the original author, I believe. See baa239c.

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

Signed-off-by: Danny Kopping <[email protected]>

dannykopping · 2022-06-06T13:29:06Z

pkg/logqlmodel/stats/context.go

+	ChunkCache       CacheType = "chunk"
+	IndexCache                 = "index"
+	ResultCache                = "result"
+	WriteDedupeCache           = "write-dedupe"


I've added the write dedupe cache here for the sake of completeness, but it's not yet displayed along with the others.

Documenting function Signed-off-by: Danny Kopping <[email protected]>

grafanabot · 2022-06-06T14:02:58Z

./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.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

DylanGuedes

few nits but looks great

CHANGELOG.md

pkg/logqlmodel/stats/context.go

MasslessParticle

This looks awesome!

Signed-off-by: Danny Kopping <[email protected]>

dannykopping

Thanks for the review! Addressed your comments

CHANGELOG.md

pkg/logqlmodel/stats/context.go

grafanabot · 2022-06-06T14:51:40Z

./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%

DylanGuedes

lgtm!

cyriltovena

LGTM

…tion If we keep the stats collection in pkg/storage/chunk/cache/instrumented.go, then any implementation that wraps it will cause the stats collected to be incomplete. For example: NewBackground(cacheName, cfg.Background, Instrument(cacheName, cache, reg), reg)) - the background cache requests are not collected Signed-off-by: Danny Kopping <[email protected]>

Signed-off-by: Danny Kopping <[email protected]>

grafanabot · 2022-06-07T13:06:37Z

./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.3%
+            querier	0%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0.5%
+               loki	0%

grafanabot · 2022-06-07T13:55:17Z

./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.1%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

* Adding cache statistics Signed-off-by: Danny Kopping <[email protected]> * Adding metrics to metrics.go Signed-off-by: Danny Kopping <[email protected]> * Creating new stats context for use in metric queries middleware Signed-off-by: Danny Kopping <[email protected]> * Clean up unnecessary log fields Signed-off-by: Danny Kopping <[email protected]> * Fixing tests Signed-off-by: Danny Kopping <[email protected]> * Adding stats tests Signed-off-by: Danny Kopping <[email protected]> * CHANGELOG entry Signed-off-by: Danny Kopping <[email protected]> * Appeasing the linter Documenting function Signed-off-by: Danny Kopping <[email protected]> * Moving CHANGELOG entry to appropriate section Signed-off-by: Danny Kopping <[email protected]> * Implementing a stats collector cache wrapper to simplify stats collection If we keep the stats collection in pkg/storage/chunk/cache/instrumented.go, then any implementation that wraps it will cause the stats collected to be incomplete. For example: NewBackground(cacheName, cfg.Background, Instrument(cacheName, cache, reg), reg)) - the background cache requests are not collected Signed-off-by: Danny Kopping <[email protected]> * Fixing tests Signed-off-by: Danny Kopping <[email protected]>

This reverts commit 36e0979.

Danny Kopping added 6 commits June 6, 2022 11:16

Adding cache statistics

b5c496d

Signed-off-by: Danny Kopping <[email protected]>

Adding metrics to metrics.go

438581e

Signed-off-by: Danny Kopping <[email protected]>

Creating new stats context for use in metric queries middleware

baa239c

Signed-off-by: Danny Kopping <[email protected]>

Clean up unnecessary log fields

f56d642

Signed-off-by: Danny Kopping <[email protected]>

Fixing tests

4e9fafb

Signed-off-by: Danny Kopping <[email protected]>

Adding stats tests

648b335

Signed-off-by: Danny Kopping <[email protected]>

pull-request-size bot added the size/XXL label Jun 6, 2022

dannykopping changed the title ~~Add cache statistics~~ Add cache usage statistics Jun 6, 2022

CHANGELOG entry

6cb5665

Signed-off-by: Danny Kopping <[email protected]>

dannykopping commented Jun 6, 2022

View reviewed changes

Appeasing the linter

311e89b

Documenting function Signed-off-by: Danny Kopping <[email protected]>

dannykopping marked this pull request as ready for review June 6, 2022 14:04

dannykopping requested a review from a team as a code owner June 6, 2022 14:04

DylanGuedes approved these changes Jun 6, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/logqlmodel/stats/context.go Show resolved Hide resolved

MasslessParticle approved these changes Jun 6, 2022

View reviewed changes

Moving CHANGELOG entry to appropriate section

9aa789e

Signed-off-by: Danny Kopping <[email protected]>

dannykopping commented Jun 6, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/logqlmodel/stats/context.go Show resolved Hide resolved

dannykopping mentioned this pull request Jun 6, 2022

Add per-tenant cache metrics #6289

Closed

4 tasks

DylanGuedes approved these changes Jun 6, 2022

View reviewed changes

cyriltovena approved these changes Jun 7, 2022

View reviewed changes

Danny Kopping added 2 commits June 7, 2022 12:48

Fixing tests

b966286

Signed-off-by: Danny Kopping <[email protected]>

dannykopping merged commit 36e0979 into grafana:main Jun 7, 2022

dannykopping deleted the dannykopping/cache-stats branch June 7, 2022 14:01

dannykopping mentioned this pull request Jun 7, 2022

Recut k101 with cache statistics #6328

Merged

dannykopping pushed a commit to dannykopping/loki that referenced this pull request Jun 28, 2022

Revert "Add cache usage statistics (grafana#6317)"

0ef457b

This reverts commit 36e0979.

This was referenced Jun 28, 2022

dannykopping/remove cache stats dannykopping/loki#13

Closed

dannykopping/remove cache stats dannykopping/loki#14

Draft

Remove cache statistics PR from release-2.6.x #6525

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cache usage statistics #6317

Add cache usage statistics #6317

dannykopping commented Jun 6, 2022 •

edited

Loading

dannykopping Jun 6, 2022 •

edited

Loading

grafanabot commented Jun 6, 2022

DylanGuedes left a comment

MasslessParticle left a comment

dannykopping left a comment

grafanabot commented Jun 6, 2022

DylanGuedes left a comment

cyriltovena left a comment

grafanabot commented Jun 7, 2022

grafanabot commented Jun 7, 2022

Add cache usage statistics #6317

Add cache usage statistics #6317

Conversation

dannykopping commented Jun 6, 2022 • edited Loading

dannykopping Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

grafanabot commented Jun 6, 2022

DylanGuedes left a comment

Choose a reason for hiding this comment

MasslessParticle left a comment

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

grafanabot commented Jun 6, 2022

DylanGuedes left a comment

Choose a reason for hiding this comment

cyriltovena left a comment

Choose a reason for hiding this comment

grafanabot commented Jun 7, 2022

grafanabot commented Jun 7, 2022

dannykopping commented Jun 6, 2022 •

edited

Loading

dannykopping Jun 6, 2022 •

edited

Loading