Skip to content

Commit

Permalink
fix(capman): register statistical detectors cross org query referrer,…
Browse files Browse the repository at this point in the history
… make overrides of unregistered referrers not possible (#5422)

I turned on the cross org query policy and even though I had an override for the statistical detectors in it, the policy did not pick it up because it was not a registered referrer. This took too long to understand. This PR:

https://sentry.sentry.io/issues/4889272228/?project=1

* registered the referrer that was being overthrottled
* Makes it so unregistered referrers cannot be overridden at runtime (and tells the user what to do to make that happen)
* Updates the policy decision explanation to tell the user why what is happening is happening

I considered making it possible to override unregistered referrers but I'm being especially protective of this class of queries. They are new to sentry and I don't want people making a whole bunch of them without having us know what they are and what they are for
  • Loading branch information
volokluev authored Jan 18, 2024
1 parent e4d1601 commit 03a82d5
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ allocation_policies:
statistical_detectors.distributions.fetch_top_transaction_names:
max_threads: 4
concurrent_limit: 10
statistical_detectors.distributions.fetch_transaction_timeseries:
max_threads: 4
concurrent_limit: 20



Expand Down
24 changes: 22 additions & 2 deletions snuba/query/allocation_policies/cross_org.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from snuba.datasets.storages.storage_key import StorageKey
from snuba.query.allocation_policies import (
AllocationPolicyConfig,
InvalidPolicyConfig,
QueryResultOrError,
QuotaAllowance,
)
Expand Down Expand Up @@ -55,6 +56,21 @@ class CrossOrgQueryAllocationPolicy(BaseConcurrentRateLimitAllocationPolicy):
def rate_limit_name(self) -> str:
return "cross_org_query_policy"

def set_config_value(
self,
config_key: str,
value: Any,
params: dict[str, Any] = {},
user: str | None = None,
) -> None:
"""makes sure only registered referrers can be overridden"""
referrer = params.get("referrer", "")
if not self._referrer_is_registered(referrer):
raise InvalidPolicyConfig(
f"Referrer {referrer} is not registered in the the {self._storage_key.value} yaml. Register it first to be able to override its limits"
)
super().set_config_value(config_key, value, params, user)

def _additional_config_definitions(self) -> list[AllocationPolicyConfig]:
return super()._additional_config_definitions() + [
AllocationPolicyConfig(
Expand Down Expand Up @@ -138,7 +154,6 @@ def _get_quota_allowance(
self, tenant_ids: dict[str, str | int], query_id: str
) -> QuotaAllowance:
referrer = str(tenant_ids.get("referrer", "no_referrer"))

if not self._referrer_is_registered(referrer) and not self.is_cross_org_query(
tenant_ids
):
Expand All @@ -154,10 +169,15 @@ def _get_quota_allowance(
query_id,
RateLimitParameters(self.rate_limit_name, referrer, None, concurrent_limit),
)
decision_explanation = {"reason": explanation}
if not self._referrer_is_registered(referrer):
decision_explanation[
"cross_org_query"
] = f"This referrer is not registered for the current storage {self._storage_key.value}, if you want to increase its limits, register it in the yaml of the CrossOrgQueryAllocationPolicy"
return QuotaAllowance(
can_run=can_run,
max_threads=self._get_max_threads(referrer),
explanation={"reason": explanation},
explanation=decision_explanation,
)

def _update_quota_balance(
Expand Down
33 changes: 32 additions & 1 deletion tests/query/allocation_policies/test_cross_org_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from snuba.query.allocation_policies import (
AllocationPolicyViolation,
InvalidPolicyConfig,
QueryResultOrError,
)
from snuba.query.allocation_policies.cross_org import CrossOrgQueryAllocationPolicy
Expand Down Expand Up @@ -130,6 +131,34 @@ def test_override(self):
tenant_ids={"referrer": "statistical_detectors"}, query_id="2"
)

@pytest.mark.redis_db
def test_override_unregistered_referrer(self):
# overrides of unregistered referrers should not be allowed
policy = CrossOrgQueryAllocationPolicy.from_kwargs(
**{
"storage_key": "generic_metrics_distributions",
"required_tenant_types": ["referrer"],
"cross_org_referrer_limits": {
"statistical_detectors": {
"concurrent_limit": 1,
"max_threads": 1,
},
},
}
)
with pytest.raises(InvalidPolicyConfig):
policy.set_config_value(
"referrer_concurrent_override",
6,
{"referrer": ""},
)
# this referrer is registered so that's fine
policy.set_config_value(
"referrer_concurrent_override",
6,
{"referrer": "statistical_detectors"},
)

@pytest.mark.redis_db
def test_throttle_cross_org_query_with_unregistered_referrer(self):
policy = CrossOrgQueryAllocationPolicy.from_kwargs(
Expand All @@ -151,8 +180,10 @@ def test_throttle_cross_org_query_with_unregistered_referrer(self):
assert allowance.can_run is True
assert allowance.max_threads == 1

with pytest.raises(AllocationPolicyViolation):
with pytest.raises(AllocationPolicyViolation) as violation:
allowance = policy.get_quota_allowance(
tenant_ids={"referrer": "unregistered", "cross_org_query": 1},
query_id="2",
)

assert violation.value.quota_allowance["explanation"]["cross_org_query"] == "This referrer is not registered for the current storage generic_metrics_distributions, if you want to increase its limits, register it in the yaml of the CrossOrgQueryAllocationPolicy" # type: ignore

0 comments on commit 03a82d5

Please sign in to comment.