[CapMan self-serve] Don't emit Sentry warnings for throttled queries to Snuba #75879
Conversation
github-actions
bot
added
the
Scope: Backend
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: src/sentry/utils/snuba.py
Did you find this useful? React with a 👍 or 👎 |
xurui-c
force-pushed
the
rachel/noWarnings
branch
from
200184a to
b587759
Compare
| if ( | ||
| "throttled_by" in quota_allowance_summary | ||
| and quota_allowance_summary["throttled_by"] | ||
| ): | ||
| metrics.incr("snuba.client.query.throttle", tags={"referrer": referrer}) | ||
| if random.random() < 0.01: | ||
| logger.warning( | ||
| "Warning: Query is throttled", extra={"response.data": response.data} | ||
| ) | ||
| sentry_sdk.capture_message( | ||
| f"Warning: Query from referrer {referrer} is throttled", level="warning" | ||
| ) |
There was a problem hiding this comment.
What if, instead of deleting this, we made it configurable-by-referrer and default off?
So if there is a product team interested in monitoring throttles we can quickly turn on warnings for their referrer, but we don't annoy certain people by filling the issue dashboard with throttle warnings that won't be dealt with.
There was a problem hiding this comment.
I think this is a good idea but knowing the scope (time and efforts) will be crucial. The configurability based on the referrer sounds good as it won't overpopulate the sentry dashboard. But my request is to take this suggestion as a follow-up in addition to the larger change @volokluev and @xurui-c were discussing. For now, I vote to merge the PR and begin scoping the improvements.
onkar
left a comment
onkar
left a comment
There was a problem hiding this comment.
LGTM, let's include the suggestion in the scoping of the larger work.
| if ( | ||
| "throttled_by" in quota_allowance_summary | ||
| and quota_allowance_summary["throttled_by"] | ||
| ): | ||
| metrics.incr("snuba.client.query.throttle", tags={"referrer": referrer}) | ||
| if random.random() < 0.01: | ||
| logger.warning( | ||
| "Warning: Query is throttled", extra={"response.data": response.data} | ||
| ) | ||
| sentry_sdk.capture_message( | ||
| f"Warning: Query from referrer {referrer} is throttled", level="warning" | ||
| ) |
There was a problem hiding this comment.
I think this is a good idea but knowing the scope (time and efforts) will be crucial. The configurability based on the referrer sounds good as it won't overpopulate the sentry dashboard. But my request is to take this suggestion as a follow-up in addition to the larger change @volokluev and @xurui-c were discussing. For now, I vote to merge the PR and begin scoping the improvements.
xurui-c
changed the title
Allocation policies are our mechanism for doing traffic management for Snuba queries. Currently, we see a lot of warnings (Warning: Query from referrer ... is throttled) on Sentry Issues. This is because these queries are throttled due to being in the "warning zone"; however, we got feedback that this isn't actually actionable, so we're getting rid of them. We will only emit Sentry errors for rejected queries.