Skip to content

Commit

Permalink
feat(metric_alerts): Allow trigger thresholds to be float (#19075)
Browse files Browse the repository at this point in the history
This updates the data model to convert the threshold fields from int -> float.

The data migration should be fine since this table is small/not on a hot path. I tested having code
where the model still has `models.IntegerField` defined, but the database has `double`, and
everything continued to work as usual - fetches and updates all worked fine and didn't cause type
conversion errors.
  • Loading branch information
wedamija authored May 29, 2020
1 parent 3b57fe7 commit 9ba9a35
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 7 deletions.
2 changes: 1 addition & 1 deletion migrations_lockfile.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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: 0081_add_integraiton_upgrade_audit_log
sentry: 0082_alert_rules_threshold_float
sessions: 0001_initial
sites: 0002_alter_domain_unique
social_auth: 0001_initial
4 changes: 2 additions & 2 deletions src/sentry/incidents/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -469,8 +469,8 @@ class AlertRuleTrigger(Model):
alert_rule = FlexibleForeignKey("sentry.AlertRule")
label = models.TextField()
threshold_type = models.SmallIntegerField()
alert_threshold = models.IntegerField()
resolve_threshold = models.IntegerField(null=True)
alert_threshold = models.FloatField()
resolve_threshold = models.FloatField(null=True)
triggered_incidents = models.ManyToManyField(
"sentry.Incident", related_name="triggers", through=IncidentTrigger
)
Expand Down
42 changes: 42 additions & 0 deletions src/sentry/migrations/0082_alert_rules_threshold_float.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.29 on 2020-05-29 20:19
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', '0081_add_integraiton_upgrade_audit_log'),
]

operations = [
migrations.AlterField(
model_name='alertruletrigger',
name='alert_threshold',
field=models.FloatField(),
),
migrations.AlterField(
model_name='alertruletrigger',
name='resolve_threshold',
field=models.FloatField(null=True),
),
]
8 changes: 8 additions & 0 deletions tests/sentry/api/serializers/test_alert_rule_trigger.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ def test_simple(self):
result = serialize(trigger)
self.assert_alert_rule_trigger_serialized(trigger, result)

def test_decimal(self):
alert_rule = self.create_alert_rule()
trigger = create_alert_rule_trigger(
alert_rule, "hi", AlertRuleThresholdType.ABOVE, 1000.50, 200.70
)
result = serialize(trigger)
self.assert_alert_rule_trigger_serialized(trigger, result)


class DetailedAlertRuleTriggerSerializerTest(BaseAlertRuleTriggerSerializerTest, TestCase):
def test_simple(self):
Expand Down
19 changes: 15 additions & 4 deletions tests/sentry/incidents/endpoints/test_serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ def valid_params(self):
"dataset": QueryDatasets.EVENTS.value,
"query": "level:error",
"threshold_type": 0,
"resolve_threshold": 1,
"alert_threshold": 0,
"aggregate": "count()",
"threshold_period": 1,
"projects": [self.project.slug],
Expand Down Expand Up @@ -160,14 +158,27 @@ def test_transaction_dataset(self):
assert alert_rule.snuba_query.dataset == QueryDatasets.TRANSACTIONS.value
assert alert_rule.snuba_query.aggregate == "count()"

def test_decimal(self):
params = self.valid_transaction_params.copy()
alert_threshold = 0.8
resolve_threshold = 0.7
params["triggers"][0]["alertThreshold"] = alert_threshold
params["triggers"][0]["resolveThreshold"] = resolve_threshold
# Drop off the warning trigger
params["triggers"].pop()
serializer = AlertRuleSerializer(context=self.context, data=self.valid_transaction_params)
assert serializer.is_valid(), serializer.errors
alert_rule = serializer.save()
trigger = alert_rule.alertruletrigger_set.filter(label="critical").get()
assert trigger.alert_threshold == alert_threshold
assert trigger.resolve_threshold == resolve_threshold

def test_simple_below_threshold(self):
payload = {
"name": "hello_im_a_test",
"time_window": 10,
"query": "level:error",
"threshold_type": 0,
"resolve_threshold": 1,
"alert_threshold": 0,
"aggregate": "count()",
"threshold_period": 1,
"projects": [self.project.slug],
Expand Down

0 comments on commit 9ba9a35

Please sign in to comment.