chore: Remove unused threshold_type, alert_threshold and resolve_threshold fields from AlertRule (SEN-1065)

We moved these fields to AlertRuleTrigger a while back, just cleaning up the leftover code around
this.

Note that the migration runs no sql. It just updates the database state to reflect that the fields
are gone. I will follow this up with a separate PR to actually delete the columns once this is
deployed.

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

@@ -30,15 +30,18 @@ class AlertRuleSerializer(Serializer):
             "name": obj.name,
             "organizationId": six.text_type(obj.organization_id),
             "status": obj.status,
-            "thresholdType": obj.threshold_type,
+            # TODO: Remove when frontend isn't using
+            "thresholdType": 0,
             "dataset": obj.dataset,
             "query": obj.query,
             "aggregation": obj.aggregation,
             "aggregations": [obj.aggregation],
             "timeWindow": obj.time_window,
             "resolution": obj.resolution,
-            "alertThreshold": obj.alert_threshold,
-            "resolveThreshold": obj.resolve_threshold,
+            # TODO: Remove when frontend isn't using
+            "alertThreshold": 0,
+            # TODO: Remove when frontend isn't using
+            "resolveThreshold": 0,
             "thresholdPeriod": obj.threshold_period,
             "triggers": attrs.get("triggers", []),
             "includeAllProjects": obj.include_all_projects,

src/sentry/incidents/endpoints/serializers.py

@@ -42,6 +42,9 @@ class AlertRuleSerializer(CamelSnakeModelSerializer):
     # individually. If we find this to be a problem then we can look into batching.
     projects = serializers.ListField(child=ProjectField(), required=False)
     excluded_projects = serializers.ListField(child=ProjectField(), required=False)
+    threshold_type = serializers.IntegerField(required=False)
+    alert_threshold = serializers.IntegerField(required=False)
+    resolve_threshold = serializers.IntegerField(required=False)
 
     class Meta:
         model = AlertRule
@@ -72,15 +75,6 @@ class AlertRuleSerializer(CamelSnakeModelSerializer):
             "include_all_projects": {"default": 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]
-            )
-
     def validate_aggregation(self, aggregation):
         try:
             return QueryAggregations(aggregation)
@@ -101,7 +95,7 @@ class AlertRuleSerializer(CamelSnakeModelSerializer):
             )
 
     def validate(self, attrs):
-        return self._handle_aggregations_transition(attrs)
+        return self._handle_old_fields_transition(attrs)
 
     def create(self, validated_data):
         try:
@@ -136,13 +130,17 @@ class AlertRuleSerializer(CamelSnakeModelSerializer):
                 validated_data.pop(field_name)
         return validated_data
 
-    def _handle_aggregations_transition(self, validated_data):
+    def _handle_old_fields_transition(self, validated_data):
         # Temporary methods for transitioning from multiple aggregations to a single
         # aggregate
         if "aggregations" in validated_data and "aggregation" not in validated_data:
             validated_data["aggregation"] = validated_data["aggregations"][0]
 
         validated_data.pop("aggregations", None)
+        # TODO: Remove after frontend stops using these fields
+        validated_data.pop("threshold_type", None)
+        validated_data.pop("alert_threshold", None)
+        validated_data.pop("resolve_threshold", None)
         return validated_data
 
     def update(self, instance, validated_data):

src/sentry/incidents/logic.py

@@ -582,12 +582,9 @@ def create_alert_rule(
     organization,
     projects,
     name,
-    threshold_type,
     query,
     aggregation,
     time_window,
-    alert_threshold,
-    resolve_threshold,
     threshold_period,
     include_all_projects=False,
     excluded_projects=None,
@@ -600,14 +597,9 @@ def create_alert_rule(
     if `include_all_projects` is True
     :param name: Name for the alert rule. This will be used as part of the
     incident name, and must be unique per project
-    :param threshold_type: An AlertRuleThresholdType
     :param query: An event search query to subscribe to and monitor for alerts
     :param aggregation: A QueryAggregation to fetch for this alert rule
     :param time_window: Time period to aggregate over, in minutes
-    :param alert_threshold: Value that the subscription needs to reach to
-    trigger the alert
-    :param resolve_threshold: Value that the subscription needs to reach to
-    resolve the alert
     :param threshold_period: How many update periods the value of the
     subscription needs to exceed the threshold before triggering
     :param include_all_projects: Whether to include all current and future projects
@@ -625,14 +617,11 @@ def create_alert_rule(
         alert_rule = AlertRule.objects.create(
             organization=organization,
             name=name,
-            threshold_type=threshold_type.value,
             dataset=dataset.value,
             query=query,
             aggregation=aggregation.value,
             time_window=time_window,
             resolution=resolution,
-            alert_threshold=alert_threshold,
-            resolve_threshold=resolve_threshold,
             threshold_period=threshold_period,
             include_all_projects=include_all_projects,
         )
@@ -655,12 +644,9 @@ def update_alert_rule(
     alert_rule,
     projects=None,
     name=None,
-    threshold_type=None,
     query=None,
     aggregation=None,
     time_window=None,
-    alert_threshold=None,
-    resolve_threshold=None,
     threshold_period=None,
     include_all_projects=None,
     excluded_projects=None,
@@ -673,14 +659,9 @@ def update_alert_rule(
     `include_all_projects` is True
     :param name: Name for the alert rule. This will be used as part of the
     incident name, and must be unique per project.
-    :param threshold_type: An AlertRuleThresholdType
     :param query: An event search query to subscribe to and monitor for alerts
     :param aggregation: An AlertRuleAggregation that we want to fetch for this alert rule
     :param time_window: Time period to aggregate over, in minutes.
-    :param alert_threshold: Value that the subscription needs to reach to
-    trigger the alert
-    :param resolve_threshold: Value that the subscription needs to reach to
-    resolve the alert
     :param threshold_period: How many update periods the value of the
     subscription needs to exceed the threshold before triggering
     :param include_all_projects: Whether to include all current and future projects
@@ -699,8 +680,6 @@ def update_alert_rule(
     updated_fields = {}
     if name:
         updated_fields["name"] = name
-    if threshold_type:
-        updated_fields["threshold_type"] = threshold_type.value
     if query is not None:
         validate_alert_rule_query(query)
         updated_fields["query"] = query
@@ -708,10 +687,6 @@ def update_alert_rule(
         updated_fields["aggregation"] = aggregation.value
     if time_window:
         updated_fields["time_window"] = time_window
-    if alert_threshold is not None:
-        updated_fields["alert_threshold"] = alert_threshold
-    if resolve_threshold is not None:
-        updated_fields["resolve_threshold"] = resolve_threshold
     if threshold_period:
         updated_fields["threshold_period"] = threshold_period
     if include_all_projects is not None:

src/sentry/incidents/models.py

@@ -308,9 +308,6 @@ class AlertRule(Model):
     aggregation = models.IntegerField(default=QueryAggregations.TOTAL.value)
     time_window = models.IntegerField()
     resolution = models.IntegerField()
-    threshold_type = models.SmallIntegerField(null=True)
-    alert_threshold = models.IntegerField(null=True)
-    resolve_threshold = models.IntegerField(null=True)
     threshold_period = models.IntegerField()
     date_modified = models.DateTimeField(default=timezone.now)
     date_added = models.DateTimeField(default=timezone.now)

src/sentry/migrations/0008_auto_20191030_0016.py

@@ -0,0 +1,33 @@
+# -*- coding: utf-8 -*-
+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:
+    # - Adding indexes to large tables. These indexes should be created concurrently,
+    #   unfortunately we can't run migrations outside of a transaction until Django
+    #   1.10. So until then these should be run manually.
+    # - 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
+
+    dependencies = [("sentry", "0007_auto_20191029_0131")]
+
+    operations = [
+        migrations.SeparateDatabaseAndState(
+            database_operations=[],
+            state_operations=[
+                migrations.RemoveField(model_name="alertrule", name="alert_threshold"),
+                migrations.RemoveField(model_name="alertrule", name="resolve_threshold"),
+                migrations.RemoveField(model_name="alertrule", name="threshold_type"),
+            ],
+        )
+    ]

src/sentry/testutils/factories.py

@@ -24,7 +24,6 @@ from sentry.event_manager import EventManager
 from sentry.constants import SentryAppStatus
 from sentry.incidents.logic import create_alert_rule
 from sentry.incidents.models import (
-    AlertRuleThresholdType,
     Incident,
     IncidentGroup,
     IncidentProject,
@@ -941,12 +940,9 @@ class Factories(object):
         organization,
         projects,
         name=None,
-        threshold_type=AlertRuleThresholdType.ABOVE,
         query="level:error",
         aggregation=QueryAggregations.TOTAL,
         time_window=10,
-        alert_threshold=100,
-        resolve_threshold=10,
         threshold_period=1,
         include_all_projects=False,
         excluded_projects=None,
@@ -958,12 +954,9 @@ class Factories(object):
             organization,
             projects,
             name,
-            threshold_type,
             query,
             aggregation,
             time_window,
-            alert_threshold,
-            resolve_threshold,
             threshold_period,
             include_all_projects=include_all_projects,
             excluded_projects=excluded_projects,

tests/sentry/api/serializers/test_alert_rule.py

@@ -17,14 +17,14 @@ class BaseAlertRuleSerializerTest(object):
         assert result["id"] == six.text_type(alert_rule.id)
         assert result["organizationId"] == six.text_type(alert_rule.organization_id)
         assert result["name"] == alert_rule.name
-        assert result["thresholdType"] == alert_rule.threshold_type
+        assert result["thresholdType"] == 0
         assert result["dataset"] == alert_rule.dataset
         assert result["query"] == alert_rule.query
         assert result["aggregation"] == alert_rule.aggregation
         assert result["timeWindow"] == alert_rule.time_window
         assert result["resolution"] == alert_rule.resolution
-        assert result["alertThreshold"] == alert_rule.alert_threshold
-        assert result["resolveThreshold"] == alert_rule.resolve_threshold
+        assert result["alertThreshold"] == 0
+        assert result["resolveThreshold"] == 0
         assert result["thresholdPeriod"] == alert_rule.threshold_period
         assert result["includeAllProjects"] == alert_rule.include_all_projects
         assert result["dateModified"] == alert_rule.date_modified
@@ -37,12 +37,9 @@ class AlertRuleSerializerTest(BaseAlertRuleSerializerTest, TestCase):
             self.organization,
             [self.project],
             "hello",
-            AlertRuleThresholdType.ABOVE,
             "level:error",
             QueryAggregations.TOTAL,
             10,
-            1000,
-            400,
             1,
         )
         result = serialize(alert_rule)

tests/sentry/api/serializers/test_incident.py

@@ -11,7 +11,7 @@ from freezegun import freeze_time
 from sentry.api.serializers import serialize
 from sentry.api.serializers.models.incident import DetailedIncidentSerializer
 from sentry.incidents.logic import create_alert_rule, subscribe_to_incident
-from sentry.incidents.models import AlertRuleThresholdType, IncidentGroup
+from sentry.incidents.models import IncidentGroup
 from sentry.snuba.models import QueryAggregations
 from sentry.testutils import TestCase
 
@@ -68,16 +68,7 @@ class DetailedIncidentSerializerTest(TestCase):
     def test_alert_rule(self):
         incident = self.create_incident()
         alert_rule = create_alert_rule(
-            self.organization,
-            [self.project],
-            "hi",
-            AlertRuleThresholdType.ABOVE,
-            "test query",
-            QueryAggregations.TOTAL,
-            10,
-            1000,
-            400,
-            1,
+            self.organization, [self.project], "hi", "test query", QueryAggregations.TOTAL, 10, 1
         )
         incident.update(alert_rule=alert_rule)
 

tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py

@@ -5,7 +5,7 @@ from exam import fixture
 from sentry.api.serializers import serialize
 from sentry.api.serializers.models.alert_rule import DetailedAlertRuleSerializer
 from sentry.incidents.logic import create_alert_rule
-from sentry.incidents.models import AlertRule, AlertRuleThresholdType
+from sentry.incidents.models import AlertRule
 from sentry.snuba.models import QueryAggregations
 from sentry.testutils import APITestCase
 
@@ -31,12 +31,9 @@ class AlertRuleDetailsBase(object):
             self.organization,
             [self.project],
             "hello",
-            AlertRuleThresholdType.ABOVE,
             "level:error",
             QueryAggregations.TOTAL,
             10,
-            1000,
-            400,
             1,
         )
 

tests/sentry/incidents/endpoints/test_organization_alert_rule_index.py

@@ -5,7 +5,7 @@ from freezegun import freeze_time
 
 from sentry.api.serializers import serialize
 from sentry.incidents.logic import create_alert_rule
-from sentry.incidents.models import AlertRule, AlertRuleThresholdType
+from sentry.incidents.models import AlertRule
 from sentry.snuba.models import QueryAggregations
 from sentry.testutils import APITestCase
 
@@ -31,12 +31,9 @@ class AlertRuleListEndpointTest(APITestCase):
             self.organization,
             [self.project],
             "hello",
-            AlertRuleThresholdType.ABOVE,
             "level:error",
             QueryAggregations.TOTAL,
             10,
-            1000,
-            400,
             1,
         )