fix(crons): Minimally updates monitor config during upsert (#48657)

Minimally update monitor config during upsert so as not to overwrite
upcoming `alert_rule_id` or existing variables like `checkin_margin`
`max_runtime` or `timezone`

src/sentry/monitors/consumers/check_in.py

@@ -77,7 +77,7 @@ def _ensure_monitor_with_config(
 
     # Update existing monitor
     if monitor and not created and monitor.config != validated_config:
-        monitor.update(config=validated_config)
+        monitor.update_config(config, validated_config)
 
     return monitor
 

src/sentry/monitors/endpoints/monitor_ingest_checkin_index.py

@@ -174,7 +174,9 @@ class MonitorIngestCheckInIndexEndpoint(MonitorIngestEndpoint):
 
             # Update monitor configuration during checkin if config is changed
             if update_monitor and monitor_data["config"] != monitor.config:
-                monitor.update(config=monitor_data["config"])
+                monitor.update_config(
+                    request.data.get("monitor_config", {}), monitor_data["config"]
+                )
 
             try:
                 monitor_environment = MonitorEnvironment.objects.ensure_environment(

src/sentry/monitors/models.py

@@ -254,6 +254,14 @@ class Monitor(Model):
         )
         return next_checkin + timedelta(minutes=int(self.config.get("checkin_margin") or 0))
 
+    def update_config(self, config_payload, validated_config):
+        monitor_config = self.config
+        # Only update keys that were specified in the payload
+        for key in config_payload.keys():
+            if key in validated_config:
+                monitor_config[key] = validated_config[key]
+        self.save()
+
 
 @receiver(pre_save, sender=Monitor)
 def check_organization_monitor_limits(sender, instance, **kwargs):

tests/sentry/monitors/endpoints/test_monitor_ingest_checkin_index.py

@@ -159,13 +159,18 @@ class CreateMonitorCheckInTest(MonitorIngestTestCase):
                 path,
                 {
                     "status": "ok",
-                    "monitor_config": {"schedule_type": "crontab", "schedule": "5 * * * *"},
+                    "monitor_config": {
+                        "schedule_type": "crontab",
+                        "schedule": "5 * * * *",
+                        "checkin_margin": 5,
+                    },
                 },
                 **self.dsn_auth_headers,
             )
             assert resp.status_code == 201, resp.content
             monitor = Monitor.objects.get(slug=slug)
             assert monitor.config["schedule"] == "5 * * * *"
+            assert monitor.config["checkin_margin"] == 5
 
             checkins = MonitorCheckIn.objects.filter(monitor=monitor)
             assert len(checkins) == 1
@@ -182,6 +187,8 @@ class CreateMonitorCheckInTest(MonitorIngestTestCase):
 
             monitor = Monitor.objects.get(guid=monitor.guid)
             assert monitor.config["schedule"] == "10 * * * *"
+            # The monitor config is merged, so checkin_margin is not overwritten
+            assert monitor.config["checkin_margin"] == 5
 
             checkins = MonitorCheckIn.objects.filter(monitor=monitor)
             assert len(checkins) == 2

tests/sentry/monitors/test_models.py

@@ -16,6 +16,7 @@ from sentry.monitors.models import (
     MonitorType,
     ScheduleType,
 )
+from sentry.monitors.validators import ConfigValidator
 from sentry.testutils import TestCase
 from sentry.testutils.silo import region_silo_test
 
@@ -304,3 +305,35 @@ class MonitorEnvironmentTestCase(TestCase):
             MonitorEnvironment.objects.ensure_environment(
                 self.project, monitor, f"space-{settings.MAX_ENVIRONMENTS_PER_MONITOR}"
             )
+
+    def test_update_config(self):
+        monitor = Monitor.objects.create(
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            name="Unicron",
+            slug="unicron",
+            config={
+                "schedule": [1, "month"],
+                "schedule_type": ScheduleType.INTERVAL,
+                "alert_rule_id": 1,
+            },
+        )
+
+        new_config = {
+            "schedule": [2, "month"],
+            "schedule_type": "interval",
+            "max_runtime": 10,
+            "garbage": "data",
+        }
+        validator = ConfigValidator(data=new_config)
+        assert validator.is_valid()
+        validated_config = validator.validated_data
+        monitor.update_config(new_config, validated_config)
+
+        assert monitor.config == {
+            "schedule": [2, "month"],
+            "schedule_type": ScheduleType.INTERVAL,
+            "max_runtime": 10,
+            "alert_rule_id": 1,
+        }

tests/sentry/monitors/test_monitor_consumer.py

@@ -53,7 +53,11 @@ class MonitorConsumerTest(TestCase):
             project_id=self.project.id,
             next_checkin=timezone.now() + timedelta(minutes=1),
             type=MonitorType.CRON_JOB,
-            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+            config={
+                "schedule": "* * * * *",
+                "schedule_type": ScheduleType.CRONTAB,
+                "checkin_margin": 5,
+            },
             **kwargs,
         )
 
@@ -214,6 +218,8 @@ class MonitorConsumerTest(TestCase):
 
         monitor = Monitor.objects.get(id=monitor.id)
         assert monitor.config["schedule"] == "13 * * * *"
+        # The monitor config is merged, so checkin_margin is not overwritten
+        assert monitor.config["checkin_margin"] == 5
 
         monitor_environment = MonitorEnvironment.objects.get(id=checkin.monitor_environment.id)
         assert monitor_environment.status == MonitorStatus.OK