ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter #2849

Dav1dde · 2023-12-13T14:37:53Z

Changes the cardinality limiter implementation to use a Lua script. The Lua script evaluates each individual hash and returns a decision wether this hash is accepted or rejected by the cardinality limiter, this allows us to save one additional round trip to Redis.

Additionally there is an in memory cache infront of Redis remembering previous decisions from the Redis script, allowing us to skip Redis calls alltogether.

Epic: #2717

relay-cardinality/src/statsd.rs

relay-cardinality/src/redis.rs

jjbayer

Looks good, see comment on batch calls. I assume that getting reducing the number of redis commands is still beneficial, even within a Lua script.

relay-cardinality/src/cardinality.lua

relay-cardinality/src/redis.rs

relay-cardinality/src/statsd.rs

jjbayer

Still some open comments, but no blockers as far as I am concerned!

jjbayer · 2023-12-18T12:06:25Z

relay-cardinality/src/cardinality.lua

+    for i = 1, #KEYS do
+        local is_working_set = i == 1
+
+        for from, to in batches(#t, 7000, offset, max) do


I think we should batch exclusively on the Relay side. That would simplify this script, and we need some sort of batching there anyway.

I like batching in the lua script, makes the relay side easier to read and we have a guarantee that this never fails in lua (one less error to handle and invariant).

The lua implementation would get slightly easier, but we would still need a slicing function like batches (to slice offset and max), which now is just a minimal overhead to the batches function (just a change of the max value and the offset).

So overall imo we don't gain much by moving it to relay.

but we would still need a slicing function like batches

That makes sense!

So overall imo we don't gain much by moving it to relay.

Even if we keep batching on the Lua side, don't we also need batching on the Rust side? Not sure if we would run into size limits or network timeouts otherwise.

I am not sure how Redis and the network behaves here, in my local tests it didn't make a difference but there is also no latency and Redis does not do anything else (e.g. rate limiting etc.)

Would we batch and use a pipeline, I feel like that would make no difference then?

relay-cardinality/src/cardinality.lua

relay-cardinality/src/redis.rs

jjbayer

Couple of questions, but no blockers!

Please update the PR title & description to mention the cache as well as the Lua script.

jjbayer · 2023-12-19T10:38:19Z

relay-cardinality/src/cardinality.lua

+    for i = 1, #KEYS do
+        local is_working_set = i == 1
+
+        for from, to in batches(#t, 7000, offset, max) do


but we would still need a slicing function like batches

That makes sense!

So overall imo we don't gain much by moving it to relay.

Even if we keep batching on the Lua side, don't we also need batching on the Rust side? Not sure if we would run into size limits or network timeouts otherwise.

jjbayer · 2023-12-19T10:56:40Z

relay-cardinality/src/redis.rs

 /// Implementation uses Redis sets to keep track of cardinality.
 pub struct RedisSetLimiter {
    redis: RedisPool,
    window: SlidingWindow,
    script: CardinalityScript,
+    cache: Cache,


nit: Does the cache have to be a property of RedisSetLimiter? Since it's independent of redis, could it be part of CardinalityLimiter?

Or go fancy and use the Composite Pattern:

classDiagram Limiter <|-- CachedLimiter Limiter <|-- RedisSetLimiter CachedLimiter "1" *-- "2" Limiter

Loading

👍 definitely can be improved, initially this was heavily tied into the Lua logic, but it is pretty separate now. Ill do this in a followup PR.

It is still a bit tricky since it does require the same sliding window logic and ties into the splitting by scope logic which is Redis specific.

relay-cardinality/src/redis.rs

* master: (35 commits) fix(spans): Parse quotes in MySQL (#2846) ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter (#2849) fix(spans): Detect hex with fallback scrubber (#2868) release: 23.12.0 Revert "ci: Update upload-artifact and download-artifact actions" (#2866) Revert "build: Update axum and http" (#2863) feat(spans): Allow resource.img spans (#2855) build: Update axum and http (#2844) fix(build): Add additional dependencies to the release build (#2858) ci: Update upload-artifact and download-artifact actions (#2861) feat(spans): Parse timestamps from strings (#2857) fix(spans): Scrub integer file extensions (#2856) feat(spans): Remove unused transaction tag from resource metrics (#2853) ref(cardinality): Recover buckets on cardinality limiter failure (#2852) feat(server): Org rate limit per metric bucket (#2836) ref(spans): List metric tags explicitly (#2834) feat(spans): Resource response sizes as measurements (#2845) feat(crons): Add thresholds to monitor config payload (#2842) feat(spans): Allow ingestion of metrics summary on spans (#2823) ref(crons): Add documentation to CheckInMessageType (#2840) ...

* master: feat(processor): Add a metric to track the time of all messages (#2877) build(py): Update to python 3.9 (#2876) fix(spans): Collapse constants in SQL select (#2867) fix(spans): Parse quotes in MySQL (#2846) ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter (#2849) fix(spans): Detect hex with fallback scrubber (#2868)

Dav1dde force-pushed the dav1d/cardinality-lua branch 3 times, most recently from 226d4b1 to 313f0df Compare December 13, 2023 15:12

ref(cardinality): Use a lua script for the cardinality limiter

20d0799

Dav1dde force-pushed the dav1d/cardinality-lua branch from 313f0df to 20d0799 Compare December 13, 2023 15:39

Dav1dde commented Dec 13, 2023

View reviewed changes

relay-cardinality/src/statsd.rs Outdated Show resolved Hide resolved

Dav1dde requested a review from jjbayer December 13, 2023 15:53

Dav1dde self-assigned this Dec 13, 2023

Dav1dde marked this pull request as ready for review December 13, 2023 17:25

Dav1dde requested a review from a team as a code owner December 13, 2023 17:25

Dav1dde commented Dec 13, 2023

View reviewed changes

relay-cardinality/src/redis.rs Show resolved Hide resolved

Dav1dde mentioned this pull request Dec 14, 2023

[EPIC] Move the metrics cardinality limiter to Relay #2717

Closed

jjbayer reviewed Dec 14, 2023

View reviewed changes

optimize lua script to batch calls

2df679e

Dav1dde force-pushed the dav1d/cardinality-lua branch from 5fdf9c3 to 2df679e Compare December 15, 2023 14:34

jjbayer approved these changes Dec 18, 2023

View reviewed changes

implement caching layer for cardinality

1157c5c

Dav1dde force-pushed the dav1d/cardinality-lua branch from 2b3423d to 1157c5c Compare December 18, 2023 16:29

review changes, new metrics

2782500

Dav1dde force-pushed the dav1d/cardinality-lua branch from 470e1df to 2782500 Compare December 18, 2023 17:04

Dav1dde added 2 commits December 18, 2023 18:08

itertools is no longer a dependency

a26c9f6

dont need to convert a string to a string

1cd6c57

Dav1dde requested a review from jjbayer December 18, 2023 17:12

get rid of Hash in metrics

37369c2

Dav1dde force-pushed the dav1d/cardinality-lua branch from d151122 to 37369c2 Compare December 18, 2023 17:14

Dav1dde added 2 commits December 18, 2023 18:46

Merge branch 'master' into dav1d/cardinality-lua

5db88ff

fix benchmarks for CI, when run without all features

ac109fc

jjbayer approved these changes Dec 19, 2023

View reviewed changes

Dav1dde changed the title ~~ref(cardinality): Use a lua script for the cardinality limiter~~ ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter Dec 19, 2023

review changes

ac35d4b

Dav1dde added 2 commits December 19, 2023 13:55

Merge remote-tracking branch 'origin/master' into dav1d/cardinality-lua

806eec9

fix changelog

23f7250

Dav1dde merged commit c9198f9 into master Dec 19, 2023
21 checks passed

Dav1dde deleted the dav1d/cardinality-lua branch December 19, 2023 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter #2849

ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter #2849

Dav1dde commented Dec 13, 2023 •

edited

Loading

jjbayer left a comment

jjbayer left a comment

jjbayer Dec 18, 2023

Dav1dde Dec 18, 2023

jjbayer Dec 19, 2023

Dav1dde Dec 19, 2023

Dav1dde Dec 19, 2023

jjbayer left a comment

jjbayer Dec 19, 2023

jjbayer Dec 19, 2023

Dav1dde Dec 19, 2023 •

edited

Loading

ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter #2849

ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter #2849

Conversation

Dav1dde commented Dec 13, 2023 • edited Loading

jjbayer left a comment

Choose a reason for hiding this comment

jjbayer left a comment

Choose a reason for hiding this comment

jjbayer Dec 18, 2023

Choose a reason for hiding this comment

Dav1dde Dec 18, 2023

Choose a reason for hiding this comment

jjbayer Dec 19, 2023

Choose a reason for hiding this comment

Dav1dde Dec 19, 2023

Choose a reason for hiding this comment

Dav1dde Dec 19, 2023

Choose a reason for hiding this comment

jjbayer left a comment

Choose a reason for hiding this comment

jjbayer Dec 19, 2023

Choose a reason for hiding this comment

jjbayer Dec 19, 2023

Choose a reason for hiding this comment

Dav1dde Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Dav1dde commented Dec 13, 2023 •

edited

Loading

Dav1dde Dec 19, 2023 •

edited

Loading