-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(incidents): compute resolution correctly in metric issue detector #112623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,11 @@ | |
|
|
||
| from sentry import features, quotas | ||
| from sentry.constants import ObjectStatus | ||
| from sentry.incidents.logic import enable_disable_subscriptions | ||
| from sentry.incidents.logic import ( | ||
| DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER, | ||
| enable_disable_subscriptions, | ||
| get_alert_resolution, | ||
| ) | ||
| from sentry.incidents.models.alert_rule import AlertRuleDetectionType | ||
| from sentry.relay.config.metric_extraction import on_demand_metrics_feature_flags | ||
| from sentry.search.eap.trace_metrics.validator import validate_trace_metrics_aggregate | ||
|
|
@@ -245,6 +249,27 @@ | |
| f"{extrapolation_mode.name.lower()} extrapolation mode is not supported for new detectors. Allowed modes are: client_and_server_weighted, unknown." | ||
| ) | ||
|
|
||
| def _get_resolution_for_window( | ||
| self, | ||
| time_window_seconds: int | float, | ||
| detection_type: str | None, | ||
| comparison_delta: int | float | None, | ||
| ) -> timedelta: | ||
| """ | ||
| Compute the appropriate SnubaQuery resolution for a given time window | ||
| (in seconds), mirroring the logic in create_alert_rule / update_alert_rule. | ||
| """ | ||
| organization = self.context["organization"] | ||
|
|
||
| if detection_type == AlertRuleDetectionType.DYNAMIC: | ||
|
Check failure on line 264 in src/sentry/incidents/metric_issue_detector.py
|
||
|
kcons marked this conversation as resolved.
|
||
| resolution = timedelta(seconds=time_window_seconds) | ||
| else: | ||
| resolution = get_alert_resolution(int(time_window_seconds) // 60, organization) | ||
| if comparison_delta is not None: | ||
| resolution *= DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER | ||
|
|
||
| return resolution | ||
|
|
||
| def get_quota(self) -> DetectorQuota: | ||
| organization = self.context.get("organization") | ||
| request = self.context.get("request") | ||
|
|
@@ -295,7 +320,10 @@ | |
| return False | ||
|
|
||
| def update_data_source( | ||
| self, instance: Detector, data_source: SnubaQueryDataSourceType, seer_updated: bool = False | ||
| self, | ||
| instance: Detector, | ||
| data_source: SnubaQueryDataSourceType, | ||
| seer_updated: bool = False, | ||
| ) -> None: | ||
| try: | ||
| source_instance = DataSource.objects.get(detector=instance) | ||
|
|
@@ -312,7 +340,7 @@ | |
| except SnubaQuery.DoesNotExist: | ||
| raise serializers.ValidationError("SnubaQuery not found, can't update") | ||
|
|
||
| event_types = SnubaQueryEventType.objects.filter(snuba_query_id=snuba_query.id) | ||
| event_types = snuba_query.event_types | ||
|
|
||
| if self.is_editing_transaction_dataset(snuba_query, data_source): | ||
| raise serializers.ValidationError( | ||
|
|
@@ -342,16 +370,23 @@ | |
| data_source.get("extrapolation_mode", snuba_query.extrapolation_mode) | ||
| ) | ||
|
|
||
| new_time_window = data_source.get("time_window", snuba_query.time_window) | ||
| resolution = self._get_resolution_for_window( | ||
| new_time_window, | ||
| instance.config.get("detection_type"), | ||
| instance.config.get("comparison_delta"), | ||
| ) | ||
|
|
||
| update_snuba_query( | ||
| snuba_query=snuba_query, | ||
| query_type=data_source.get("query_type", snuba_query.type), | ||
| dataset=data_source.get("dataset", snuba_query.dataset), | ||
| query_type=data_source.get("query_type", SnubaQuery.Type(snuba_query.type)), | ||
| dataset=data_source.get("dataset", Dataset(snuba_query.dataset)), | ||
|
Comment on lines
+382
to
+383
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. <3 |
||
| query=data_source.get("query", snuba_query.query), | ||
| aggregate=data_source.get("aggregate", snuba_query.aggregate), | ||
| time_window=timedelta(seconds=data_source.get("time_window", snuba_query.time_window)), | ||
| resolution=timedelta(seconds=data_source.get("resolution", snuba_query.resolution)), | ||
| time_window=timedelta(seconds=new_time_window), | ||
| resolution=resolution, | ||
| environment=data_source.get("environment", snuba_query.environment), | ||
| event_types=data_source.get("event_types", [event_type for event_type in event_types]), | ||
| event_types=data_source.get("event_types", event_types), | ||
| extrapolation_mode=extrapolation_mode, | ||
| ) | ||
|
|
||
|
|
@@ -413,6 +448,10 @@ | |
|
|
||
| if data_source is not None: | ||
| self.update_data_source(instance, data_source, seer_updated) | ||
| elif "config" in validated_data: | ||
| # Config changed (e.g. detection_type or comparison_delta) without a | ||
| # data_source update — recalculate resolution to match the new config. | ||
| self.update_data_source(instance, {}, seer_updated) | ||
|
|
||
| instance.save() | ||
|
|
||
|
|
@@ -421,10 +460,20 @@ | |
|
|
||
| def create(self, validated_data: dict[str, Any]) -> Detector: | ||
| if "data_sources" in validated_data: | ||
| config = validated_data.get("config", {}) | ||
| detection_type = config.get("detection_type") | ||
| comparison_delta = config.get("comparison_delta") | ||
|
|
||
| for validated_data_source in validated_data["data_sources"]: | ||
| self._validate_transaction_dataset_deprecation(validated_data_source.get("dataset")) | ||
| self._validate_extrapolation_mode(validated_data_source.get("extrapolation_mode")) | ||
|
|
||
| time_window = validated_data_source.get("time_window") | ||
| if time_window is not None: | ||
| validated_data_source["resolution"] = self._get_resolution_for_window( | ||
| time_window, detection_type, comparison_delta | ||
| ) | ||
|
|
||
| detector = super().create(validated_data) | ||
|
|
||
| if detector.config.get("detection_type") == AlertRuleDetectionType.DYNAMIC.value: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -483,7 +483,7 @@ def create_source(self, validated_data: dict[str, Any]) -> QuerySubscription: | |
| query=validated_data["query"], | ||
| aggregate=validated_data["aggregate"], | ||
| time_window=timedelta(seconds=validated_data["time_window"]), | ||
| resolution=timedelta(minutes=1), | ||
| resolution=validated_data.get("resolution", timedelta(minutes=1)), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👀 - should we do anything to see if the existing metric alerts have the correct resolutions?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory, but by my count there are fewer than 87 potentially impacted cases so far in US (and probably far less than that), so I'm not entirely sure it's worth pursuing. This is more of a thing where we want to close the door before we let in the crowds. |
||
| environment=validated_data["environment"], | ||
| event_types=validated_data["event_types"], | ||
| group_by=validated_data.get("group_by"), | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.