Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CapMan visibility] add throttling to ReferrerGuardRail policy #6014

Merged
merged 12 commits into from
Jun 11, 2024

Conversation

xurui-c
Copy link
Member

@xurui-c xurui-c commented Jun 10, 2024

https://getsentry.atlassian.net/browse/SNS-2793

The ReferrerGuardRail policy prevents referrers from sending too many queries by rejecting them once a referrer reaches a rejection threshold. I add a throttle threshold so the offending referrer will get throttled first before getting rejected. ReferrerGuardRailPolicy will half the threads when the number of concurrent queries goes over 50% of the rejection threshold for that referrer (e.g. subscriptions_executor rejects at 200 concurrent queries, we throttle at 100 queries)

@xurui-c xurui-c marked this pull request as ready for review June 10, 2024 19:20
@xurui-c xurui-c requested a review from a team as a code owner June 10, 2024 19:20
@xurui-c xurui-c requested a review from volokluev June 10, 2024 19:20
Copy link
Member

@volokluev volokluev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good overall just one change

snuba/query/allocation_policies/per_referrer.py Outdated Show resolved Hide resolved
@xurui-c xurui-c requested a review from volokluev June 10, 2024 21:11
@volokluev
Copy link
Member

How are you planning to roll this out?

@xurui-c
Copy link
Member Author

xurui-c commented Jun 11, 2024

How are you planning to roll this out?

In this PR, I set the throttle dividers to 1 so that the rejection threshold and the throttle threshold are the same, so effectively this PR will create the options on Snuba Admin without doing any throttling. Then, within S4S, I will adjust the throttle dividers. With this, I expect to see queries being throttled in DD under the metric concurrent_queries_throttled. I will change the throttle dividers to 0.5 afterwards.

Copy link

codecov bot commented Jun 11, 2024

Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time.

❌ Failed Test Results:

Completed 1614 tests with 1 failed, 1611 passed and 2 skipped.

View the full list of failed tests
Test Description Failure message
Testsuite:
pytest

Test name:
tests.query.allocation_policies.test_per_referrer.TestPerReferrerPolicy::test_throttle

Envs:
- default
Traceback (most recent call last):
File ".../query/allocation_policies/test_per_referrer.py", line 69, in test_throttle
assert first_quota_allowance.max_threads == policy.max_threads
AssertionError: assert 0 == 10
+ where 0 = QuotaAllowance(can_run=True, max_threads=0, explanation={'reason': 'within limit', 'policy': 'referrer_guard_rail_policy', 'referrer': 'statistical_detectors', 'storage_key': 'StorageKey.GENERIC_METRICS_DISTRIBUTIONS'}).max_threads
+ and 10 = <snuba.query.allocation_policies.per_referrer.ReferrerGuardRailPolicy object at 0x7f20c1ca39d0>.max_threads

Rachel Chen added 2 commits June 11, 2024 11:19
@xurui-c xurui-c requested a review from volokluev June 11, 2024 18:56
Copy link
Contributor

@onewland onewland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attached some suggestions for changes, but fundamentally the concept and approach seem sound

snuba/query/allocation_policies/per_referrer.py Outdated Show resolved Hide resolved
@xurui-c xurui-c requested a review from onewland June 11, 2024 22:16
snuba/query/allocation_policies/per_referrer.py Outdated Show resolved Hide resolved
snuba/query/allocation_policies/per_referrer.py Outdated Show resolved Hide resolved
@xurui-c xurui-c merged commit 85d8728 into master Jun 11, 2024
29 checks passed
@xurui-c xurui-c deleted the rachel/throttleReferrerGuardRailPolicy branch June 11, 2024 22:54
@xurui-c xurui-c changed the title [CapMan] Add throttling to ReferrerGuardRail policy [CapMan visibility] add throttling to ReferrerGuardRail policy Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants