refs(metric_alerts): Stop using trigger level `resolve_threshold` and `threshold_type` fields (#19961)

We now use only the rule level versions of `resolve_threhsold` and `threshold_type`. We can stop
writing these and using them in the codebase to prepare to remove.

migrations_lockfile.txt

@@ -10,7 +10,7 @@ auth: 0008_alter_user_username_max_length
 contenttypes: 0002_remove_content_type_name
 jira_ac: 0001_initial
 nodestore: 0001_initial
-sentry: 0091_alertruleactivity
+sentry: 0092_remove_trigger_threshold_type_nullable
 sessions: 0001_initial
 sites: 0002_alter_domain_unique
 social_auth: 0001_initial

src/sentry/api/serializers/models/alert_rule_trigger.py

@@ -11,11 +11,13 @@ from sentry.incidents.models import (
     AlertRuleTriggerExclusion,
 )
 from sentry.utils.compat import zip
+from sentry.utils.db import attach_foreignkey
 
 
 @register(AlertRuleTrigger)
 class AlertRuleTriggerSerializer(Serializer):
     def get_attrs(self, item_list, user, **kwargs):
+        attach_foreignkey(item_list, AlertRuleTrigger.alert_rule)
 
         triggers = {item.id: item for item in item_list}
         result = defaultdict(dict)
@@ -37,9 +39,9 @@ class AlertRuleTriggerSerializer(Serializer):
             "id": six.text_type(obj.id),
             "alertRuleId": six.text_type(obj.alert_rule_id),
             "label": obj.label,
-            "thresholdType": obj.threshold_type,
+            "thresholdType": obj.alert_rule.threshold_type,
             "alertThreshold": obj.alert_threshold,
-            "resolveThreshold": obj.resolve_threshold,
+            "resolveThreshold": obj.alert_rule.resolve_threshold,
             "dateCreated": obj.date_added,
             "actions": attrs.get("actions", []),
         }

src/sentry/incidents/action_handlers.py

@@ -163,7 +163,7 @@ def generate_incident_trigger_email_context(project, incident, alert_rule_trigge
     alert_rule = trigger.alert_rule
     snuba_query = alert_rule.snuba_query
     is_active = status == TriggerStatus.ACTIVE
-    is_threshold_type_above = trigger.threshold_type == AlertRuleThresholdType.ABOVE.value
+    is_threshold_type_above = alert_rule.threshold_type == AlertRuleThresholdType.ABOVE.value
 
     # if alert threshold and threshold type is above then show '>'
     # if resolve threshold and threshold type is *BELOW* then show '>'
@@ -198,7 +198,7 @@ def generate_incident_trigger_email_context(project, incident, alert_rule_trigge
         "triggered_at": incident_trigger.date_added,
         "aggregate": aggregate,
         "query": snuba_query.query,
-        "threshold": trigger.alert_threshold if is_active else trigger.resolve_threshold,
+        "threshold": trigger.alert_threshold if is_active else alert_rule.resolve_threshold,
         # if alert threshold and threshold type is above then show '>'
         # if resolve threshold and threshold type is *BELOW* then show '>'
         "threshold_direction_string": ">" if show_greater_than_string else "<",

src/sentry/incidents/endpoints/serializers.py

@@ -184,28 +184,8 @@ class AlertRuleTriggerSerializer(CamelSnakeModelSerializer):
 
     class Meta:
         model = AlertRuleTrigger
-        fields = [
-            "id",
-            "label",
-            "threshold_type",
-            "alert_threshold",
-            "resolve_threshold",
-            "excluded_projects",
-            "actions",
-        ]
-        extra_kwargs = {
-            "label": {"min_length": 1, "max_length": 64},
-            "threshold_type": {"required": False},
-        }
-
-    def validate_threshold_type(self, threshold_type):
-        try:
-            return AlertRuleThresholdType(threshold_type)
-        except ValueError:
-            raise serializers.ValidationError(
-                "Invalid threshold type, valid values are %s"
-                % [item.value for item in AlertRuleThresholdType]
-            )
+        fields = ["id", "label", "alert_threshold", "excluded_projects", "actions"]
+        extra_kwargs = {"label": {"min_length": 1, "max_length": 64}}
 
     def create(self, validated_data):
         try:
@@ -314,7 +294,7 @@ class AlertRuleSerializer(CamelSnakeModelSerializer):
         extra_kwargs = {
             "name": {"min_length": 1, "max_length": 64},
             "include_all_projects": {"default": False},
-            "threshold_type": {"required": False},
+            "threshold_type": {"required": True},
             "resolve_threshold": {"required": False},
         }
 
@@ -412,42 +392,12 @@ class AlertRuleSerializer(CamelSnakeModelSerializer):
                     'Trigger {} must be labeled "{}"'.format(i + 1, expected_label)
                 )
         critical = triggers[0]
-        if "threshold_type" in data:
-            threshold_type = data["threshold_type"]
-            for trigger in triggers:
-                trigger["threshold_type"] = threshold_type.value
-        else:
-            data["threshold_type"] = threshold_type = AlertRuleThresholdType(
-                critical.get("threshold_type", AlertRuleThresholdType.ABOVE.value)
-            )
-
-        if "resolve_threshold" in data:
-            for trigger in triggers:
-                trigger["resolve_threshold"] = data["resolve_threshold"]
-        else:
-            trigger_resolve_thresholds = [
-                trigger["resolve_threshold"]
-                for trigger in triggers
-                if trigger.get("resolve_threshold")
-            ]
-            if trigger_resolve_thresholds:
-                data["resolve_threshold"] = (
-                    min(trigger_resolve_thresholds)
-                    if threshold_type == AlertRuleThresholdType.ABOVE
-                    else max(trigger_resolve_thresholds)
-                )
-            else:
-                data["resolve_threshold"] = None
+        threshold_type = data["threshold_type"]
 
         self._validate_trigger_thresholds(threshold_type, critical, data.get("resolve_threshold"))
 
         if len(triggers) == 2:
             warning = triggers[1]
-            if critical["threshold_type"] != warning["threshold_type"]:
-                raise serializers.ValidationError(
-                    "Must have matching threshold types (i.e. critical and warning "
-                    "triggers must both be an upper or lower bound)"
-                )
             self._validate_trigger_thresholds(
                 threshold_type, warning, data.get("resolve_threshold")
             )
@@ -456,9 +406,6 @@ class AlertRuleSerializer(CamelSnakeModelSerializer):
         return data
 
     def _validate_trigger_thresholds(self, threshold_type, trigger, resolve_threshold):
-        resolve_threshold = (
-            resolve_threshold if resolve_threshold is not None else trigger.get("resolve_threshold")
-        )
         if resolve_threshold is None:
             return
         # Since we're comparing non-inclusive thresholds here (>, <), we need
@@ -490,11 +437,6 @@ class AlertRuleSerializer(CamelSnakeModelSerializer):
                     threshold_type
                 )
             )
-        elif alert_op(critical["resolve_threshold"], warning["resolve_threshold"]):
-            raise serializers.ValidationError(
-                "Critical trigger must have a resolution threshold {} (or equal to) "
-                "warning trigger".format(threshold_type)
-            )
 
     def create(self, validated_data):
         try:

src/sentry/incidents/logic.py

@@ -52,6 +52,7 @@ from sentry.shared_integrations.exceptions import DuplicateDisplayNameError
 # It attempts to center the start of the incident, only showing earlier data if there isn't enough time
 # after the incident started to display the correct start date.
 WINDOWED_STATS_DATA_POINTS = 200
+NOT_SET = object()
 
 
 class AlreadyDeletedError(Exception):
@@ -657,7 +658,7 @@ def update_alert_rule(
     environment=None,
     threshold_type=None,
     threshold_period=None,
-    resolve_threshold=None,
+    resolve_threshold=NOT_SET,
     include_all_projects=None,
     excluded_projects=None,
 ):
@@ -704,7 +705,7 @@ def update_alert_rule(
         updated_query_fields["time_window"] = timedelta(minutes=time_window)
     if threshold_type:
         updated_fields["threshold_type"] = threshold_type.value
-    if resolve_threshold:
+    if resolve_threshold is not NOT_SET:
         updated_fields["resolve_threshold"] = resolve_threshold
     if threshold_period:
         updated_fields["threshold_period"] = threshold_period
@@ -867,23 +868,13 @@ class ProjectsNotAssociatedWithAlertRuleError(Exception):
         self.project_slugs = project_slugs
 
 
-def create_alert_rule_trigger(
-    alert_rule,
-    label,
-    threshold_type,
-    alert_threshold,
-    resolve_threshold=None,
-    excluded_projects=None,
-):
+def create_alert_rule_trigger(alert_rule, label, alert_threshold, excluded_projects=None):
     """
     Creates a new AlertRuleTrigger
     :param alert_rule: The alert rule to create the trigger for
     :param label: A description of the trigger
-    :param threshold_type: An AlertRuleThresholdType
     :param alert_threshold: Value that the subscription needs to reach to trigger the
     alert rule
-    :param resolve_threshold: Optional value that the subscription needs to reach to
-    resolve the alert
     :param excluded_projects: A list of Projects that should be excluded from this
     trigger. These projects must be associate with the alert rule already
     :return: The created AlertRuleTrigger
@@ -897,11 +888,7 @@ def create_alert_rule_trigger(
 
     with transaction.atomic():
         trigger = AlertRuleTrigger.objects.create(
-            alert_rule=alert_rule,
-            label=label,
-            threshold_type=threshold_type.value,
-            alert_threshold=alert_threshold,
-            resolve_threshold=resolve_threshold,
+            alert_rule=alert_rule, label=label, alert_threshold=alert_threshold
         )
         if excluded_subs:
             new_exclusions = [
@@ -913,22 +900,12 @@ def create_alert_rule_trigger(
     return trigger
 
 
-def update_alert_rule_trigger(
-    trigger,
-    label=None,
-    threshold_type=None,
-    alert_threshold=None,
-    resolve_threshold=None,
-    excluded_projects=None,
-):
+def update_alert_rule_trigger(trigger, label=None, alert_threshold=None, excluded_projects=None):
     """
     :param trigger: The AlertRuleTrigger to update
     :param label: A description of the trigger
-    :param threshold_type: An AlertRuleThresholdType
     :param alert_threshold: Value that the subscription needs to reach to trigger the
     alert rule
-    :param resolve_threshold: Optional value that the subscription needs to reach to
-    resolve the alert
     :param excluded_projects: A list of Projects that should be excluded from this
     trigger. These projects must be associate with the alert rule already
     :return: The updated AlertRuleTrigger
@@ -944,12 +921,8 @@ def update_alert_rule_trigger(
     updated_fields = {}
     if label is not None:
         updated_fields["label"] = label
-    if threshold_type is not None:
-        updated_fields["threshold_type"] = threshold_type.value
     if alert_threshold is not None:
         updated_fields["alert_threshold"] = alert_threshold
-    # We set resolve_threshold to None as a 'reset', in case it was previously a value and we're removing it here.
-    updated_fields["resolve_threshold"] = resolve_threshold
 
     deleted_exclusion_ids = []
     new_subs = []

src/sentry/incidents/models.py

@@ -483,7 +483,7 @@ class AlertRuleTrigger(Model):
 
     alert_rule = FlexibleForeignKey("sentry.AlertRule")
     label = models.TextField()
-    threshold_type = models.SmallIntegerField()
+    threshold_type = models.SmallIntegerField(null=True)
     alert_threshold = models.FloatField()
     resolve_threshold = models.FloatField(null=True)
     triggered_incidents = models.ManyToManyField(

src/sentry/migrations/0090_fix_auditlog_pickled_data_take_2.py

@@ -44,8 +44,6 @@ class Migration(migrations.Migration):
     # want to create an index concurrently when adding one to an existing table.
     atomic = False
 
-    dependencies = [
-        ("sentry", "0089_rule_level_fields_backfill"),
-    ]
+    dependencies = [("sentry", "0089_rule_level_fields_backfill")]
 
     operations = [migrations.RunPython(code=cleanup_audit_log_data)]

src/sentry/migrations/0092_remove_trigger_threshold_type_nullable.py

@@ -0,0 +1,37 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.29 on 2020-07-24 01:25
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    # This flag is used to mark that a migration shouldn't be automatically run in
+    # production. We set this to True for operations that we think are risky and want
+    # someone from ops to run manually and monitor.
+    # General advice is that if in doubt, mark your migration as `is_dangerous`.
+    # Some things you should always mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that
+    #   they can be monitored. Since data migrations will now hold a transaction open
+    #   this is even more important.
+    # - Adding columns to highly active tables, even ones that are NULL.
+    is_dangerous = False
+
+    # This flag is used to decide whether to run this migration in a transaction or not.
+    # By default we prefer to run in a transaction, but for migrations where you want
+    # to `CREATE INDEX CONCURRENTLY` this needs to be set to False. Typically you'll
+    # want to create an index concurrently when adding one to an existing table.
+    atomic = True
+
+
+    dependencies = [
+        ('sentry', '0091_alertruleactivity'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='alertruletrigger',
+            name='threshold_type',
+            field=models.SmallIntegerField(null=True),
+        ),
+    ]

src/sentry/testutils/factories.py

@@ -895,19 +895,11 @@ class Factories(object):
         return alert_rule
 
     @staticmethod
-    def create_alert_rule_trigger(
-        alert_rule,
-        label=None,
-        threshold_type=AlertRuleThresholdType.ABOVE,
-        alert_threshold=100,
-        resolve_threshold=10,
-    ):
+    def create_alert_rule_trigger(alert_rule, label=None, alert_threshold=100):
         if not label:
             label = petname.Generate(2, " ", letters=10).title()
 
-        return create_alert_rule_trigger(
-            alert_rule, label, threshold_type, alert_threshold, resolve_threshold
-        )
+        return create_alert_rule_trigger(alert_rule, label, alert_threshold)
 
     @staticmethod
     def create_incident_trigger(incident, alert_rule_trigger, status=None):
@@ -915,7 +907,7 @@ class Factories(object):
             status = TriggerStatus.ACTIVE.value
 
         return IncidentTrigger.objects.create(
-            alert_rule_trigger=alert_rule_trigger, incident=incident, status=status,
+            alert_rule_trigger=alert_rule_trigger, incident=incident, status=status
         )
 
     @staticmethod

tests/sentry/api/serializers/test_alert_rule.py

@@ -86,7 +86,7 @@ class AlertRuleSerializerTest(BaseAlertRuleSerializerTest, TestCase):
     def test_triggers(self):
         alert_rule = self.create_alert_rule()
         other_alert_rule = self.create_alert_rule()
-        trigger = create_alert_rule_trigger(alert_rule, "test", AlertRuleThresholdType.ABOVE, 1000)
+        trigger = create_alert_rule_trigger(alert_rule, "test", 1000)
         result = serialize([alert_rule, other_alert_rule])
         assert result[0]["triggers"] == [serialize(trigger)]
         assert result[1]["triggers"] == []
@@ -126,7 +126,7 @@ class DetailedAlertRuleSerializerTest(BaseAlertRuleSerializerTest, TestCase):
     def test_triggers(self):
         alert_rule = self.create_alert_rule()
         other_alert_rule = self.create_alert_rule()
-        trigger = create_alert_rule_trigger(alert_rule, "test", AlertRuleThresholdType.ABOVE, 1000)
+        trigger = create_alert_rule_trigger(alert_rule, "test", 1000)
         result = serialize([alert_rule, other_alert_rule], serializer=DetailedAlertRuleSerializer())
         assert result[0]["triggers"] == [serialize(trigger)]
         assert result[1]["triggers"] == []