Skip to content

Commit fb20f0b

Browse files
committed
feat(cron-monitors): Add billing seat management to cron monitor detectors
Implements billing seat management for cron monitor detectors via the MonitorIncidentDetectorValidator. This ensures that cron monitors created through the detector API properly handle seat assignment, validation, and removal. Changes: - Add validate_enabled() to check seat availability before enabling - Modify create() to assign seats with graceful degradation when no seats are available (detector created but disabled) - Modify update() to handle enable/disable transitions with seat operations and race condition handling - Add delete() to remove seats immediately when detector is deleted - Add comprehensive test coverage for all seat management scenarios Uses generic seat APIs (assign_seat, check_assign_seat, disable_seat, remove_seat) with DataCategory.MONITOR following the same pattern as uptime monitors. Fixes [NEW-619: Ensure CRUD / enable+disable Cron Detectors in new UI handles assigning / unassigning seats](https://linear.app/getsentry/issue/NEW-619/ensure-crud-enabledisable-cron-detectors-in-new-ui-handles-assigning)
1 parent 8dd1ef6 commit fb20f0b

File tree

3 files changed

+272
-8
lines changed

3 files changed

+272
-8
lines changed

src/sentry/monitors/models.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050

5151
if TYPE_CHECKING:
5252
from sentry.models.project import Project
53+
from sentry.workflow_engine.models import Detector
5354

5455
MONITOR_CONFIG = {
5556
"type": "object",
@@ -812,6 +813,15 @@ class Meta:
812813
db_table = "sentry_monitorenvbrokendetection"
813814

814815

816+
def get_cron_monitor(detector: Detector) -> Monitor:
817+
"""
818+
Given a detector get the matching cron monitor.
819+
"""
820+
data_source = detector.data_sources.first()
821+
assert data_source
822+
return Monitor.objects.get(id=int(data_source.source_id))
823+
824+
815825
@data_source_type_registry.register(DATA_SOURCE_CRON_MONITOR)
816826
class CronMonitorDataSourceHandler(DataSourceTypeHandler[Monitor]):
817827
@staticmethod

src/sentry/monitors/validators.py

Lines changed: 56 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
from sentry.api.fields.sentry_slug import SentrySerializerSlugField
1919
from sentry.api.serializers.rest_framework import CamelSnakeSerializer
2020
from sentry.api.serializers.rest_framework.project import ProjectField
21-
from sentry.constants import ObjectStatus
21+
from sentry.constants import DataCategory, ObjectStatus
2222
from sentry.db.models import BoundedPositiveIntegerField
2323
from sentry.db.models.fields.slug import DEFAULT_SLUG_MAX_LENGTH
2424
from sentry.db.postgres.transactions import in_test_hide_transaction_boundary
@@ -35,6 +35,7 @@
3535
MonitorLimitsExceeded,
3636
ScheduleType,
3737
check_organization_monitor_limit,
38+
get_cron_monitor,
3839
)
3940
from sentry.monitors.schedule import get_next_schedule, get_prev_schedule
4041
from sentry.monitors.types import CrontabSchedule, slugify_monitor_slug
@@ -52,7 +53,7 @@
5253
BaseDataSourceValidator,
5354
BaseDetectorTypeValidator,
5455
)
55-
from sentry.workflow_engine.models import DataSource, Detector
56+
from sentry.workflow_engine.models import Detector
5657

5758
MONITOR_STATUSES = {
5859
"active": ObjectStatus.ACTIVE,
@@ -686,16 +687,58 @@ class MonitorIncidentDetectorValidator(BaseDetectorTypeValidator):
686687

687688
data_sources = serializers.ListField(child=MonitorDataSourceValidator(), required=False)
688689

690+
def validate_enabled(self, value: bool) -> bool:
691+
"""
692+
Validate that enabling a detector is allowed based on seat availability.
693+
"""
694+
detector = self.instance
695+
if detector and value and not detector.enabled:
696+
monitor = get_cron_monitor(detector)
697+
result = quotas.backend.check_assign_seat(DataCategory.MONITOR, monitor)
698+
if not result.assignable:
699+
raise serializers.ValidationError(result.reason)
700+
return value
701+
702+
def create(self, validated_data):
703+
detector = super().create(validated_data)
704+
monitor = get_cron_monitor(detector)
705+
706+
# Try to assign a seat for the monitor
707+
seat_outcome = quotas.backend.assign_seat(DataCategory.MONITOR, monitor)
708+
if seat_outcome != Outcome.ACCEPTED:
709+
detector.update(enabled=False)
710+
monitor.update(status=ObjectStatus.DISABLED)
711+
712+
return detector
713+
689714
def update(self, instance: Detector, validated_data: dict[str, Any]) -> Detector:
715+
was_enabled = instance.enabled
716+
enabled = validated_data.get("enabled", was_enabled)
717+
718+
# Handle enable/disable seat operations
719+
if was_enabled != enabled:
720+
monitor = get_cron_monitor(instance)
721+
722+
if enabled:
723+
seat_outcome = quotas.backend.assign_seat(DataCategory.MONITOR, monitor)
724+
# We should have already validated that a seat was available in
725+
# validate_enabled, avoid races by failing here if we can't
726+
# accept the seat
727+
if seat_outcome != Outcome.ACCEPTED:
728+
raise serializers.ValidationError("Failed to update monitor")
729+
monitor.update(status=ObjectStatus.ACTIVE)
730+
else:
731+
quotas.backend.disable_seat(DataCategory.MONITOR, monitor)
732+
monitor.update(status=ObjectStatus.DISABLED)
733+
690734
super().update(instance, validated_data)
691735

692736
data_source_data = None
693737
if "data_sources" in validated_data:
694738
data_source_data = validated_data.pop("data_sources")[0]
695739

696740
if data_source_data is not None:
697-
data_source = DataSource.objects.get(detectors=instance)
698-
monitor = Monitor.objects.get(id=data_source.source_id)
741+
monitor = get_cron_monitor(instance)
699742

700743
monitor_validator = MonitorDataSourceValidator(
701744
instance=monitor,
@@ -709,3 +752,12 @@ def update(self, instance: Detector, validated_data: dict[str, Any]) -> Detector
709752
monitor_validator.save()
710753

711754
return instance
755+
756+
def delete(self) -> None:
757+
assert self.instance is not None
758+
monitor = get_cron_monitor(self.instance)
759+
760+
# Remove the seat immediately
761+
quotas.backend.remove_seat(DataCategory.MONITOR, monitor)
762+
763+
super().delete()

tests/sentry/monitors/test_validators.py

Lines changed: 206 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,22 +6,25 @@
66
from django.test import RequestFactory
77
from django.test.utils import override_settings
88
from django.utils import timezone
9+
from rest_framework.exceptions import ErrorDetail
910

1011
from sentry.analytics.events.cron_monitor_created import CronMonitorCreated, FirstCronMonitorCreated
11-
from sentry.constants import ObjectStatus
12+
from sentry.constants import DataCategory, ObjectStatus
1213
from sentry.models.rule import Rule, RuleSource
13-
from sentry.monitors.models import Monitor, MonitorStatus, ScheduleType
14+
from sentry.monitors.models import Monitor, MonitorStatus, ScheduleType, get_cron_monitor
15+
from sentry.monitors.types import DATA_SOURCE_CRON_MONITOR
1416
from sentry.monitors.validators import (
1517
MonitorDataSourceValidator,
1618
MonitorIncidentDetectorValidator,
1719
MonitorValidator,
1820
)
21+
from sentry.quotas.base import SeatAssignmentResult
1922
from sentry.testutils.cases import MonitorTestCase
2023
from sentry.testutils.helpers.analytics import assert_any_analytics_event
2124
from sentry.types.actor import Actor
2225
from sentry.utils.outcomes import Outcome
2326
from sentry.utils.slug import DEFAULT_SLUG_ERROR_MESSAGE
24-
from sentry.workflow_engine.models import DataConditionGroup
27+
from sentry.workflow_engine.models import DataConditionGroup, DataSource, DataSourceDetector
2528

2629

2730
class MonitorValidatorCreateTest(MonitorTestCase):
@@ -166,7 +169,6 @@ def test_monitor_organization_limit(self):
166169
}
167170
validator = MonitorValidator(data=data, context=self.context)
168171
assert not validator.is_valid()
169-
from rest_framework.exceptions import ErrorDetail
170172

171173
assert validator.errors["nonFieldErrors"] == [
172174
ErrorDetail(
@@ -1123,3 +1125,203 @@ def test_create_detector_validates_data_source(self):
11231125
assert validator.is_valid(), validator.errors
11241126
assert "_creator" in validator.validated_data["data_sources"][0]
11251127
assert validator.validated_data["data_sources"][0]["data_source_type"] == "cron_monitor"
1128+
1129+
@patch("sentry.quotas.backend.assign_seat", return_value=Outcome.ACCEPTED)
1130+
def test_create_enabled_assigns_seat(self, mock_assign_seat):
1131+
"""Test that creating an enabled detector assigns a billing seat."""
1132+
1133+
condition_group = DataConditionGroup.objects.create(
1134+
organization_id=self.organization.id,
1135+
logic_type=DataConditionGroup.Type.ANY,
1136+
)
1137+
context = {**self.context, "condition_group": condition_group}
1138+
validator = MonitorIncidentDetectorValidator(
1139+
data=self.valid_data,
1140+
context=context,
1141+
)
1142+
assert validator.is_valid(), validator.errors
1143+
detector = validator.save()
1144+
1145+
detector.refresh_from_db()
1146+
assert detector.enabled is True
1147+
1148+
# Verify seat was assigned
1149+
monitor = get_cron_monitor(detector)
1150+
mock_assign_seat.assert_called_with(DataCategory.MONITOR, monitor)
1151+
1152+
@patch("sentry.quotas.backend.assign_seat", return_value=Outcome.RATE_LIMITED)
1153+
def test_create_enabled_no_seat_available(self, mock_assign_seat):
1154+
"""
1155+
Test that creating a detector with no seats available creates it but
1156+
leaves it disabled.
1157+
"""
1158+
condition_group = DataConditionGroup.objects.create(
1159+
organization_id=self.organization.id,
1160+
logic_type=DataConditionGroup.Type.ANY,
1161+
)
1162+
context = {**self.context, "condition_group": condition_group}
1163+
validator = MonitorIncidentDetectorValidator(
1164+
data=self.valid_data,
1165+
context=context,
1166+
)
1167+
assert validator.is_valid(), validator.errors
1168+
detector = validator.save()
1169+
1170+
detector.refresh_from_db()
1171+
# Detector created but not enabled due to no seat assignment
1172+
assert detector.enabled is False
1173+
monitor = get_cron_monitor(detector)
1174+
assert monitor.status == ObjectStatus.DISABLED
1175+
1176+
# Verify seat assignment was attempted
1177+
mock_assign_seat.assert_called_with(DataCategory.MONITOR, monitor)
1178+
1179+
@patch("sentry.quotas.backend.assign_seat", return_value=Outcome.ACCEPTED)
1180+
def test_update_enable_assigns_seat(self, mock_assign_seat):
1181+
"""
1182+
Test that enabling a previously disabled detector assigns a seat.
1183+
"""
1184+
# Create a disabled detector
1185+
detector = self.create_detector(
1186+
project=self.project,
1187+
name="Test Detector",
1188+
type="monitor_check_in_failure",
1189+
enabled=False,
1190+
)
1191+
monitor = self._create_monitor(
1192+
name="Test Monitor",
1193+
slug="test-monitor",
1194+
status=ObjectStatus.DISABLED,
1195+
)
1196+
data_source = DataSource.objects.create(
1197+
type=DATA_SOURCE_CRON_MONITOR,
1198+
organization_id=self.organization.id,
1199+
source_id=str(monitor.id),
1200+
)
1201+
DataSourceDetector.objects.create(data_source=data_source, detector=detector)
1202+
1203+
validator = MonitorIncidentDetectorValidator(
1204+
instance=detector, data={"enabled": True}, context=self.context, partial=True
1205+
)
1206+
assert validator.is_valid(), validator.errors
1207+
validator.save()
1208+
1209+
detector.refresh_from_db()
1210+
monitor.refresh_from_db()
1211+
assert detector.enabled is True
1212+
assert monitor.status == ObjectStatus.ACTIVE
1213+
1214+
# Verify seat was assigned
1215+
mock_assign_seat.assert_called_with(DataCategory.MONITOR, monitor)
1216+
1217+
@patch(
1218+
"sentry.quotas.backend.check_assign_seat",
1219+
return_value=SeatAssignmentResult(assignable=False, reason="No seats available"),
1220+
)
1221+
def test_update_enable_no_seat_available(self, mock_check_seat):
1222+
"""
1223+
Test that enabling fails with validation error when no seats are
1224+
available.
1225+
"""
1226+
# Create a disabled detector
1227+
detector = self.create_detector(
1228+
project=self.project,
1229+
name="Test Detector",
1230+
type="monitor_check_in_failure",
1231+
enabled=False,
1232+
)
1233+
monitor = self._create_monitor(
1234+
name="Test Monitor",
1235+
slug="test-monitor",
1236+
status=ObjectStatus.DISABLED,
1237+
)
1238+
data_source = DataSource.objects.create(
1239+
type=DATA_SOURCE_CRON_MONITOR,
1240+
organization_id=self.organization.id,
1241+
source_id=str(monitor.id),
1242+
)
1243+
DataSourceDetector.objects.create(data_source=data_source, detector=detector)
1244+
1245+
validator = MonitorIncidentDetectorValidator(
1246+
instance=detector, data={"enabled": True}, context=self.context, partial=True
1247+
)
1248+
1249+
# Validation should fail due to no seats available
1250+
assert not validator.is_valid()
1251+
assert "enabled" in validator.errors
1252+
assert validator.errors["enabled"] == ["No seats available"]
1253+
1254+
# Detector and monitor should still be disabled
1255+
detector.refresh_from_db()
1256+
monitor.refresh_from_db()
1257+
assert detector.enabled is False
1258+
assert monitor.status == ObjectStatus.DISABLED
1259+
1260+
# Verify seat availability check was performed
1261+
mock_check_seat.assert_called_with(DataCategory.MONITOR, monitor)
1262+
1263+
@patch("sentry.quotas.backend.disable_seat")
1264+
def test_update_disable_disables_seat(self, mock_disable_seat):
1265+
"""Test that disabling a previously enabled detector disables the seat."""
1266+
# Create an enabled detector
1267+
detector = self.create_detector(
1268+
project=self.project,
1269+
name="Test Detector",
1270+
type="monitor_check_in_failure",
1271+
enabled=True,
1272+
)
1273+
monitor = self._create_monitor(
1274+
name="Test Monitor",
1275+
slug="test-monitor",
1276+
status=ObjectStatus.ACTIVE,
1277+
)
1278+
data_source = DataSource.objects.create(
1279+
type=DATA_SOURCE_CRON_MONITOR,
1280+
organization_id=self.organization.id,
1281+
source_id=str(monitor.id),
1282+
)
1283+
DataSourceDetector.objects.create(data_source=data_source, detector=detector)
1284+
1285+
validator = MonitorIncidentDetectorValidator(
1286+
instance=detector, data={"enabled": False}, context=self.context, partial=True
1287+
)
1288+
assert validator.is_valid(), validator.errors
1289+
validator.save()
1290+
1291+
detector.refresh_from_db()
1292+
monitor.refresh_from_db()
1293+
assert detector.enabled is False
1294+
assert monitor.status == ObjectStatus.DISABLED
1295+
1296+
# Verify disable_seat was called
1297+
mock_disable_seat.assert_called_with(DataCategory.MONITOR, monitor)
1298+
1299+
@patch("sentry.quotas.backend.remove_seat")
1300+
def test_delete_removes_seat(self, mock_remove_seat: MagicMock) -> None:
1301+
"""Test that deleting a detector removes its billing seat immediately."""
1302+
detector = self.create_detector(
1303+
project=self.project,
1304+
name="Test Detector",
1305+
type="monitor_check_in_failure",
1306+
enabled=True,
1307+
)
1308+
monitor = self._create_monitor(
1309+
name="Test Monitor",
1310+
slug="test-monitor",
1311+
status=ObjectStatus.ACTIVE,
1312+
)
1313+
data_source = DataSource.objects.create(
1314+
type=DATA_SOURCE_CRON_MONITOR,
1315+
organization_id=self.organization.id,
1316+
source_id=str(monitor.id),
1317+
)
1318+
DataSourceDetector.objects.create(data_source=data_source, detector=detector)
1319+
1320+
validator = MonitorIncidentDetectorValidator(
1321+
instance=detector, data={}, context=self.context
1322+
)
1323+
1324+
validator.delete()
1325+
1326+
# Verify remove_seat was called immediately
1327+
mock_remove_seat.assert_called_with(DataCategory.MONITOR, monitor)

0 commit comments

Comments
 (0)