Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,10 @@ def update_alert_rule(
partial=True,
)
if validator.is_valid():
if data.get("dataset") == Dataset.EventsAnalyticsPlatform.value:
if (
data.get("dataset", alert_rule.snuba_query.dataset)
== Dataset.EventsAnalyticsPlatform.value
):
if data.get("extrapolation_mode"):
old_extrapolation_mode = ExtrapolationMode(
alert_rule.snuba_query.extrapolation_mode
Expand Down
30 changes: 26 additions & 4 deletions src/sentry/incidents/metric_issue_detector.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ def is_invalid_extrapolation_mode(old_extrapolation_mode, new_extrapolation_mode
new_extrapolation_mode = ExtrapolationMode(new_extrapolation_mode).name.lower()
if type(old_extrapolation_mode) is int:
old_extrapolation_mode = ExtrapolationMode(old_extrapolation_mode).name.lower()
if new_extrapolation_mode not in [name for name, value in ExtrapolationMode.as_text_choices()]:
return True
if (
new_extrapolation_mode == ExtrapolationMode.SERVER_WEIGHTED.name.lower()
and old_extrapolation_mode != ExtrapolationMode.SERVER_WEIGHTED.name.lower()
Expand All @@ -169,6 +171,20 @@ def is_invalid_extrapolation_mode(old_extrapolation_mode, new_extrapolation_mode
return False


def format_extrapolation_mode(extrapolation_mode) -> ExtrapolationMode:
if type(extrapolation_mode) is int:
return extrapolation_mode
extrapolation_mode_options = ExtrapolationMode.as_text_choices()
numerical_extrapolation_mode = None
for name, value in extrapolation_mode_options:
if name == extrapolation_mode:
numerical_extrapolation_mode = value
break
if type(numerical_extrapolation_mode) is not int:
return None
return numerical_extrapolation_mode


class MetricIssueDetectorValidator(BaseDetectorTypeValidator):
data_sources = serializers.ListField(
child=SnubaQueryValidator(timeWindowSeconds=True), required=False
Expand Down Expand Up @@ -197,7 +213,11 @@ def _validate_transaction_dataset_deprecation(self, dataset: Dataset) -> None:
)

def _validate_extrapolation_mode(self, extrapolation_mode: str) -> None:
if extrapolation_mode == ExtrapolationMode.SERVER_WEIGHTED.value:
if extrapolation_mode is not None and extrapolation_mode not in [
Copy link
Member

Choose a reason for hiding this comment

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

This will also be simpler if you return the enum from the validator

name for name, value in ExtrapolationMode.as_text_choices()
]:
raise serializers.ValidationError(f"Invalid extrapolation mode: {extrapolation_mode}")
if extrapolation_mode == ExtrapolationMode.SERVER_WEIGHTED.name.lower():
raise serializers.ValidationError(
"server_weighted extrapolation mode is not supported for new detectors."
)
Expand Down Expand Up @@ -288,6 +308,10 @@ def update_data_source(
"Failed to send data to Seer, cannot update detector"
)

numerical_extrapolation_mode = format_extrapolation_mode(
Copy link
Member

Choose a reason for hiding this comment

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

If you return an enum, you can instead do:

numerical_extrapolation_mode = data_source.get("extrapolation_mode", snuba_query.extrapolation_mode.value)

data_source.get("extrapolation_mode", snuba_query.extrapolation_mode)
)

update_snuba_query(
snuba_query=snuba_query,
query_type=data_source.get("query_type", snuba_query.type),
Expand All @@ -298,9 +322,7 @@ def update_data_source(
resolution=timedelta(seconds=data_source.get("resolution", snuba_query.resolution)),
environment=data_source.get("environment", snuba_query.environment),
event_types=data_source.get("event_types", [event_type for event_type in event_types]),
extrapolation_mode=data_source.get(
"extrapolation_mode", snuba_query.extrapolation_mode
),
extrapolation_mode=numerical_extrapolation_mode,
)

def update_anomaly_detection(self, instance: Detector, validated_data: dict[str, Any]) -> bool:
Expand Down
12 changes: 10 additions & 2 deletions src/sentry/snuba/snuba_query_validator.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class SnubaQueryValidator(BaseDataSourceValidator[QuerySubscription]):
required=False,
allow_empty=False,
)
extrapolation_mode = serializers.IntegerField(required=False, allow_null=True)
extrapolation_mode = serializers.CharField(required=False, allow_null=True)

class Meta:
model = QuerySubscription
Expand Down Expand Up @@ -408,7 +408,15 @@ def _validate_group_by(self, value: Sequence[str] | None) -> Sequence[str] | Non
def create_source(self, validated_data) -> QuerySubscription:
extrapolation_mode = validated_data.get("extrapolation_mode")
if extrapolation_mode is not None:
extrapolation_mode = ExtrapolationMode(extrapolation_mode)
extrapolation_mode_options = ExtrapolationMode.as_text_choices()
for name, value in extrapolation_mode_options:
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this, you can add a classmethod from_str(self, name: str) function to ExtrapolationMode. We do that for some other enums. Then you can just do ExtrapolationMode.from_str(extrapolation_mode)

if name == extrapolation_mode:
extrapolation_mode = ExtrapolationMode(value)
break
if type(extrapolation_mode) is not ExtrapolationMode:
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a validate_extrapolation_mode() function to this validator that will transform the value into the enum class automatically? That way you won't need to do it here

raise serializers.ValidationError(
f"Invalid extrapolation mode: {extrapolation_mode}"
)
snuba_query = create_snuba_query(
query_type=validated_data["query_type"],
dataset=validated_data["dataset"],
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/snuba/subscriptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ def update_snuba_query(
extrapolation_mode=(
extrapolation_mode
if extrapolation_mode is not None
else ExtrapolationMode.UNKNOWN.value
else snuba_query.extrapolation_mode
),
)
if new_event_types:
Expand Down
77 changes: 74 additions & 3 deletions tests/sentry/incidents/endpoints/validators/test_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -1238,7 +1238,7 @@ def test_invalid_extrapolation_mode_create(self) -> None:
"timeWindow": 3600,
"environment": self.environment.name,
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
"extrapolation_mode": ExtrapolationMode.SERVER_WEIGHTED.value,
"extrapolation_mode": ExtrapolationMode.SERVER_WEIGHTED.name.lower(),
},
],
}
Expand All @@ -1263,7 +1263,7 @@ def test_invalid_extrapolation_mode_update(self) -> None:
"timeWindow": 3600,
"environment": self.environment.name,
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
"extrapolation_mode": ExtrapolationMode.CLIENT_AND_SERVER_WEIGHTED.value,
"extrapolation_mode": ExtrapolationMode.CLIENT_AND_SERVER_WEIGHTED.name.lower(),
},
],
}
Expand All @@ -1283,7 +1283,78 @@ def test_invalid_extrapolation_mode_update(self) -> None:
"timeWindow": 3600,
"environment": self.environment.name,
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
"extrapolation_mode": ExtrapolationMode.SERVER_WEIGHTED.value,
"extrapolation_mode": ExtrapolationMode.SERVER_WEIGHTED.name.lower(),
}
],
}
update_validator = MetricIssueDetectorValidator(
instance=detector, data=update_data, context=self.context, partial=True
)
assert update_validator.is_valid(), update_validator.errors
with self.assertRaisesMessage(
ValidationError,
expected_message="Invalid extrapolation mode for this detector type.",
):
update_validator.save()

def test_nonexistent_extrapolation_mode_create(self) -> None:
data = {
**self.valid_data,
"dataSources": [
{
"queryType": SnubaQuery.Type.PERFORMANCE.value,
"dataset": Dataset.EventsAnalyticsPlatform.value,
"query": "test query",
"aggregate": "count()",
"timeWindow": 3600,
"environment": self.environment.name,
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
"extrapolation_mode": "blah",
},
],
}

validator = MetricIssueDetectorValidator(data=data, context=self.context)
assert validator.is_valid(), validator.errors
with self.assertRaisesMessage(
ValidationError,
expected_message="Invalid extrapolation mode: blah",
):
validator.save()

def test_nonexistent_extrapolation_mode_update(self) -> None:
data = {
**self.valid_data,
"dataSources": [
{
"queryType": SnubaQuery.Type.PERFORMANCE.value,
"dataset": Dataset.EventsAnalyticsPlatform.value,
"query": "test query",
"aggregate": "count()",
"timeWindow": 3600,
"environment": self.environment.name,
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
"extrapolation_mode": ExtrapolationMode.CLIENT_AND_SERVER_WEIGHTED.name.lower(),
},
],
}

validator = MetricIssueDetectorValidator(data=data, context=self.context)
assert validator.is_valid(), validator.errors

detector = validator.save()

update_data = {
"dataSources": [
{
"queryType": SnubaQuery.Type.PERFORMANCE.value,
"dataset": Dataset.EventsAnalyticsPlatform.value,
"query": "updated query",
"aggregate": "count()",
"timeWindow": 3600,
"environment": self.environment.name,
"eventTypes": [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN.name.lower()],
"extrapolation_mode": "blah",
}
],
}
Expand Down
Loading