perf(crons): Enforce crons rate limiting at the Relay level - #6330
perf(crons): Enforce crons rate limiting at the Relay level#6330wedamija wants to merge 3 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| let monitor_slug = check_in.monitor_slug.clone(); | ||
| let environment = check_in.environment.clone(); |
There was a problem hiding this comment.
lol it really was already deserializing it huh
e1c1279 to
8f34f5e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8f34f5e. Configure here.
We already route by this key in the consumer when we put things into batches, but missed it here. This has the effect of causing extremely hot partitions for any monitor that has a lot of envs. This can also be exacerbated if they're also sending more than the rate limit. A missing or empty environment is normalized to "production" for the routing key, matching how Sentry resolves it in `MonitorEnvironment.objects.ensure_environment`. Without that, check-ins omitting the environment and those sending "production" describe one monitor environment but would route to different partitions, which have independent lag. Only the routing key is normalized; the payload is forwarded untouched.
This enforces the 6 checkin per monitor/env limit at the Relay level. It's also configurable via options automator using the relay.cron-monitor-rate-limit option. We care about this because we partition checkins in the Kafka topic by monitor environment. So when we have someone sending a large number of checkins to a particular monitorenv, we end up with hot partitions that cause the consumer to lag behind. Ideally this should mean we don't see any rate limit events happen on the consumer, and we should see more stability there.
8f34f5e to
2ba7aa5
Compare
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
This is the wrong approach, quotas are defined in Sentry and only in Sentry.
If you need more functionality than the quota system currently allows for, we need to build out these capabilities first.
Having quotas with more dimensions came up before for generic metrics/ddm, where we added the namespace to the quota, arguably this should've been something more generic. So we'll have to look into this first.
Note, this also requires thinking about how these quotas are propagated to clients/SDKs and how clients should enforce these.
It looks like with this PR we'd propagate a generic Monitor quota to clients, which I don't think is what you intended. With multiple Relays this may even have the effect to drop all monitors.
Maybe we're just misusing the quota system here. We already have a 6 checkin/minute per monitor environment ratelimit on the consumer side. We're just looking to move that validation out to the relay level instead. Is there a simple way to manage that? |
Well we don't really have that right. We have a per project per minute check-in rate limit where the rate limit is computed as the number of monitor environments * 6, that gets propagated to Relay. What we really want is to actually rate limit per monitor environment. |
We have this on the consumer here: https://github.com/getsentry/sentry/blob/c821d197ef6cad954a17a707c4509ed56146f0c4/src/sentry/monitors/consumers/monitor_consumer.py#L213-L240 |
The right way to do this is to extend the quota system to support this requirement. This approach bypasses the quota system while at the same time re-using the Redis parts of it. To actually support this, we need to extend the quota system first. Once this new type of quota is supported, other items can also make use of it and limits will be correctly propagated downstream. This change will require a few more fundamental changes, let's discuss the urgency/timelines either on Slack or in a quick meeting so we can check how we can extend the existing quota platform to support your usecase. |
3 participants
This enforces the 6 checkin per monitor/env limit at the Relay level. It's also configurable via options automator using the relay.cron-monitor-rate-limit option.
We care about this because we partition checkins in the Kafka topic by monitor environment. So when we have someone sending a large number of checkins to a particular monitorenv, we end up with hot partitions that cause the consumer to lag behind.
Ideally this should mean we don't see any rate limit events happen on the consumer, and we should see more stability there.