Skip to content

Commit

Permalink
fix(quotas): Make redis rate limiter work with quantity 0 (#1519)
Browse files Browse the repository at this point in the history
For #1515, it is required to check for a required quota of another data
category without incrementing it. This PR updates the Redis LUA script
to support a rate limiting quantity of `0`, which checks for existing
rate limits without incrementing internal counters.

The rate limiter gains a new explicit branch to check whether the
quantity is `0`. 

Co-authored-by: Jan Michael Auer <mail@jauer.org>
  • Loading branch information
jjbayer and jan-auer authored Oct 7, 2022
1 parent d00d437 commit 926827a
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 5 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
- Migrate to 2021 Rust edition. ([#1510](https://github.com/getsentry/relay/pull/1510))
- Make the profiling frame object compatible with the stacktrace frame object from event. ([#1512](https://github.com/getsentry/relay/pull/1512))
- Fix quota DataCategory::TransactionProcessed serialisation to match that of the CAPI. ([#1514](https://github.com/getsentry/relay/pull/1514))
- Remove unused rate_limits from ProcessEnvelopeState. ([#1516](https://github.com/getsentry/relay/pull/1516))
- Support checking quotas in the Redis rate limiter without incrementing them. ([#1519](https://github.com/getsentry/relay/pull/1519))

## 22.9.0

Expand Down
16 changes: 12 additions & 4 deletions relay-quotas/src/is_rate_limited.lua
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
-- ``ARGV`` (3 per quota):
-- * [number] Quota limit. Can be ``-1`` for unlimited quotas.
-- * [number] Absolute Expiration time as Unix timestamp (secs since 1.1.1970 ) for the key.
-- * [number] Quantity to increment the quota by.
-- * [number] Quantity to increment the quota by, or ``0`` to check without incrementing.
--
-- For example, to check the following two quotas each with a timeout of 10 minutes from now:
-- * Key ``foo``, refund key ``foo_refund``, limit ``10``; quantity ``5``
Expand Down Expand Up @@ -43,7 +43,13 @@ for i=0, num_quotas - 1 do
local rejected = false
-- limit=-1 means "no limit"
if limit >= 0 then
rejected = (redis.call('GET', KEYS[k]) or 0) - (redis.call('GET', KEYS[k + 1]) or 0) + quantity > limit
local consumed = (redis.call('GET', KEYS[k]) or 0) - (redis.call('GET', KEYS[k + 1]) or 0)
-- we never increment past the limit. if quantity is 0, check instead if we reached limit.
if quantity == 0 then
rejected = consumed >= limit
else
rejected = consumed + quantity > limit
end
end

if rejected then
Expand All @@ -57,8 +63,10 @@ if not failed then
local k = i * 2 + 1
local v = i * 3 + 1

redis.call('INCRBY', KEYS[k], ARGV[v + 2])
redis.call('EXPIREAT', KEYS[k], ARGV[v + 1])
if tonumber(ARGV[v + 2]) > 0 then
redis.call('INCRBY', KEYS[k], ARGV[v + 2])
redis.call('EXPIREAT', KEYS[k], ARGV[v + 1])
end
end
end

Expand Down
53 changes: 53 additions & 0 deletions relay-quotas/src/redis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ impl RedisRateLimiter {
///
/// If no key is specified, then only organization-wide and project-wide quotas are checked. If
/// a key is specified, then key-quotas are also checked.
///
/// The passed `quantity` may be `0`. In this case, the rate limiter will check if the quota
/// limit has been reached or exceeded without incrementing it in the success case. This can be
/// useful to check for required quotas in a different data category.
pub fn is_rate_limited(
&self,
quotas: &[Quota],
Expand Down Expand Up @@ -364,6 +368,55 @@ mod tests {
}
}

#[test]
fn test_quantity_0() {
let quotas = &[Quota {
id: Some(format!("test_quantity_0_{:?}", SystemTime::now())),
categories: DataCategories::new(),
scope: QuotaScope::Organization,
scope_id: None,
limit: Some(1),
window: Some(60),
reason_code: Some(ReasonCode::new("get_lost")),
}];

let scoping = ItemScoping {
category: DataCategory::Error,
scoping: &Scoping {
organization_id: 42,
project_id: ProjectId::new(43),
project_key: ProjectKey::parse("a94ae32be2584e0bbd7a4cbb95971fee").unwrap(),
key_id: Some(44),
},
};

let rate_limiter = build_rate_limiter();

// limit is 1, so first call not rate limited
assert!(!rate_limiter
.is_rate_limited(quotas, scoping, 1)
.unwrap()
.is_limited());

// quota is now exhausted
assert!(rate_limiter
.is_rate_limited(quotas, scoping, 1)
.unwrap()
.is_limited());

// quota is exhausted, regardless of the quantity
assert!(rate_limiter
.is_rate_limited(quotas, scoping, 0)
.unwrap()
.is_limited());

// quota is exhausted, regardless of the quantity
assert!(rate_limiter
.is_rate_limited(quotas, scoping, 1)
.unwrap()
.is_limited());
}

#[test]
fn test_bails_immediately_without_any_quota() {
let scoping = ItemScoping {
Expand Down

0 comments on commit 926827a

Please sign in to comment.