feat(rate-limiting): Switch global rate limiter to a service #4581
Conversation
iambriccardo
changed the title
iambriccardo
force-pushed
the
riccardo/feat/global-limits-service
branch
from
58fd44e to
354efca
Compare
relay-quotas/src/global.rs
Outdated
| /// This is typically sent to a [`GlobalRateLimitsService`] to determine which quotas | ||
| /// should be rate limited based on the current usage. | ||
| pub struct CheckRateLimited { | ||
| pub quotas: Vec<OwnedRedisQuota>, |
There was a problem hiding this comment.
I am not happy with this solution of owning the data by cloning the original, but the alternative I found was using Arc<Quota> in configs, which I deemed too complex.
| /// A trait that exposes methods to check global rate limits. | ||
| pub trait GlobalLimiter { | ||
| /// Returns the [`RedisQuota`]s that should be rate limited. | ||
| fn check_global_rate_limits<'a>( |
There was a problem hiding this comment.
I decided to hide the OwnedRedisQuota type from the trait to provide flexibility to the implementers, at the cost of some inefficiency, since the service implementation must convert data from RedisQuota to OwnedRedisQuota.
| // | ||
| // The operation is O(n^2) but we are assuming the number of quotas is bounded by a low number | ||
| // since now they are only used for metric buckets limiting. | ||
| let rate_limited_global_quotas = |
There was a problem hiding this comment.
Not happy about this, but it felt like a good tradeoff between proper abstraction and performance. If we really want to avoid the backward mapping, we can directly return OwnedRedisQuota, as the caller side can support that, but it would tie the abstraction to the Service which requires the transfer of owned data.
A middle ground could be returning the indexes of the quotas that have been rate-limited, but this adds unnecessary complexity for relatively little benefit.
| type Interface = GlobalRateLimits; | ||
|
|
||
| async fn run(self, mut rx: Receiver<Self::Interface>) { | ||
| let limiter = Arc::new(Mutex::new(self.limiter)); |
There was a problem hiding this comment.
This ugly mutex will be here until we switch to async Redis.
| let (mut enforcement, mut rate_limits) = envelope_limiter | ||
| .compute(envelope.envelope_mut(), &scoping) | ||
| .await?; |
There was a problem hiding this comment.
This should not need to be a future. And we should also not even have it in the signature even if it technically never goes to redis to avoid it even happening in the future.
There was a problem hiding this comment.
I did try to make this work for both sync and async, but it’s not feasible. Besides duplicating the code, I see no better solution.
One approach I explored was to make the constructor accept a normal closure and convert it inside to return a Future, but since closures are anonymous, we can’t name the return value of the method.
This PR switches the global rate limiter from a locked struct to a service. The rationale for this change is to enable FIFO behavior when checking global rate limits from the
RedisRateLimiter.This change is necessary for the upcoming async Redis integration, since we observed that holding a Tokio lock across an await point causes issues in task scheduling and doesn't lead to strict FIFO ordering. More details are provided in the code comments.
In addition to converting
GlobalRateLimitsinto aService, this PR enhances documentation and improves the overall clarity of the relay-quotas module.Closes: https://github.com/getsentry/team-ingest/issues/673