Skip to content

Commit

Permalink
feat(cardinality-limit): Improve validation (#70217)
Browse files Browse the repository at this point in the history
Add upper limit for cardinality limit value.
Add tests that ensures that the generated limit is normalized.

Closes #69750
  • Loading branch information
ArthurKnaus authored and cmanallen committed May 21, 2024
1 parent 3a5a556 commit 29cda22
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 2 deletions.
12 changes: 11 additions & 1 deletion src/sentry/api/endpoints/project_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,19 @@ def validate_relayPiiConfig(self, value):
return validate_pii_config_update(organization, value)

def validate_relayCustomMetricCardinalityLimit(self, value):
if value is not None and value < 0:
if value is None:
return value

if value < 0:
raise serializers.ValidationError("Cardinality limit must be a non-negative integer.")

# Value is stored as uint32 in relay
# TODO: find a way to share this constant between relay and sentry
if value > 4_294_967_295:
raise serializers.ValidationError(
"Cardinality limit must be smaller or equal to 4,294,967,295."
)

return value

def validate_builtinSymbolSources(self, value):
Expand Down
22 changes: 21 additions & 1 deletion tests/sentry/api/endpoints/test_project_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import orjson
from django.db import router
from django.urls import reverse
from sentry_relay.processing import normalize_cardinality_limit_config

from sentry import audit_log
from sentry.constants import RESERVED_PROJECT_SLUGS, ObjectStatus
Expand Down Expand Up @@ -751,7 +752,10 @@ def test_custom_metrics_cardinality_limit(self):
self.proj_slug,
relayCustomMetricCardinalityLimit=1000,
)
assert self.project.get_option("relay.cardinality-limiter.limits") == [

config = self.project.get_option("relay.cardinality-limiter.limits")

assert config == [
{
"limit": {
"id": "project-override-custom",
Expand All @@ -762,6 +766,11 @@ def test_custom_metrics_cardinality_limit(self):
}
}
]

limit = config[0]["limit"]
normalized_limit = normalize_cardinality_limit_config(limit)
assert normalized_limit == limit

assert resp.data["relayCustomMetricCardinalityLimit"] == 1000

def test_custom_metrics_cardinality_limit_invalid_text(self):
Expand All @@ -784,6 +793,17 @@ def test_custom_metrics_cardinality_limit_invalid_negative_number(self):
"Cardinality limit must be a non-negative integer."
]

def test_custom_metrics_cardinality_limit_invalid_too_high(self):
resp = self.get_error_response(
self.org_slug,
self.proj_slug,
relayCustomMetricCardinalityLimit=4_294_967_296,
)
assert self.project.get_option("replay.cardinality-limiter.limts", []) == []
assert resp.data["relayCustomMetricCardinalityLimit"] == [
"Cardinality limit must be smaller or equal to 4,294,967,295."
]

def test_custom_metrics_cardinality_limit_accepts_none(self):
resp = self.get_success_response(
self.org_slug,
Expand Down

0 comments on commit 29cda22

Please sign in to comment.