From b449f656f7aa2aa64c1e7b22c744cf77d54b1fa2 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 8 Aug 2025 16:16:23 -0700 Subject: [PATCH 1/3] feat(crons): Start dual writing `DataSource` and `Detector` rows for crons We want to have these rows available so that we can start linking crons to workflows. --- .../monitors/consumers/monitor_consumer.py | 2 + .../endpoints/organization_monitor_index.py | 8 +- src/sentry/monitors/models.py | 1 + src/sentry/monitors/utils.py | 27 ++++ .../consumers/test_monitor_consumer.py | 7 +- .../test_organization_monitor_index.py | 7 ++ tests/sentry/monitors/test_utils.py | 119 ++++++++++++++++++ 7 files changed, 169 insertions(+), 2 deletions(-) create mode 100644 tests/sentry/monitors/test_utils.py diff --git a/src/sentry/monitors/consumers/monitor_consumer.py b/src/sentry/monitors/consumers/monitor_consumer.py index 0fd057c4de33ce..cefc12e36bae8d 100644 --- a/src/sentry/monitors/consumers/monitor_consumer.py +++ b/src/sentry/monitors/consumers/monitor_consumer.py @@ -70,6 +70,7 @@ from sentry.monitors.system_incidents import update_check_in_volume from sentry.monitors.types import CheckinItem from sentry.monitors.utils import ( + ensure_cron_detector, get_new_timeout_at, get_timeout_at, signal_first_checkin, @@ -165,6 +166,7 @@ def _ensure_monitor_with_config( "is_upserting": True, }, ) + ensure_cron_detector(monitor) if created: signal_monitor_created(project, None, True, monitor, None) diff --git a/src/sentry/monitors/endpoints/organization_monitor_index.py b/src/sentry/monitors/endpoints/organization_monitor_index.py index e7e17cda91e671..d767f9d941a410 100644 --- a/src/sentry/monitors/endpoints/organization_monitor_index.py +++ b/src/sentry/monitors/endpoints/organization_monitor_index.py @@ -47,7 +47,11 @@ MonitorSerializer, MonitorSerializerResponse, ) -from sentry.monitors.utils import create_issue_alert_rule, signal_monitor_created +from sentry.monitors.utils import ( + create_issue_alert_rule, + ensure_cron_detector, + signal_monitor_created, +) from sentry.monitors.validators import MonitorBulkEditValidator, MonitorValidator from sentry.search.utils import tokenize_query from sentry.types.actor import Actor @@ -291,6 +295,8 @@ def post(self, request: AuthenticatedHttpRequest, organization) -> Response: except MonitorLimitsExceeded as e: return self.respond({type(e).__name__: str(e)}, status=403) + ensure_cron_detector(monitor) + # Attempt to assign a seat for this monitor seat_outcome = quotas.backend.assign_monitor_seat(monitor) if seat_outcome != Outcome.ACCEPTED: diff --git a/src/sentry/monitors/models.py b/src/sentry/monitors/models.py index cf985cfd112787..a4c00e040438f2 100644 --- a/src/sentry/monitors/models.py +++ b/src/sentry/monitors/models.py @@ -209,6 +209,7 @@ def get_name(cls, value): return dict(cls.as_choices())[value] +@data_source_type_registry.register(DATA_SOURCE_CRON_MONITOR) @region_silo_model class Monitor(Model): __relocation_scope__ = RelocationScope.Organization diff --git a/src/sentry/monitors/utils.py b/src/sentry/monitors/utils.py index 0f69cc022f365e..d3228446586b13 100644 --- a/src/sentry/monitors/utils.py +++ b/src/sentry/monitors/utils.py @@ -1,3 +1,4 @@ +import logging from collections import defaultdict from datetime import datetime, timedelta @@ -13,6 +14,7 @@ from sentry.models.rule import Rule, RuleActivity, RuleActivityType, RuleSource from sentry.monitors.constants import DEFAULT_CHECKIN_MARGIN, MAX_TIMEOUT, TIMEOUT from sentry.monitors.models import CheckInStatus, Monitor, MonitorCheckIn +from sentry.monitors.types import DATA_SOURCE_CRON_MONITOR from sentry.projects.project_rules.creator import ProjectRuleCreator from sentry.projects.project_rules.updater import ProjectRuleUpdater from sentry.signals import ( @@ -23,7 +25,11 @@ from sentry.users.models.user import User from sentry.utils.audit import create_audit_entry, create_system_audit_entry from sentry.utils.auth import AuthenticatedHttpRequest +from sentry.utils.db import atomic_transaction from sentry.utils.projectflags import set_project_flag_and_signal +from sentry.workflow_engine.models import DataSource, DataSourceDetector, Detector + +logger = logging.getLogger(__name__) def signal_first_checkin(project: Project, monitor: Monitor): @@ -383,3 +389,24 @@ def update_issue_alert_rule( ) return issue_alert_rule.id + + +def ensure_cron_detector(monitor: Monitor): + try: + with atomic_transaction(using=router.db_for_write(DataSource)): + data_source, created = DataSource.objects.get_or_create( + type=DATA_SOURCE_CRON_MONITOR, + organization_id=monitor.organization_id, + source_id=str(monitor.id), + ) + if created: + detector = Detector.objects.create( + type="monitor_check_in_failure", + project_id=monitor.project_id, + name=monitor.name, + owner_user_id=monitor.owner_user_id, + owner_team_id=monitor.owner_team_id, + ) + DataSourceDetector.objects.create(data_source=data_source, detector=detector) + except Exception: + logger.exception("Error creating cron detector") diff --git a/tests/sentry/monitors/consumers/test_monitor_consumer.py b/tests/sentry/monitors/consumers/test_monitor_consumer.py index 83e729cf38244c..1375fdb39d1f00 100644 --- a/tests/sentry/monitors/consumers/test_monitor_consumer.py +++ b/tests/sentry/monitors/consumers/test_monitor_consumer.py @@ -29,13 +29,14 @@ ScheduleType, ) from sentry.monitors.processing_errors.errors import ProcessingErrorsException, ProcessingErrorType -from sentry.monitors.types import CheckinItem +from sentry.monitors.types import DATA_SOURCE_CRON_MONITOR, CheckinItem from sentry.testutils.asserts import assert_org_audit_log_exists from sentry.testutils.cases import TestCase from sentry.testutils.helpers.options import override_options from sentry.testutils.outbox import outbox_runner from sentry.utils import json from sentry.utils.outcomes import Outcome +from sentry.workflow_engine.models import Detector class ExpectNoProcessingError: @@ -572,6 +573,10 @@ def test_monitor_create(self) -> None: monitor_environment.next_checkin_latest == monitor_environment.monitor.get_next_expected_checkin_latest(checkin.date_added) ) + assert Detector.objects.filter( + datasource__type=DATA_SOURCE_CRON_MONITOR, + datasource__source_id=str(monitor_environment.monitor_id), + ).exists() def test_monitor_create_owner(self) -> None: self.send_checkin( diff --git a/tests/sentry/monitors/endpoints/test_organization_monitor_index.py b/tests/sentry/monitors/endpoints/test_organization_monitor_index.py index 4f05801bc54d2a..e21630ebadfbab 100644 --- a/tests/sentry/monitors/endpoints/test_organization_monitor_index.py +++ b/tests/sentry/monitors/endpoints/test_organization_monitor_index.py @@ -12,12 +12,14 @@ from sentry.constants import ObjectStatus from sentry.models.rule import Rule, RuleSource from sentry.monitors.models import Monitor, MonitorStatus, ScheduleType +from sentry.monitors.types import DATA_SOURCE_CRON_MONITOR from sentry.quotas.base import SeatAssignmentResult from sentry.slug.errors import DEFAULT_SLUG_ERROR_MESSAGE from sentry.testutils.asserts import assert_org_audit_log_exists from sentry.testutils.cases import MonitorTestCase from sentry.testutils.outbox import outbox_runner from sentry.utils.outcomes import Outcome +from sentry.workflow_engine.models import Detector class ListOrganizationMonitorsTest(MonitorTestCase): @@ -402,6 +404,11 @@ def test_simple(self, mock_record: MagicMock) -> None: data={"upsert": False, **monitor.get_audit_log_data()}, ) + assert Detector.objects.filter( + datasource__type=DATA_SOURCE_CRON_MONITOR, + datasource__source_id=str(monitor.id), + ).exists() + self.project.refresh_from_db() assert self.project.flags.has_cron_monitors diff --git a/tests/sentry/monitors/test_utils.py b/tests/sentry/monitors/test_utils.py new file mode 100644 index 00000000000000..0f09b6bd37c412 --- /dev/null +++ b/tests/sentry/monitors/test_utils.py @@ -0,0 +1,119 @@ +from unittest.mock import patch + +from django.db import IntegrityError + +from sentry.monitors.types import DATA_SOURCE_CRON_MONITOR +from sentry.monitors.utils import ensure_cron_detector +from sentry.testutils.cases import TestCase +from sentry.workflow_engine.models import DataSource, DataSourceDetector, Detector + + +class EnsureCronDetectorTest(TestCase): + def setUp(self): + super().setUp() + self.monitor = self.create_monitor(owner_user_id=None) + + def test_creates_data_source_and_detector_for_new_monitor(self): + assert not DataSource.objects.filter( + type=DATA_SOURCE_CRON_MONITOR, + organization_id=self.monitor.organization_id, + source_id=str(self.monitor.id), + ).exists() + + ensure_cron_detector(self.monitor) + data_source = DataSource.objects.get( + type=DATA_SOURCE_CRON_MONITOR, + organization_id=self.monitor.organization_id, + source_id=str(self.monitor.id), + ) + assert data_source is not None + detector = Detector.objects.get( + type="monitor_check_in_failure", + project_id=self.monitor.project_id, + name=self.monitor.name, + ) + assert detector is not None + assert detector.owner_user_id == self.monitor.owner_user_id + assert detector.owner_team_id == self.monitor.owner_team_id + assert DataSourceDetector.objects.filter( + data_source=data_source, + detector=detector, + ).exists() + + def test_idempotent_for_existing_data_source(self): + ensure_cron_detector(self.monitor) + data_source = DataSource.objects.get( + type=DATA_SOURCE_CRON_MONITOR, + organization_id=self.monitor.organization_id, + source_id=str(self.monitor.id), + ) + detector = Detector.objects.get( + type="monitor_check_in_failure", + project_id=self.monitor.project_id, + name=self.monitor.name, + ) + link = DataSourceDetector.objects.get( + data_source=data_source, + detector=detector, + ) + ensure_cron_detector(self.monitor) + data_source_after = DataSource.objects.get( + type=DATA_SOURCE_CRON_MONITOR, + organization_id=self.monitor.organization_id, + source_id=str(self.monitor.id), + ) + detector_after = Detector.objects.get( + type="monitor_check_in_failure", + project_id=self.monitor.project_id, + name=self.monitor.name, + ) + link_after = DataSourceDetector.objects.get( + data_source=data_source, + detector=detector, + ) + assert data_source.id == data_source_after.id + assert detector.id == detector_after.id + assert link.id == link_after.id + + def test_with_owner_user(self): + self.monitor.owner_user_id = self.user.id + self.monitor.save() + ensure_cron_detector(self.monitor) + detector = Detector.objects.get( + type="monitor_check_in_failure", + project_id=self.monitor.project_id, + ) + assert detector.owner_user_id == self.user.id + assert detector.owner_team_id is None + + def test_with_no_owner(self): + ensure_cron_detector(self.monitor) + + detector = Detector.objects.get( + type="monitor_check_in_failure", + project_id=self.monitor.project_id, + ) + assert detector.owner_user_id is None + assert detector.owner_team_id is None + + def test_handles_database_errors_gracefully(self): + with ( + patch("sentry.monitors.utils.logger") as mock_logger, + patch("sentry.monitors.utils.DataSource.objects.get_or_create") as mock_get_or_create, + ): + mock_get_or_create.side_effect = IntegrityError("Database error") + + ensure_cron_detector(self.monitor) + mock_logger.exception.assert_called_once_with("Error creating cron detector") + assert not DataSource.objects.filter( + type=DATA_SOURCE_CRON_MONITOR, source_id=str(self.monitor.id) + ).exists() + + def test_atomic_transaction_rollback(self): + with patch("sentry.monitors.utils.Detector.objects.create") as mock_create: + mock_create.side_effect = IntegrityError("Cannot create detector") + + ensure_cron_detector(self.monitor) + assert not DataSource.objects.filter( + type=DATA_SOURCE_CRON_MONITOR, source_id=str(self.monitor.id) + ).exists() From e74950d690d8bde33c6004a92e20e7f970b4183d Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 8 Aug 2025 17:00:49 -0700 Subject: [PATCH 2/3] Models aren't DataSourceHandlers --- src/sentry/monitors/models.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/sentry/monitors/models.py b/src/sentry/monitors/models.py index a4c00e040438f2..cf985cfd112787 100644 --- a/src/sentry/monitors/models.py +++ b/src/sentry/monitors/models.py @@ -209,7 +209,6 @@ def get_name(cls, value): return dict(cls.as_choices())[value] -@data_source_type_registry.register(DATA_SOURCE_CRON_MONITOR) @region_silo_model class Monitor(Model): __relocation_scope__ = RelocationScope.Organization From c920e2695457ac28234f8fbc5b48075dd5639829 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Mon, 11 Aug 2025 15:52:42 -0700 Subject: [PATCH 3/3] stop hardcoding type --- src/sentry/monitors/utils.py | 4 +++- tests/sentry/monitors/test_utils.py | 11 ++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/sentry/monitors/utils.py b/src/sentry/monitors/utils.py index d3228446586b13..782ded49071b28 100644 --- a/src/sentry/monitors/utils.py +++ b/src/sentry/monitors/utils.py @@ -392,6 +392,8 @@ def update_issue_alert_rule( def ensure_cron_detector(monitor: Monitor): + from sentry.issues.grouptype import MonitorCheckInFailure + try: with atomic_transaction(using=router.db_for_write(DataSource)): data_source, created = DataSource.objects.get_or_create( @@ -401,7 +403,7 @@ def ensure_cron_detector(monitor: Monitor): ) if created: detector = Detector.objects.create( - type="monitor_check_in_failure", + type=MonitorCheckInFailure.slug, project_id=monitor.project_id, name=monitor.name, owner_user_id=monitor.owner_user_id, diff --git a/tests/sentry/monitors/test_utils.py b/tests/sentry/monitors/test_utils.py index 0f09b6bd37c412..0fc43c18a660c1 100644 --- a/tests/sentry/monitors/test_utils.py +++ b/tests/sentry/monitors/test_utils.py @@ -2,6 +2,7 @@ from django.db import IntegrityError +from sentry.issues.grouptype import MonitorIncidentType from sentry.monitors.types import DATA_SOURCE_CRON_MONITOR from sentry.monitors.utils import ensure_cron_detector from sentry.testutils.cases import TestCase @@ -28,7 +29,7 @@ def test_creates_data_source_and_detector_for_new_monitor(self): ) assert data_source is not None detector = Detector.objects.get( - type="monitor_check_in_failure", + type=MonitorIncidentType.slug, project_id=self.monitor.project_id, name=self.monitor.name, ) @@ -48,7 +49,7 @@ def test_idempotent_for_existing_data_source(self): source_id=str(self.monitor.id), ) detector = Detector.objects.get( - type="monitor_check_in_failure", + type=MonitorIncidentType.slug, project_id=self.monitor.project_id, name=self.monitor.name, ) @@ -63,7 +64,7 @@ def test_idempotent_for_existing_data_source(self): source_id=str(self.monitor.id), ) detector_after = Detector.objects.get( - type="monitor_check_in_failure", + type=MonitorIncidentType.slug, project_id=self.monitor.project_id, name=self.monitor.name, ) @@ -80,7 +81,7 @@ def test_with_owner_user(self): self.monitor.save() ensure_cron_detector(self.monitor) detector = Detector.objects.get( - type="monitor_check_in_failure", + type=MonitorIncidentType.slug, project_id=self.monitor.project_id, ) assert detector.owner_user_id == self.user.id @@ -90,7 +91,7 @@ def test_with_no_owner(self): ensure_cron_detector(self.monitor) detector = Detector.objects.get( - type="monitor_check_in_failure", + type=MonitorIncidentType.slug, project_id=self.monitor.project_id, ) assert detector.owner_user_id is None