-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(aci): backfill detectorgroup for metric issues #103399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a0c2c04
604f0bb
1ae5ade
e05df16
e7f3c54
af4d35d
7536854
bcd82e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| # Generated by Django 5.2.1 | ||
| import logging | ||
| from collections.abc import Sequence | ||
| from datetime import datetime | ||
| from enum import Enum | ||
| from typing import Any | ||
|
|
||
| from django.db import migrations | ||
| from django.db.backends.base.schema import BaseDatabaseSchemaEditor | ||
| from django.db.migrations.state import StateApps | ||
| from snuba_sdk import Column, Condition, Op | ||
|
|
||
| from sentry import eventstore | ||
| from sentry.new_migrations.migrations import CheckedMigration | ||
| from sentry.snuba.dataset import Dataset | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class EventOrdering(Enum): | ||
| LATEST = ["project_id", "-timestamp", "-event_id"] | ||
| OLDEST = ["project_id", "timestamp", "event_id"] | ||
| RECOMMENDED = [ | ||
| "-replay.id", | ||
| "-trace.sampled", | ||
| "num_processing_errors", | ||
| "-profile.id", | ||
| "-timestamp", | ||
| "-event_id", | ||
| ] | ||
|
|
||
|
|
||
| def get_oldest_or_latest_event( | ||
| group: Any, | ||
| ordering: EventOrdering, | ||
| conditions: Sequence[Condition] | None = None, | ||
| start: datetime | None = None, | ||
| end: datetime | None = None, | ||
| ) -> Any: | ||
| dataset = Dataset.IssuePlatform | ||
|
|
||
| all_conditions = [ | ||
| Condition(Column("project_id"), Op.IN, [group.project.id]), | ||
| Condition(Column("group_id"), Op.IN, [group.id]), | ||
| ] | ||
|
|
||
| if conditions: | ||
| all_conditions.extend(conditions) | ||
|
|
||
| events = eventstore.backend.get_events_snql( | ||
| organization_id=group.project.organization_id, | ||
| group_id=group.id, | ||
| start=start, | ||
| end=end, | ||
| conditions=all_conditions, | ||
| limit=1, | ||
| orderby=ordering.value, | ||
| referrer="Group.get_latest", | ||
| dataset=dataset, | ||
| tenant_ids={"organization_id": group.project.organization_id}, | ||
| ) | ||
|
|
||
| if events: | ||
| return events[0].for_group(group) | ||
|
|
||
| return None | ||
|
|
||
|
|
||
| def backfill_metric_issue_detectorgroup( | ||
| apps: StateApps, schema_editor: BaseDatabaseSchemaEditor | ||
| ) -> None: | ||
| """ | ||
| Backfill the DetectorGroup table for metric issues. | ||
| """ | ||
| Group = apps.get_model("sentry", "Group") | ||
| DetectorGroup = apps.get_model("workflow_engine", "DetectorGroup") | ||
| Detector = apps.get_model("workflow_engine", "Detector") | ||
|
|
||
| for group in Group.objects.filter(type=8001, detectorgroup__isnull=True).select_related( | ||
| "project" | ||
| ): # metric issues | ||
| # figure out the detector | ||
| latest_event = get_oldest_or_latest_event(group, EventOrdering.LATEST) | ||
cathteng marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if not latest_event: | ||
| logger.info("No latest event found for group", extra={"group_id": group.id}) | ||
| continue | ||
|
|
||
| occurrence = latest_event.occurrence | ||
| if not occurrence: | ||
| logger.info( | ||
| "No occurrence found for latest event", extra={"event_id": latest_event.event_id} | ||
| ) | ||
| continue | ||
|
|
||
| detector_id = occurrence.evidence_data.get("detector_id") | ||
| if detector_id is None: | ||
| logger.info( | ||
| "No detector id found for occurrence", extra={"occurrence_id": occurrence.id} | ||
| ) | ||
| continue | ||
|
|
||
| # try to fetch detector | ||
| detector = Detector.objects.filter(id=detector_id).first() | ||
| if detector is None: | ||
| DetectorGroup.objects.create( | ||
| group_id=group.id, | ||
| detector_id=None, | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Safe concurrent object creation prevents errors.Using |
||
| logger.info( | ||
| "Creating DetectorGroup with null detector", | ||
| extra={"group_id": group.id, "detector_id": detector_id}, | ||
| ) | ||
| continue | ||
|
|
||
| DetectorGroup.objects.create( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think technically Group is observable before DetectorGroup for a short period, so a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should error in that case because DetectorGroup is now unique on Group, but we can rerun in that case |
||
| group_id=group.id, | ||
| detector_id=detector.id, | ||
| ) | ||
| logger.info( | ||
| "Creating DetectorGroup", | ||
| extra={"group_id": group.id, "detector_id": detector_id}, | ||
| ) | ||
|
|
||
|
|
||
| class Migration(CheckedMigration): | ||
| # This flag is used to mark that a migration shouldn't be automatically run in production. | ||
| # This should only be used for operations where it's safe to run the migration after your | ||
| # code has deployed. So this should not be used for most operations that alter the schema | ||
| # of a table. | ||
| # Here are some things that make sense to mark as post deployment: | ||
| # - Large data migrations. Typically we want these to be run manually so that they can be | ||
| # monitored and not block the deploy for a long period of time while they run. | ||
| # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to | ||
| # run this outside deployments so that we don't block them. Note that while adding an index | ||
| # is a schema change, it's completely safe to run the operation after the code has deployed. | ||
| # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment | ||
|
|
||
| is_post_deployment = True | ||
|
|
||
| dependencies = [ | ||
| ("sentry", "1003_group_history_prev_history_safe_removal"), | ||
| ("workflow_engine", "0098_detectorgroup_detector_set_null"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython( | ||
| backfill_metric_issue_detectorgroup, | ||
| migrations.RunPython.noop, | ||
| hints={ | ||
| "tables": [ | ||
| "workflow_engine_detectorgroup", | ||
| "sentry_groupedmessage", | ||
| ] | ||
| }, | ||
| ), | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| from sentry.incidents.grouptype import MetricIssue | ||
| from sentry.incidents.models.alert_rule import AlertRuleDetectionType | ||
| from sentry.issues.ingest import save_issue_occurrence | ||
| from sentry.testutils.cases import TestMigrations | ||
| from sentry.testutils.helpers.datetime import before_now | ||
| from sentry.workflow_engine.models import Detector, DetectorGroup | ||
| from tests.sentry.issues.test_utils import OccurrenceTestMixin | ||
|
|
||
|
|
||
| class BackfillMetricIssueDetectorGroupTest(TestMigrations, OccurrenceTestMixin): | ||
| migrate_from = "0098_detectorgroup_detector_set_null" | ||
| migrate_to = "0099_backfill_metric_issue_detectorgroup" | ||
| app = "workflow_engine" | ||
|
|
||
| def setup_initial_state(self) -> None: | ||
| self.detector = Detector.objects.create( | ||
| project=self.project, | ||
| name="Test Detector", | ||
| type=MetricIssue.slug, | ||
| config={"detection_type": AlertRuleDetectionType.STATIC.value}, | ||
| ) | ||
|
|
||
| occurrence_data = self.build_occurrence_data( | ||
| event_id=self.event.event_id, | ||
| project_id=self.project.id, | ||
| fingerprint=[f"detector-{self.detector.id}"], | ||
| evidence_data={"detector_id": self.detector.id}, | ||
| type=MetricIssue.type_id, | ||
| ) | ||
|
|
||
| self.occurrence, group_info = save_issue_occurrence(occurrence_data, self.event) | ||
| assert group_info is not None | ||
| self.metric_issue = group_info.group | ||
|
|
||
| deleted_detector = Detector.objects.create( | ||
| project=self.project, | ||
| name="Test Detector 2", | ||
| type=MetricIssue.slug, | ||
| config={"detection_type": AlertRuleDetectionType.STATIC.value}, | ||
| ) | ||
| event = self.store_event( | ||
| data={ | ||
| "event_id": "b" * 32, | ||
| "timestamp": before_now(seconds=1).isoformat(), | ||
| }, | ||
| project_id=self.project.id, | ||
| ) | ||
| occurrence_data = self.build_occurrence_data( | ||
| event_id=event.event_id, | ||
| project_id=self.project.id, | ||
| fingerprint=[f"detector-{deleted_detector.id}"], | ||
| evidence_data={"detector_id": deleted_detector.id}, | ||
| type=MetricIssue.type_id, | ||
| ) | ||
|
|
||
| _, group_info = save_issue_occurrence(occurrence_data, event) | ||
| assert group_info is not None | ||
| self.metric_issue_deleted_detector = group_info.group | ||
| deleted_detector.delete() | ||
|
|
||
| self.metric_issue_no_occurrence = self.create_group( | ||
| project=self.project, type=MetricIssue.type_id | ||
| ) | ||
|
|
||
| self.metric_issue_existing_detectorgroup = self.create_group( | ||
| project=self.project, type=MetricIssue.type_id | ||
| ) | ||
| self.detector2 = Detector.objects.create( | ||
| project=self.project, | ||
| name="Test Detector 2", | ||
| type=MetricIssue.slug, | ||
| config={"detection_type": AlertRuleDetectionType.STATIC.value}, | ||
| ) | ||
| DetectorGroup.objects.all().delete() | ||
| DetectorGroup.objects.create( | ||
| group=self.metric_issue_existing_detectorgroup, | ||
| detector=self.detector2, | ||
| ) | ||
|
|
||
| def test_migration(self) -> None: | ||
| assert DetectorGroup.objects.filter( | ||
| group=self.metric_issue, detector=self.detector | ||
| ).exists() | ||
|
|
||
| assert DetectorGroup.objects.filter( | ||
| group=self.metric_issue_deleted_detector, detector=None | ||
| ).exists() | ||
|
|
||
| assert not DetectorGroup.objects.filter( | ||
| group=self.metric_issue_no_occurrence | ||
| ).exists() # does not exist because we should figure out what to do with this | ||
|
|
||
| assert DetectorGroup.objects.filter( | ||
| group=self.metric_issue_existing_detectorgroup, detector=self.detector2 | ||
| ).exists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a "copied from group.py with minimal edits" comment, otherwise the flexibility feels a bit unnecessary.