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(alerts): ACI dual write alert rule helpers #82400

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

ceorourke
Copy link
Member

A smaller chunk of #81953 that creates the helper methods to migrate the AlertRule and not the AlertRuleTrigger or AlertRuleTriggerAction just yet.

@ceorourke ceorourke requested a review from a team December 19, 2024 19:51
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 19, 2024
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 92.68293% with 3 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ry/workflow_engine/migration_helpers/alert_rule.py 92.68% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #82400       +/-   ##
===========================================
+ Coverage   55.70%   80.43%   +24.73%     
===========================================
  Files        6168     7307     +1139     
  Lines      257140   322167    +65027     
  Branches    21011    21011               
===========================================
+ Hits       143231   259129   +115898     
+ Misses     113504    62633    -50871     
  Partials      405      405               



def create_data_source(
organization_id: int, snuba_query: SnubaQuery | None = None
Copy link
Member

Choose a reason for hiding this comment

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

seems weird but appears to be to appease typing

self.metric_alert = self.create_alert_rule()
self.rpc_user = user_service.get_user(user_id=self.user.id)

def test_create_metric_alert(self):
Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to test the resulting models in workflow engine like process_workflow_engine? doesn't have to be in this PR

@ceorourke ceorourke merged commit 526a059 into master Dec 20, 2024
50 checks passed
@ceorourke ceorourke deleted the ceorourke/dual-write-alert-rule branch December 20, 2024 19:42
detector_workflow = DetectorWorkflow.objects.get(detector=detector)
assert detector_workflow.workflow == workflow

workflow_data_condition_group = WorkflowDataConditionGroup.objects.get(workflow=workflow)
Copy link
Member

Choose a reason for hiding this comment

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

i think the when condition group doesn't need to exist in WorkflowDataConditionGroup, it's already attached to the workflow via workflow.when_condition_group. i believe this model is used for the IF condition groups

andrewshie-sentry pushed a commit that referenced this pull request Jan 2, 2025
A smaller chunk of #81953 that
creates the helper methods to migrate the `AlertRule` and not the
`AlertRuleTrigger` or `AlertRuleTriggerAction` just yet.

def create_data_condition_group(organization_id: int) -> DataConditionGroup:
return DataConditionGroup.objects.create(
logic_type=DataConditionGroup.Type.ANY,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need to set this since it's the default.

Comment on lines +131 to +133
data_condition_group = create_data_condition_group(organization_id)
workflow = create_workflow(alert_rule.name, organization_id, data_condition_group, user)
detector = create_detector(alert_rule, project.id, data_condition_group, user)
Copy link
Contributor

@saponifi3d saponifi3d Jan 6, 2025

Choose a reason for hiding this comment

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

You'll want to create many data condition groups here instead.

the first is for the detector, on the workflow_condition_group. This condition group generates state changes for the workflow to respond to.

Looking at a specific example...
image

In this image, is the Detector.workflow_condition_group. In this DataConditionGroup, we're configuring two DataConditions;

  • if 300 > {metric} then return Critical
  • if 300 <= {metric} then return OK

When the data is evaluated, a DetectorState object is created for the state change. That state is then passed back to process_detectors where the issue occurrence is created.


For the workflow data condition group(s) we'll have a few. The first one, workflow.when_condition_group is Null. This is Null, because we always want to evaluate a detector state change events.

Next, we'll want to create data condition groups for the actions for the metric alert (and eventually detector)
image

Each of these rows is a new Data Condition Group, so two groups in this screenshot.

Looking at the first row, the Data Condition would be the priority level; "critical status". This is allows us to check if the detectors state change (which is stored as an issue occurrence). if the state makes that change, then we'll trigger the associated action (send email to me).

In the processor, it will continue to evaluate all those conditions generating a list of actions to trigger based on the data condition groups associated action.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants