cache: switch to Rueidis client for Redis #5593

GiedriusS · 2022-08-12T13:37:33Z

Add support for the Rueidis client. It has been instrumental for us in
avoiding an incident in production from too much network load on the
servers. Since we have lots of alerting/recording rules that use
historical data i.e. from Thanos Store, it means that without
client-side caching the same data is retrieved repeatedly.
Rueidis sends a PTTL command to Redis after retrieving data from Redis
to know what the TTL is.

I contemplated sending a design doc for this but the design doc ended up
very slim. It's either something like this or:

Adding client-side caching support to go-redis (it's probably a huge
undertaking);
Using the type field to differentiate clients.

I think it's ready to go except for two things:

There's no way to disable local cache in Rueidis for now due to
limitation in the Rueidis library;
There's no way to disable the default TTL in case PTTL fails - we
probably don't want to cache sensitive things like index data for longer
then we're supposed to.

I'll try to work on these things if this whole idea of having different
clients look good to you.

Signed-off-by: Giedrius Statkevičius giedrius.statkevicius@vinted.com

bwplotka

Thanks a lot! One sentence to fix in docs, otherwise LGTM!

bwplotka · 2022-08-17T14:51:22Z

docs/components/store.md

+* `GOREDIS` (the default one)
+* `RUEIDIS`
+
+The main difference is that [Rueidis](https://github.com/rueian/rueidis) supports [client-side caching](https://redis.io/docs/manual/client-side-caching/) which is really important. If you have high cardinality metrics then it means that it is no longer necessary to go to remote-object storage to get information about potentially thousands or millions of metrics.


If you have high cardinality metrics then it means that it is no longer necessary to go to remote-object storage to get information about potentially thousands or millions of metrics.

I think this is wrong. The client caching is for Redis client NOT object storage. Also how it helps with cardinality in particular?

Client side caching to me sounds like extra in-mem cache that is checked first before accessing shared cache in REDIS, right?

Yeah I am also a little bit confused. What's the benefits of client side caching over server side caching. Less network calls?

matej-g · 2022-08-26T13:33:32Z

What's the status on this? Will we proceed with single client as we discussed?

GiedriusS · 2022-08-30T07:44:22Z

I talked with @hanjm in private and they depend on RESP2 support in the Redis client so unfortunately we cannot easily from go-redis to Rueidis. So, I propose moving forward with this custom client support if that sounds good. Our discussion still stands though - we shouldn't add support for custom clients without good rationale. Let me update the PR and fix the docs.

rueian · 2022-10-13T15:54:16Z

I talked with @hanjm in private and they depend on RESP2 support in the Redis client so unfortunately we cannot easily from go-redis to Rueidis. So, I propose moving forward with this custom client support if that sounds good. Our discussion still stands though - we shouldn't add support for custom clients without good rationale. Let me update the PR and fix the docs.

Hi @GiedriusS, just let you know that rueidis now also works in RESP2 with client-side cache disabled.

hanjm · 2022-10-14T12:02:48Z

Hi @GiedriusS, just let you know that rueidis now also works in RESP2 with client-side cache disabled.

Cool.

matej-g · 2022-10-17T08:41:48Z

@GiedriusS @hanjm does this mean we can go forward with the original suggestion, i.e. replace clients?

GiedriusS · 2022-10-17T08:44:59Z

I think so. I'll update this PR soon.

GiedriusS · 2022-10-18T11:06:41Z

Testing out the newest Rueidis client before updating this PR (:

bwplotka · 2022-12-07T14:11:40Z

We are looking forward to this change, sounds like deadlock is over (:

GiedriusS · 2022-12-08T12:20:42Z

Replaced go-redis with rueidis and simplified everything since rueidis now supports RESP2 as well. Most notably, this includes support for Redis Enterprise. Also, support for disabling client-side caching has been added so I've adjusted the code for that too. I've been testing this in prod for 3 months+ if not more so I think this should be good to go 💪 huge thank you goes to @rueian for all of the development and support, and a great library ❤️

Switch client to rueidis because it is faster and supports client-side caching. Tested it in production for a few months. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

douglascamata · 2022-12-09T11:01:20Z

pkg/cacheutil/redis_client.go

+			return errors.New("both client key and certificate must be provided")
+		}
+	}
+
 	return nil
 }

 // RedisClient is a wrap of redis.Client.


This comment is now outdated.

douglascamata · 2022-12-09T11:02:26Z

pkg/cacheutil/redis_client.go

-	redisClient := redis.NewClient(opts)
-	if reg != nil {
-		reg = prometheus.WrapRegistererWith(prometheus.Labels{"name": name}, reg)
+	var disableCaching = false


Why not disableCaching := false? I thought we used var only to declare variables and use their type's zero-value.

Also, this is client-side caching, right? Could the variable be named clientSideCacheEnabled then? It is more concise with what it really is and avoids the "negative thinking" (which causes double negation when disableCaching = false.

douglascamata · 2022-12-09T11:34:21Z

pkg/cacheutil/redis_client.go

-	if _, err := c.Set(ctx, key, value, ttl).Result(); err != nil {
-		level.Warn(c.logger).Log("msg", "failed to set item into redis", "err", err, "key", key,
-			"value_size", len(value))
+	if err := c.client.Do(ctx, c.client.B().Set().Key(key).Value(rueidis.BinaryString(value)).ExSeconds(int64(ttl.Seconds())).Build()).Error(); err != nil {


Why here we use Set with ExSeconds being added later instead of Setex, like in the SetMulti function? Would be cool to be consistent.

Also, wouldn't it be wise to use DoCache here to populate the client side cache too? I don't see DoCache or DoMultiCache being used anymore and from what I understand from Rueidis' README those are functions that will also populate client side cache.

I noticed there is MGetCache for getting through client side cache, but I fail to see where we write to the client side cache.

Hi @douglascamata,

The population of client side cache is done by rueidis when you get successful responses from DoCache and DoMultiCache with supported commands, such as GET and MGET.

SET is not supported by DoCache by design, because the client must be tracked by redis server via cacheable requests to receive related invalidation notifications later.

@rueian gotcha, thanks for the clarification!

I thought it would be useful for us to populate the client side cache whenever we SET something, but it if the cost is another GET and by consequence of the design, might not be a good idea.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

GiedriusS · 2022-12-14T13:07:00Z

Thanks for the review @douglascamata, I've updated the PR.

yeya24

Overall this looks good. Looking forward to merge it!

pkg/cacheutil/redis_client.go

yeya24 · 2022-12-19T18:12:50Z

Btw let's fix the changelog.

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka

Amazing work! Worth taking it to 0.30 release as well.

* cacheutil: replace go-redis with rueidis client Switch client to rueidis because it is faster and supports client-side caching. Tested it in production for a few months. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> * cacheutil: fix according to review Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>

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

GiedriusS force-pushed the add_rueidis_client_support branch from 5c6f283 to dd2529f Compare August 12, 2022 13:47

bwplotka previously approved these changes Aug 17, 2022

View reviewed changes

GiedriusS mentioned this pull request Aug 17, 2022

Add ability to disable local caching redis/rueidis#79

Closed

GiedriusS mentioned this pull request Aug 29, 2022

query: add --grpc-compression for enabling compression #5654

Merged

GiedriusS mentioned this pull request Oct 24, 2022

Deadlock spotted in v0.0.80 redis/rueidis#108

Closed

GiedriusS dismissed bwplotka’s stale review via 1ee7262 December 8, 2022 11:34

GiedriusS force-pushed the add_rueidis_client_support branch 4 times, most recently from 8c84c55 to 12699e4 Compare December 8, 2022 12:04

GiedriusS force-pushed the add_rueidis_client_support branch from 12699e4 to afeecad Compare December 8, 2022 12:59

cacheutil: replace go-redis with rueidis client

7ee0670

Switch client to rueidis because it is faster and supports client-side caching. Tested it in production for a few months. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

GiedriusS force-pushed the add_rueidis_client_support branch from afeecad to 7ee0670 Compare December 8, 2022 15:42

douglascamata reviewed Dec 9, 2022

View reviewed changes

cacheutil: fix according to review

1c3d163

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

GiedriusS changed the title ~~cache: add Rueidis client support~~ cache: switch to Rueidis client for Redis Dec 15, 2022

yeya24 previously approved these changes Dec 19, 2022

View reviewed changes

pkg/cacheutil/redis_client.go Show resolved Hide resolved

yeya24 mentioned this pull request Dec 19, 2022

Support redis caching bucket cortexproject/cortex#5049

Closed

Merge branch 'main' into add_rueidis_client_support

b756c41

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka dismissed yeya24’s stale review via b756c41 December 20, 2022 13:39

bwplotka approved these changes Dec 20, 2022

View reviewed changes

bwplotka enabled auto-merge (squash) December 20, 2022 13:41

bwplotka merged commit e85bc1f into thanos-io:main Dec 20, 2022

GiedriusS deleted the add_rueidis_client_support branch December 20, 2022 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache: switch to Rueidis client for Redis #5593

cache: switch to Rueidis client for Redis #5593

GiedriusS commented Aug 12, 2022 •

edited

bwplotka left a comment

bwplotka Aug 17, 2022

yeya24 Sep 27, 2022

matej-g commented Aug 26, 2022

GiedriusS commented Aug 30, 2022

rueian commented Oct 13, 2022

hanjm commented Oct 14, 2022

matej-g commented Oct 17, 2022

GiedriusS commented Oct 17, 2022

GiedriusS commented Oct 18, 2022

bwplotka commented Dec 7, 2022

GiedriusS commented Dec 8, 2022

douglascamata Dec 9, 2022

douglascamata Dec 9, 2022

douglascamata Dec 9, 2022

douglascamata Dec 9, 2022

douglascamata Dec 9, 2022

douglascamata Dec 9, 2022

rueian Dec 9, 2022 •

edited

douglascamata Dec 9, 2022 •

edited

GiedriusS commented Dec 14, 2022

yeya24 left a comment

yeya24 commented Dec 19, 2022

bwplotka left a comment

cache: switch to Rueidis client for Redis #5593

cache: switch to Rueidis client for Redis #5593

Conversation

GiedriusS commented Aug 12, 2022 • edited

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matej-g commented Aug 26, 2022

GiedriusS commented Aug 30, 2022

rueian commented Oct 13, 2022

hanjm commented Oct 14, 2022

matej-g commented Oct 17, 2022

GiedriusS commented Oct 17, 2022

GiedriusS commented Oct 18, 2022

bwplotka commented Dec 7, 2022

GiedriusS commented Dec 8, 2022

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

rueian Dec 9, 2022 • edited

Choose a reason for hiding this comment

douglascamata Dec 9, 2022 • edited

Choose a reason for hiding this comment

GiedriusS commented Dec 14, 2022

yeya24 left a comment

Choose a reason for hiding this comment

yeya24 commented Dec 19, 2022

bwplotka left a comment

Choose a reason for hiding this comment

GiedriusS commented Aug 12, 2022 •

edited

rueian Dec 9, 2022 •

edited

douglascamata Dec 9, 2022 •

edited