Skip to content
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

feat(aci): Add helpers to dual write to Actions and DataConditions #82492

Merged
merged 9 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 134 additions & 2 deletions src/sentry/workflow_engine/migration_helpers/alert_rule.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,157 @@

from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion
from sentry.incidents.grouptype import MetricAlertFire
from sentry.incidents.models.alert_rule import AlertRule
from sentry.incidents.models.alert_rule import (
AlertRule,
AlertRuleThresholdType,
AlertRuleTrigger,
AlertRuleTriggerAction,
)
from sentry.incidents.utils.types import DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION
from sentry.integrations.services.integration import integration_service
from sentry.snuba.models import QuerySubscription, SnubaQuery
from sentry.users.services.user import RpcUser
from sentry.workflow_engine.models import (
Action,
AlertRuleDetector,
AlertRuleTriggerDataCondition,
AlertRuleWorkflow,
DataCondition,
DataConditionGroup,
DataConditionGroupAction,
DataSource,
Detector,
DetectorState,
DetectorWorkflow,
Workflow,
WorkflowDataConditionGroup,
)
from sentry.workflow_engine.models.data_condition import Condition
from sentry.workflow_engine.types import DetectorPriorityLevel

logger = logging.getLogger(__name__)


def get_action_type(alert_rule_trigger_action: AlertRuleTriggerAction) -> str | None:
if alert_rule_trigger_action.sentry_app_id:
return Action.Type.SENTRY_APP

elif alert_rule_trigger_action.integration_id:
integration = integration_service.get_integration(
integration_id=alert_rule_trigger_action.integration_id
)
if not integration:
return None
try:
return Action.Type(integration.provider)
except Exception:
return None
else:
return Action.Type.EMAIL


def migrate_metric_action(
alert_rule_trigger_action: AlertRuleTriggerAction,
) -> tuple[Action, DataConditionGroupAction] | None:
try:
alert_rule_trigger_data_condition = AlertRuleTriggerDataCondition.objects.get(
alert_rule_trigger=alert_rule_trigger_action.alert_rule_trigger
)
except AlertRuleTriggerDataCondition.DoesNotExist:
logger.exception(
"AlertRuleTriggerDataCondition does not exist",
extra={"alert_rule_trigger_id": alert_rule_trigger_action.alert_rule_trigger.id},
)
return None

action_type = get_action_type(alert_rule_trigger_action)
if not action_type:
logger.warning(
"Could not find a matching Action.Type for the trigger action",
extra={"alert_rule_trigger_action_id": alert_rule_trigger_action.id},
)
return None

data = {
"type": alert_rule_trigger_action.type,
"sentry_app_id": alert_rule_trigger_action.sentry_app_id,
"sentry_app_config": alert_rule_trigger_action.sentry_app_config,
}
action = Action.objects.create(
required=False,
type=action_type,
data=data,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does Action.data need json schema validation too? it's a JSONField cc @iamrajjoshi

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah - i think so too

also @iamrajjoshi - if this uses the superset schema we could ensure that the migration includes the mappings too.

integration_id=alert_rule_trigger_action.integration_id,
target_display=alert_rule_trigger_action.target_display,
target_identifier=alert_rule_trigger_action.target_identifier,
target_type=alert_rule_trigger_action.target_type,
)
data_condition_group_action = DataConditionGroupAction.objects.create(
condition_group_id=alert_rule_trigger_data_condition.data_condition.condition_group.id,
action_id=action.id,
)
return action, data_condition_group_action


def migrate_metric_data_condition(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to migrate the AlertRule's resolve_threshold if it's not null.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like as it's own DataCondition?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so

Copy link
Member Author

@ceorourke ceorourke Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok just updated 👍 I left a comment but I can't make an AlertRuleTriggerDataCondition like we do for the other triggers because it's not really a trigger, but I'm not sure yet whether that's a problem. If it is, we'll want to add another column to that table to map to the alert rule id.

alert_rule_trigger: AlertRuleTrigger,
) -> tuple[DataCondition, AlertRuleTriggerDataCondition] | None:
alert_rule = alert_rule_trigger.alert_rule

data_condition_group = DataConditionGroup.objects.create(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to connect this DataConditionGroup with the workflow somehow (via WorkflowDataConditionGroup maybe)? Or am I misinterpreting the comment on the dual write helpers?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 yeah, using the WorkflowDataConditionGroup creates the association. This allows us to go form detector -> workflow -> workflow actions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted with Josh: we want to create entries in the WorkflowDataConditionGroup table for each DCG. Connect the alert rule's associated workflow with the DCG.

organization_id=alert_rule.organization_id
)
alert_rule_workflow = AlertRuleWorkflow.objects.get(alert_rule=alert_rule)
WorkflowDataConditionGroup.objects.create(
condition_group=data_condition_group,
workflow=alert_rule_workflow.workflow,
)
data_condition = DataCondition.objects.create(
comparison=(
DetectorPriorityLevel.MEDIUM
if alert_rule_trigger.label == "warning"
else DetectorPriorityLevel.HIGH
),
condition_result=True,
type=Condition.ISSUE_PRIORITY_EQUALS,
condition_group=data_condition_group,
)
alert_rule_trigger_data_condition = AlertRuleTriggerDataCondition.objects.create(
alert_rule_trigger=alert_rule_trigger, data_condition=data_condition
)
return data_condition, alert_rule_trigger_data_condition


def migrate_resolve_threshold_data_condition(alert_rule: AlertRule) -> DataCondition:
"""
Create data conditions for rules with a resolve threshold
"""
data_condition_group = DataConditionGroup.objects.create(
organization_id=alert_rule.organization_id
)
alert_rule_workflow = AlertRuleWorkflow.objects.get(alert_rule=alert_rule)
WorkflowDataConditionGroup.objects.create(
condition_group=data_condition_group,
workflow=alert_rule_workflow.workflow,
)
threshold_type = (
Condition.LESS
if alert_rule.threshold_type == AlertRuleThresholdType.ABOVE.value
else Condition.GREATER
)
# XXX: we set the resolve trigger's threshold_type to whatever the opposite of the rule's threshold_type is
# e.g. if the rule has a critical trigger ABOVE some number, the resolve threshold is automatically set to BELOW

data_condition = DataCondition.objects.create(
comparison=alert_rule.resolve_threshold,
condition_result=DetectorPriorityLevel.OK,
type=threshold_type,
condition_group=data_condition_group,
)
# XXX: can't make an AlertRuleTriggerDataCondition since this isn't really a trigger
return data_condition


def create_metric_alert_lookup_tables(
alert_rule: AlertRule,
detector: Detector,
Expand Down Expand Up @@ -132,7 +264,7 @@ def migrate_alert_rule(
detector_state = DetectorState.objects.create(
detector=detector,
active=False,
state=DetectorPriorityLevel.OK,
state=DetectorPriorityLevel.OK, # TODO this should be determined based on whether or not the rule has an active incident
)
alert_rule_detector, alert_rule_workflow, detector_workflow = create_metric_alert_lookup_tables(
alert_rule, detector, workflow
Expand Down
16 changes: 15 additions & 1 deletion src/sentry/workflow_engine/models/action.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,23 @@ class Action(DefaultFieldsModel):
__repr__ = sane_repr("id", "type")

class Type(models.TextChoices):
EMAIL = "email"
SLACK = "slack"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TYTY

MSTEAMS = "msteams"
DISCORD = "discord"

PAGERDUTY = "pagerduty"
OPSGENIE = "opsgenie"

GITHUB = "github"
GITHUB_ENTERPRISE = "github_enterprise"
JIRA = "jira"
JIRA_SERVER = "jira_server"
AZURE_DEVOPS = "azure_devops"

EMAIL = "email"
SENTRY_APP = "sentry_app"

PLUGIN = "plugin"
WEBHOOK = "webhook"

# The type field is used to denote the type of action we want to trigger
Expand Down
1 change: 1 addition & 0 deletions src/sentry/workflow_engine/models/data_condition.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class Condition(models.TextChoices):
REGRESSION_EVENT = "regression_event"
REAPPEARED_EVENT = "reappeared_event"
TAGGED_EVENT = "tagged_event"
ISSUE_PRIORITY_EQUALS = "issue_priority_equals"


condition_ops = {
Expand Down
Loading
Loading