feat(api-abuse): Add ability to throttle threads for an entire referrer #3376
Conversation
Codecov ReportBase: 90.82% // Head: 21.85% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3376 +/- ##
===========================================
- Coverage 90.82% 21.85% -68.98%
===========================================
Files 705 664 -41
Lines 32541 31312 -1229
===========================================
- Hits 29555 6842 -22713
- Misses 2986 24470 +21484
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
When I look at how resource quota is used, this is the code where it is being used https://github.com/getsentry/snuba/blob/master/snuba/web/db_query.py#L343-L348. There is and condition which checks against project_rate_limit_stats and the concurrent parameter being greater than 1.
My question is if we apply only the referrer based rate limit settings, will the code where it is being used still be triggered? Maybe worth adding a test on that method to find out.
| # all requests with referrer = MYREFERRER are now capped at 20 threads | ||
|
|
||
| SET referrer_project_thread_quota_MYREFERRER_1337 = 2 | ||
| # all requests with referrer = MYREFERRER, project_id = 1337 are now |
There was a problem hiding this comment.
I think there should be a test to confirm this behaviour.
volokluev
force-pushed
the
volo/referrer_threadlimit
branch
from
9370af3
to
6cc8b34
Compare
|
|
||
|
|
||
| class ResourceQuotaProcessor(LogicalQueryProcessor): | ||
| """ | ||
| Applies a referrer/project thread quota to the query. | ||
| Applies a referrer/project thread quota to the query. Can throttle a referrer | ||
| or a (referrer, project) pair. The more specific restriction takes precedence |
There was a problem hiding this comment.
I wonder if precedence should be the minimum number of max threads. If REFERRER_{X} is capped at 2 threads globally, it doesn't seem like REFERRER_{X}_PROJECT_{Y} should be allowed to have 10.
Though in practice I'm guessing people won't tend to mess this up
snuba/web/db_query.py
Outdated
| query_settings: QuerySettings, | ||
| clickhouse_query_settings: MutableMapping[str, Any], | ||
| project_rate_limit_stats: Optional[RateLimitStats], | ||
| ) -> None: |
There was a problem hiding this comment.
Not a big deal because this method is private, but I think it would be nice (as a convention) to return a new QuerySettings with max_threads set and not modify the input.
In case a bad feature ships. We want to throttle an entire referrer, not just a specific project's threads