Skip to content

Commit e7be12c

Browse files
nikkikapadianikkikapadia
authored
fix(aci): accept str instead of int for extrapolation mode in validator (#104015)
Slack thread for context: https://sentry.slack.com/archives/C07H2DHMSS0/p1764091469147239 Extrapolation mode would not be accepted as a string in the validator (which is what it gets passed to the backend as) so it was throwing an error on detector updates. Along with this I had to fix some tests and other logic that went into validating the extrapolation mode so we ensure we're comparing correct types.
1 parent 761e49e commit e7be12c

File tree

6 files changed

+124
-17
lines changed

6 files changed

+124
-17
lines changed

‎src/sentry/incidents/endpoints/organization_alert_rule_details.py‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,10 @@ def update_alert_rule(
125125
partial=True,
126126
)
127127
if validator.is_valid():
128-
if data.get("dataset") == Dataset.EventsAnalyticsPlatform.value:
128+
if (
129+
data.get("dataset", alert_rule.snuba_query.dataset)
130+
== Dataset.EventsAnalyticsPlatform.value
131+
):
129132
if data.get("extrapolation_mode"):
130133
old_extrapolation_mode = ExtrapolationMode(
131134
alert_rule.snuba_query.extrapolation_mode

‎src/sentry/incidents/metric_issue_detector.py‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,17 @@ def validate_conditions(self, value):
159159
def is_invalid_extrapolation_mode(old_extrapolation_mode, new_extrapolation_mode) -> bool:
160160
if type(new_extrapolation_mode) is int:
161161
new_extrapolation_mode = ExtrapolationMode(new_extrapolation_mode).name.lower()
162+
if type(new_extrapolation_mode) is ExtrapolationMode:
163+
new_extrapolation_mode = new_extrapolation_mode.name.lower()
162164
if type(old_extrapolation_mode) is int:
163165
old_extrapolation_mode = ExtrapolationMode(old_extrapolation_mode).name.lower()
166+
if type(old_extrapolation_mode) is ExtrapolationMode:
167+
old_extrapolation_mode = old_extrapolation_mode.name.lower()
168+
if (
169+
new_extrapolation_mode is not None
170+
and ExtrapolationMode.from_str(new_extrapolation_mode) is None
171+
):
172+
return True
164173
if (
165174
new_extrapolation_mode == ExtrapolationMode.SERVER_WEIGHTED.name.lower()
166175
and old_extrapolation_mode != ExtrapolationMode.SERVER_WEIGHTED.name.lower()
@@ -169,6 +178,16 @@ def is_invalid_extrapolation_mode(old_extrapolation_mode, new_extrapolation_mode
169178
return False
170179

171180

181+
def format_extrapolation_mode(extrapolation_mode) -> ExtrapolationMode | None:
182+
if extrapolation_mode is None:
183+
return None
184+
if type(extrapolation_mode) is int:
185+
return ExtrapolationMode(extrapolation_mode)
186+
if type(extrapolation_mode) is ExtrapolationMode:
187+
return extrapolation_mode
188+
return ExtrapolationMode.from_str(extrapolation_mode)
189+
190+
172191
class MetricIssueDetectorValidator(BaseDetectorTypeValidator):
173192
data_sources = serializers.ListField(
174193
child=SnubaQueryValidator(timeWindowSeconds=True), required=False
@@ -196,8 +215,8 @@ def _validate_transaction_dataset_deprecation(self, dataset: Dataset) -> None:
196215
"Creation of transaction-based alerts is disabled, as we migrate to the span dataset. Create span-based alerts (dataset: events_analytics_platform) with the is_transaction:true filter instead."
197216
)
198217

199-
def _validate_extrapolation_mode(self, extrapolation_mode: str) -> None:
200-
if extrapolation_mode == ExtrapolationMode.SERVER_WEIGHTED.value:
218+
def _validate_extrapolation_mode(self, extrapolation_mode: ExtrapolationMode) -> None:
219+
if extrapolation_mode == ExtrapolationMode.SERVER_WEIGHTED:
201220
raise serializers.ValidationError(
202221
"server_weighted extrapolation mode is not supported for new detectors."
203222
)
@@ -288,6 +307,10 @@ def update_data_source(
288307
"Failed to send data to Seer, cannot update detector"
289308
)
290309

310+
extrapolation_mode = format_extrapolation_mode(
311+
data_source.get("extrapolation_mode", snuba_query.extrapolation_mode)
312+
)
313+
291314
update_snuba_query(
292315
snuba_query=snuba_query,
293316
query_type=data_source.get("query_type", snuba_query.type),
@@ -298,9 +321,7 @@ def update_data_source(
298321
resolution=timedelta(seconds=data_source.get("resolution", snuba_query.resolution)),
299322
environment=data_source.get("environment", snuba_query.environment),
300323
event_types=data_source.get("event_types", [event_type for event_type in event_types]),
301-
extrapolation_mode=data_source.get(
302-
"extrapolation_mode", snuba_query.extrapolation_mode
303-
),
324+
extrapolation_mode=extrapolation_mode,
304325
)
305326

306327
def update_anomaly_detection(self, instance: Detector, validated_data: dict[str, Any]) -> bool:

‎src/sentry/snuba/models.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,13 @@ def as_choices(cls):
4343
def as_text_choices(cls):
4444
return tuple((mode.name.lower(), mode.value) for mode in cls)
4545

46+
@classmethod
47+
def from_str(cls, name: str):
48+
for mode in cls:
49+
if mode.name.lower() == name:
50+
return mode
51+
return None
52+
4653

4754
@region_silo_model
4855
class SnubaQuery(Model):

‎src/sentry/snuba/snuba_query_validator.py‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ class SnubaQueryValidator(BaseDataSourceValidator[QuerySubscription]):
8787
required=False,
8888
allow_empty=False,
8989
)
90-
extrapolation_mode = serializers.IntegerField(required=False, allow_null=True)
90+
extrapolation_mode = serializers.CharField(required=False, allow_null=True)
9191

9292
class Meta:
9393
model = QuerySubscription
@@ -167,6 +167,15 @@ def validate_event_types(self, value: Sequence[str]) -> list[SnubaQueryEventType
167167

168168
return validated
169169

170+
def validate_extrapolation_mode(self, extrapolation_mode: str) -> ExtrapolationMode | None:
171+
if extrapolation_mode is not None:
172+
extrapolation_mode_enum = ExtrapolationMode.from_str(extrapolation_mode)
173+
if extrapolation_mode_enum is None:
174+
raise serializers.ValidationError(
175+
f"Invalid extrapolation mode: {extrapolation_mode}"
176+
)
177+
return extrapolation_mode_enum
178+
170179
def validate(self, data):
171180
data = super().validate(data)
172181
self._validate_aggregate(data)
@@ -406,9 +415,6 @@ def _validate_group_by(self, value: Sequence[str] | None) -> Sequence[str] | Non
406415

407416
@override
408417
def create_source(self, validated_data) -> QuerySubscription:
409-
extrapolation_mode = validated_data.get("extrapolation_mode")
410-
if extrapolation_mode is not None:
411-
extrapolation_mode = ExtrapolationMode(extrapolation_mode)
412418
snuba_query = create_snuba_query(
413419
query_type=validated_data["query_type"],
414420
dataset=validated_data["dataset"],
@@ -419,7 +425,7 @@ def create_source(self, validated_data) -> QuerySubscription:
419425
environment=validated_data["environment"],
420426
event_types=validated_data["event_types"],
421427
group_by=validated_data.get("group_by"),
422-
extrapolation_mode=extrapolation_mode,
428+
extrapolation_mode=validated_data.get("extrapolation_mode"),
423429
)
424430
return create_snuba_subscription(
425431
project=self.context["project"],

‎src/sentry/snuba/subscriptions.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ def update_snuba_query(
8989
resolution,
9090
environment,
9191
event_types,
92-
extrapolation_mode=None,
92+
extrapolation_mode: ExtrapolationMode | None = None,
9393
):
9494
"""
9595
Updates a SnubaQuery. Triggers updates to any related QuerySubscriptions.
@@ -131,9 +131,9 @@ def update_snuba_query(
131131
resolution=int(resolution.total_seconds()),
132132
environment=environment,
133133
extrapolation_mode=(
134-
extrapolation_mode
134+
extrapolation_mode.value
135135
if extrapolation_mode is not None
136-
else ExtrapolationMode.UNKNOWN.value
136+
else snuba_query.extrapolation_mode
137137
),
138138
)
139139
if new_event_types:

‎tests/sentry/incidents/endpoints/validators/test_validators.py‎

Lines changed: 73 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1238,7 +1238,7 @@ def test_invalid_extrapolation_mode_create(self) -> None:
12381238
"timeWindow": 3600,
12391239
"environment": self.environment.name,
12401240
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
1241-
"extrapolation_mode": ExtrapolationMode.SERVER_WEIGHTED.value,
1241+
"extrapolation_mode": ExtrapolationMode.SERVER_WEIGHTED.name.lower(),
12421242
},
12431243
],
12441244
}
@@ -1263,7 +1263,7 @@ def test_invalid_extrapolation_mode_update(self) -> None:
12631263
"timeWindow": 3600,
12641264
"environment": self.environment.name,
12651265
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
1266-
"extrapolation_mode": ExtrapolationMode.CLIENT_AND_SERVER_WEIGHTED.value,
1266+
"extrapolation_mode": ExtrapolationMode.CLIENT_AND_SERVER_WEIGHTED.name.lower(),
12671267
},
12681268
],
12691269
}
@@ -1283,7 +1283,7 @@ def test_invalid_extrapolation_mode_update(self) -> None:
12831283
"timeWindow": 3600,
12841284
"environment": self.environment.name,
12851285
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
1286-
"extrapolation_mode": ExtrapolationMode.SERVER_WEIGHTED.value,
1286+
"extrapolation_mode": ExtrapolationMode.SERVER_WEIGHTED.name.lower(),
12871287
}
12881288
],
12891289
}
@@ -1296,3 +1296,73 @@ def test_invalid_extrapolation_mode_update(self) -> None:
12961296
expected_message="Invalid extrapolation mode for this detector type.",
12971297
):
12981298
update_validator.save()
1299+
1300+
def test_nonexistent_extrapolation_mode_create(self) -> None:
1301+
data = {
1302+
**self.valid_data,
1303+
"dataSources": [
1304+
{
1305+
"queryType": SnubaQuery.Type.PERFORMANCE.value,
1306+
"dataset": Dataset.EventsAnalyticsPlatform.value,
1307+
"query": "test query",
1308+
"aggregate": "count()",
1309+
"timeWindow": 3600,
1310+
"environment": self.environment.name,
1311+
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
1312+
"extrapolation_mode": "blah",
1313+
},
1314+
],
1315+
}
1316+
1317+
validator = MetricIssueDetectorValidator(data=data, context=self.context)
1318+
assert not validator.is_valid(), validator.errors
1319+
assert (
1320+
validator.errors["dataSources"]["extrapolationMode"][0]
1321+
== "Invalid extrapolation mode: blah"
1322+
)
1323+
1324+
def test_nonexistent_extrapolation_mode_update(self) -> None:
1325+
data = {
1326+
**self.valid_data,
1327+
"dataSources": [
1328+
{
1329+
"queryType": SnubaQuery.Type.PERFORMANCE.value,
1330+
"dataset": Dataset.EventsAnalyticsPlatform.value,
1331+
"query": "test query",
1332+
"aggregate": "count()",
1333+
"timeWindow": 3600,
1334+
"environment": self.environment.name,
1335+
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
1336+
"extrapolation_mode": ExtrapolationMode.CLIENT_AND_SERVER_WEIGHTED.name.lower(),
1337+
},
1338+
],
1339+
}
1340+
1341+
validator = MetricIssueDetectorValidator(data=data, context=self.context)
1342+
assert validator.is_valid(), validator.errors
1343+
1344+
detector = validator.save()
1345+
1346+
update_data = {
1347+
"dataSources": [
1348+
{
1349+
"queryType": SnubaQuery.Type.PERFORMANCE.value,
1350+
"dataset": Dataset.EventsAnalyticsPlatform.value,
1351+
"query": "updated query",
1352+
"aggregate": "count()",
1353+
"timeWindow": 3600,
1354+
"environment": self.environment.name,
1355+
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
1356+
"extrapolation_mode": "blah",
1357+
}
1358+
],
1359+
}
1360+
update_validator = MetricIssueDetectorValidator(
1361+
instance=detector, data=update_data, context=self.context, partial=True
1362+
)
1363+
1364+
assert not update_validator.is_valid(), update_validator.errors
1365+
assert (
1366+
update_validator.errors["dataSources"]["extrapolationMode"][0]
1367+
== "Invalid extrapolation mode: blah"
1368+
)

0 commit comments

Comments
 (0)