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
226d4b1 to
313f0df
Compare
313f0df to
20d0799
Compare
jjbayer
left a comment
There was a problem hiding this comment.
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.
5fdf9c3 to
2df679e
Compare
jjbayer
left a comment
There was a problem hiding this comment.
Still some open comments, but no blockers as far as I am concerned!
| for i = 1, #KEYS do | ||
| local is_working_set = i == 1 | ||
|
|
||
| for from, to in batches(#t, 7000, offset, max) do |
There was a problem hiding this comment.
I think we should batch exclusively on the Relay side. That would simplify this script, and we need some sort of batching there anyway.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Would we batch and use a pipeline, I feel like that would make no difference then?
2b3423d to
1157c5c
Compare
470e1df to
2782500
Compare
d151122 to
37369c2
Compare
jjbayer
left a comment
There was a problem hiding this comment.
Couple of questions, but no blockers!
Please update the PR title & description to mention the cache as well as the Lua script.
| for i = 1, #KEYS do | ||
| local is_working_set = i == 1 | ||
|
|
||
| for from, to in batches(#t, 7000, offset, max) do |
There was a problem hiding this comment.
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.
| redis: RedisPool, | ||
| window: SlidingWindow, | ||
| script: CardinalityScript, | ||
| cache: Cache, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
👍 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.
* 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)
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