Chunks caching at bucket level

[pstibrany]

This PR implements chunks caching as described in "Caching in Thanos" document.

Chunks caching is done at Bucket level. Since bucket doesn't really know about chunks, and Thanos simply uses GetRange call on Bucket to fetch chunks, this implementation splits such requests into smaller "blocks" "subranges" (with size of 16000 bytes), and stores each block subrange into cache individually.

To enable chunks caching, one needs to configure store to use caching bucket using --experimental.caching-bucket.config=store_cache.yaml option. File looks like this:

backend: memcached
backend_config:
  addresses:
    - localhost:11211

caching_config:
  chunk_subrange_size: 16000
  max_chunks_get_range_requests: 3

Only memcached client is supported at the moment. If --caching-bucket.config-file is not passed to the store (default), no chunks caching is done. (Index cache is unaffected by this)

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Verification

Tested manually (running queries against store with caching bucket) and using unit tests.

[pracucci]

Thanks @pstibrany for the PR! I left few preliminary comments, but I think the caching implementation is a it complex and I would like to have an offline discussion about it before moving forward. I would like to explore options to simplify.

[bwplotka]

You are way too fast with this =D

We can't catch up @pstibrany ;p Just joined Google doc party with late comments: https://docs.google.com/document/d/1knNXQfDJlQDA9Dr9R9OVFvdWS83sXd0kLEFdv21IxCM/edit# Sorry for lag!

I know @brancz wanted to add some input as point as well. (:

Overall commented the TL;DR of my comments but will comment here as well:

---

Amazing, thanks for this proposal!

This is really good, my only suggestion is that it looks like we can really create a generic client-side cache layer for object storage (or maybe server-side at some point).

I think for a start we can create a generic cache layer for Bucket interface backed up by memcached. Maybe we can find a caching logic that will work for both metadata, postings/series, and chunk caching? The idea is to simplify this layer which will also allow extensibility and easy maintenance.

Then we can iterate to move it different backends OR maybe totally move as localized server-side cache for the whole cluster... ((:

[pstibrany]

I think this is ready for another review. (as soon as I fix lint and test issues 😄)

[pstibrany]

docs-tests failed with

docs/community.md
 
	ERROR	https://zoom.us/j/96476403386
 
		Error when reading response headers: small read buffer. Increase ReadBufferSize. Buffer size=4096, contents: "HTTP/1.1 200 \r\nDate: Thu, 30 Apr 2020 08:52:17 GMT\r\nContent-Type: text/html;charset=utf-8\r\nTransfer-Encoding: chunked\r\nConnection: keep-alive\r\nServer: ZOOM\r\nX-Zm-Trackingid: WEB_b1b0451a7c7a6d0f1339b4"..."=44766e66c4f14818a82eb55b073f8388; Domain=.zoom.us; Path=/; Secure\r\nContent-Language: en-US\r\nStrict-Transport-Security: max-age=31536000\r\nX-Xss-Protection: 1; mode=block\r\nReferrer-Policy: strict-origi"

which seems unrelated to my changes.

[bwplotka]

Please rebase, I think it's fixed on master.

…

On Thu, 30 Apr 2020 at 09:58, Peter Štibraný ***@***.***> wrote: docs-tests failed with docs/community.md ERROR https://zoom.us/j/96476403386 Error when reading response headers: small read buffer. Increase ReadBufferSize. Buffer size=4096, contents: "HTTP/1.1 200 \r\nDate: Thu, 30 Apr 2020 08:52:17 GMT\r\nContent-Type: text/html;charset=utf-8\r\nTransfer-Encoding: chunked\r\nConnection: keep-alive\r\nServer: ZOOM\r\nX-Zm-Trackingid: WEB_b1b0451a7c7a6d0f1339b4"..."=44766e66c4f14818a82eb55b073f8388; Domain=.zoom.us; Path=/; Secure\r\nContent-Language: en-US\r\nStrict-Transport-Security: max-age=31536000\r\nX-Xss-Protection: 1; mode=block\r\nReferrer-Policy: strict-origi" which seems unrelated to my changes. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2532 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3O2PDLPYDBGR2RCHJ2LRPE4TPANCNFSM4MS3BJJA> .

[pracucci]

Amazing job @pstibrany! The implementation looks very good to me. I left many very small comments, but overall LGTM 👏 👏 👏

[pracucci]

Amazing job @pstibrany. LGTM (modulo few last nits).

Could you also add a CHANGELOG entry, please?

[pstibrany]

We have some numbers to share from testing this PR (as of commit 9821c29).

We were testing with following query from our dashboard with 30 days time range:
sum by (status) (label_replace(label_replace(rate(cortex_request_duration_seconds_count{cluster=~"dev-us-central1/agent|dev-us-central1", job=~"(loki-dev|loki-boltdb-shipper|default|cortex-tsdb-dev|cortex-enterprise|cortex-chunks)/query-frontend"}[2400s]), "status", "${1}xx", "status_code", "([0-9]).."), "status", "${1}", "status_code", "([a-z]+)"))

This query needs to fetch about 20 MiB of chunks data.

We used separate cache for index-cache and chunks-cache. Only chunks cache was emptied between tests, not index-cache. We also used query-frontend in our setup, which split the query into smaller time ranges that were executed in parallel, but with caching of results disabled.

Times were measured with time command, timing curl.

empty cache	with full cache
1.934s	0.822s
1.732s	0.827s
1.818s	0.819s

Here are two screenshots, one showing trace with empty cache, with many 'multicache gets', followed by 'set' for fetched blocks (cortex-store-gateway is doing job of store component in Thanos)

Trace with cached data:

[bwplotka]

Super hectic day, sorry for lag and that's not the end, I am getting disrupted all the time, so only 25% of this PR reviewed.

Amazing work so far! Will be back after meetings.... 😱

[bwplotka]

I know we were doing opposite but now when I think of it I think it would be nice to have this is hidden but normal flag name.

It's painful to change flag names for users ... (:

so maybe just

Suggested change

	cachingBucketConfig := extflag.RegisterPathOrContent(cmd, "experimental.caching-bucket.config",
	cachingBucketConfig := extflag.RegisterPathOrContent(cmd, "store.caching-bucket.config",

[pstibrany]

How can we hide it?

[bwplotka]

I guess by adding RegisterHiddenPathOrContent or something (:

[pstibrany]

Oh, are you suggesting to add RegisterHiddenPathOrContent that will "hide" the command line option – how? By giving it "hidden" prefix? I'm little confused about what you're suggesting here.

[pstibrany]

Hidden by using new extflag.HiddenCmdClause(cmd) like this:

	cachingBucketConfig := extflag.RegisterPathOrContent(extflag.HiddenCmdClause(cmd), "store.caching-bucket.config", ...)

WDYT?

[bwplotka]

Amazing, cool 👍

LGTM, just I would love if we could change block into segment to not create confusion with TSDB blocks.

Does it work well with partitioning? Looks like it partition by chunk size and partitioning is not aware of this 🤔

Wonder if we can improve anything there.

[pstibrany]

LGTM, just I would love if we could change block into segment to not create confusion with TSDB blocks.

Problem with “segment” is that it already has a meaning in TSDB, and I don’t want to reuse it.

I’ve found some synonyms to “chunk” that may work: hunk, lump, piece, portion, ... I personally like hunk, because it’s subword of “chunk” :) WDYT?

Does it work well with partitioning? Looks like it partition by chunk size and partitioning is not aware of this 🤔

Partitioner merges multiple small requests into bigger one. That helps to save on number of requests, which is good. Chunking cache then “rounds” starting/ending offsets to hunk (!) size, and caches them individually.

I think it works together just fine, I’m not sure what could be improved. We certainly don’t want to skip gap-sized partitioner. From caching perspective, it may be useful to reduce gap check (it defaults to 512 KiB if I remember correctly), but that would possibly increase number of requests, which is not what we want. Ideas are welcome.

[pstibrany]

@bwplotka Thanks for your review! I will address your feedback tomorrow.

[pracucci]

I’ve found some synonyms to “chunk” that may work: hunk, lump, piece, portion, ... I personally like hunk, because it’s subword of “chunk” :) WDYT?

@pstibrany @bwplotka I'm personally dubious about "hunk" because of few reasons:

1. Finger memory risks to type "chunk"
2. While reading code, it's easy to misread "hunk" as "chunk"
3. "hunk" looks like a typo of "chunk"

I already had this discussion with Peter and at the beginning I was also suggesting "segment", but I also agree that's not much better then "block" given both names already mean something different in TSDB. Have you considered subrange? What we're caching is practically an offset-aligned subrange, it looks clear and doesn't clash with any other naming we already have.

[pstibrany]

I believe I've now addressed all feedback, and this PR is hopefully ready for final (?) review and merge!

[bwplotka]

Amazing work 🎖️

LGTM! Thanks ❤️

[pstibrany]

Thanks a lot for your review efforts! You can look forward to more caching PRs real soon :)

[bwplotka]

Yes, especially around meta fetching (:

…

On Thu, 7 May 2020 at 18:18, Peter Štibraný ***@***.***> wrote: Thanks a lot for your review efforts! You can look forward to more caching PRs real soon :) — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2532 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3OYTNMMQ6QZW7XN543TRQLUORANCNFSM4MS3BJJA> .