getsentry
feat(quotas): per-project abuse quotas
This was originally implemented in getsentry, however we'd like to now have per-project abuse quotas for everyone.
I thoroughly read all the code and yeah, barely anything needs to change since all this was designed pretty flexibly and well.
The default abuse window is 10 seconds since Relay can't really enforce 1s, and 10 seems to have worked well. When you Organization.update_option("project-abuse-quota.error-limit", 42), it'll effectively be 42 errors/s, but enforced as 420/10s.
I also wanna f/u with removing is_rate_limited everywhere here but it seems a little trickier with all the quota refund stuff tied to it. @jan-auer any pointers?
We should pull in https://github.com/getsentry/sentry/pull/33580 once approved.
Okay, so update on this...
- The current behavior as of dbdcdf7 is locked in (it's deployed in ST and working), so instead of moving code first then f/u with behavior change, it'd be better to lock in behavior with tests which I am still struggling to figure out.
- Contrary to 1, these options should really be moved to organizations rather than sentry-level. Right now, it's fine for ST, but we should never set these values in SaaS until they're org options. I'm not sure yet if org options would work for ST.
update to 2: oh lol, kind of amazing I didn't notice till now that i'm registering sentry options and referencing+modifying org options. So... these will have to change to org options. I also checked what we did for ST, we set org options, so no action needed.
@joshuarli in SaaS/getsentry we had both a global sentry option and organization options. both would work the same on ST and self-hosted. I would suggest to basically keep this pattern, since we do have the global option set and a few overrides for individual organizations.
i'm registering sentry options and referencing+modifying org options.
Right, and it only works right now because of the sentry: prefixed org options, which do not have to be registered. Wouldn't it be easier if you keep the structure from getsentry?
Alright, dusting this one off... I definitely confused myself quite a bit with this one. Even though we're already shipping this as is right now, parts of it don't really make sense. Gonna try and clear things up.
@rgibert @jan-auer Sorry for the huuuuuuge delay. I was totally confused for the longest time with this and kept putting it on the backburner.
Had a moment of clarity today and I think e82960feac01af159b5d581bb80855ad1461e303 makes much more sense with respect to global vs org options. Please give it a read. No changes required in https://github.com/getsentry/getsentry/pull/7278, I don't think.
Tests fixed and also adjusted. Still, having a tough time with the relay integration test. Since @rgibert wants this merged soon, I want to punt that to a followup, is this okay with you @jan-auer?
Please beware of getsentry/getsentry#8642
I see https://github.com/getsentry/getsentry/pull/8760 too, will make changes to reflect that here. Also, updated https://github.com/getsentry/getsentry/pull/7278.
re: https://github.com/getsentry/sentry/pull/33197#discussion_r983841220, it wasn't clear to me that we don't currently support reject-all for those options... I think I understand now. Addressed your feedback, please double check.
@joshuarli I think we're leaking between tests here. If I run test_uses_defined_quotas in isolation, the abuse quotas are not in the result set. Pretty sure it's this:
https://github.com/getsentry/sentry/blob/c93462b17a4320e31f8636a09a57c036e9768438/tests/sentry/quotas/test_redis.py#L139