Chained Alert Behaviour Changes #1079

eirsep · 2023-08-04T22:17:30Z

Context:

Workflow/Composite monitor supports triggers whose conditions are configured as Boolean conditions with AND, OR, NOT
Operators and monitors as variables. For ex. Trigger condition M1 && M2 means trigger condition is matched if both monitors M1 and M2 create alerts (in AUDIT state) in the current execution. If workflow trigger condition is matched we create Chained Alert.

Current behavior

Alerts are created in ACTIVE state.
When alert are acknowledged they move to ACKNOWLEDGED state.

New behavior introduced with this PR

- If no ACTIVE/ACKNOWLEDGEDstate alert already exists for composite monitor trigger, we create a new ACTIVE state alert.
- If ACTIVE/ACKNOWLEDGED state alert already exists, we update that alert’s lastNotificationTime itself. But we do NOT create a new Alert. The pre-existing ACTIVE state alert will remain in ACTIVE state.
- When composite monitor trigger condition is NOT matched:
If ACTIVE/ACKNOWLEDGED state alert already exists, we would mark it as COMPLETED.

codecov · 2023-08-04T22:30:11Z

Codecov Report

Merging #1079 (e1a7de7) into main (778e7ce) will decrease coverage by 0.08%.
Report is 1 commits behind head on main.
The diff coverage is 68.62%.

❗ Current head e1a7de7 differs from pull request most recent head b41a77e. Consider uploading reports for the commit b41a77e to get more accurate results

@@             Coverage Diff              @@
##               main    #1079      +/-   ##
============================================
- Coverage     67.72%   67.65%   -0.08%     
  Complexity      105      105              
============================================
  Files           160      160              
  Lines         10343    10439      +96     
  Branches       1522     1545      +23     
============================================
+ Hits           7005     7062      +57     
- Misses         2672     2705      +33     
- Partials        666      672       +6

Files Changed	Coverage Δ
...org/opensearch/alerting/workflow/WorkflowRunner.kt	`1.13% <0.00%> (-0.03%)`	⬇️
...n/kotlin/org/opensearch/alerting/TriggerService.kt	`81.57% <50.00%> (-0.86%)`	⬇️
...ting/transport/TransportGetWorkflowAlertsAction.kt	`67.42% <66.66%> (-0.55%)`	⬇️
...earch/alerting/workflow/CompositeWorkflowRunner.kt	`72.01% <67.24%> (+1.24%)`	⬆️
...ain/kotlin/org/opensearch/alerting/AlertService.kt	`79.56% <71.95%> (-2.02%)`	⬇️
...ting/script/ChainedAlertTriggerExecutionContext.kt	`58.82% <100.00%> (-14.26%)`	⬇️

... and 3 files with indirect coverage changes

sbcd90 · 2023-08-04T23:52:12Z

alerting/src/main/kotlin/org/opensearch/alerting/AlertService.kt

+            else Alert.State.ERROR
+            Alert(
+                startTime = Instant.now(),
+                lastNotificationTime = Instant.now(),


can we use the same variable we defined in line 286?

sbcd90 · 2023-08-04T23:54:59Z

alerting/src/main/kotlin/org/opensearch/alerting/AlertService.kt

+
+        val queryBuilder = QueryBuilders.boolQuery()
+            .must(QueryBuilders.termQuery(Alert.WORKFLOW_ID_FIELD, workflowId))
+            .must(QueryBuilders.termQuery(Alert.MONITOR_ID_FIELD, ""))


can there be a doc which has both workflowId & monitorId field set with respective ids?

Alert created from composite monitor trigger will have only empty monitorId.

The underlying alerts created in AUDIT state will have both monitorId and worklfowId.

sbcd90 · 2023-08-05T00:01:27Z

alerting/src/main/kotlin/org/opensearch/alerting/script/ChainedAlertTriggerExecutionContext.kt

 ) {

    constructor(
        workflow: Workflow,
        workflowRunResult: WorkflowRunResult,
        trigger: ChainedAlertTrigger,
        alertGeneratingMonitors: Set<String>,
-        monitorIdToAlertIdsMap: Map<String, Set<String>>
+        monitorIdToAlertIdsMap: Map<String, Set<String>>,


can we make periodStart in the default constructor nullable & remove this constructor? Lot of similar fields between this one & the default one.

lezzago · 2023-08-11T20:48:31Z

alerting/src/main/kotlin/org/opensearch/alerting/AlertService.kt

+            .groupBy { it.triggerId }
+        foundAlerts.values.forEach { alerts ->
+            if (alerts.size > 1) {
+                logger.warn("Found multiple alerts for same trigger: $alerts")


should we try to resolve the older alert if this occurs?

this can be a follow up. But if this is not expected, we can ignore this unless we see issues for this.

lezzago · 2023-08-11T20:58:28Z

alerting/src/main/kotlin/org/opensearch/alerting/AlertService.kt

+                startTime = Instant.now(),
+                lastNotificationTime = currentTime,
+                state = Alert.State.ACTIVE,
+                errorMessage = null, schemaVersion = -1,


Should schema version be: IndexUtils.alertIndexSchemaVersion instead of -1?

lezzago · 2023-08-11T21:00:01Z

alerting/src/main/kotlin/org/opensearch/alerting/AlertService.kt

@@ -758,6 +845,37 @@ class AlertService(
        return searchResponse
    }

+    /**
+     * Searches for Alerts in the monitor's alertIndex.


Lets call out here that this will not retrieve audit alerts.

lezzago · 2023-08-11T21:00:51Z

alerting/src/main/kotlin/org/opensearch/alerting/script/ChainedAlertTriggerExecutionContext.kt

-    constructor(
-        workflow: Workflow,
-        workflowRunResult: WorkflowRunResult,
-        trigger: ChainedAlertTrigger,
-        alertGeneratingMonitors: Set<String>,
-        monitorIdToAlertIdsMap: Map<String, Set<String>>
-    ) :
-        this(
-            workflow,
-            workflowRunResult,
-            workflowRunResult.executionStartTime,
-            workflowRunResult.executionEndTime,
-            workflowRunResult.error,
-            trigger,
-            alertGeneratingMonitors,
-            monitorIdToAlertIdsMap
-        )


we dont need the constructor anymore?

the data class itself gives the default constructor
#1079 (comment)

lezzago · 2023-08-11T21:05:16Z

alerting/src/main/kotlin/org/opensearch/alerting/workflow/CompositeWorkflowRunner.kt

+            return workflowRunResult.copy(error = e)
+        }
+        try {
+            monitorCtx.alertIndices!!.createOrUpdateAlertIndex(dataSources)


Do we need this since we do this on line 154?

It's not necessary prolly, but I think this would be consistent practice to all other monitors where we ensure alertIndex is present before we fetch current alert.

* chained alert behavior changes Signed-off-by: Surya Sashank Nistala <[email protected]> * address comments Signed-off-by: Surya Sashank Nistala <[email protected]> * update comment for search alerts method Signed-off-by: Surya Sashank Nistala <[email protected]> --------- Signed-off-by: Surya Sashank Nistala <[email protected]>

Signed-off-by: Surya Sashank Nistala <[email protected]>

* chained alert behavior changes Signed-off-by: Surya Sashank Nistala <[email protected]> * address comments Signed-off-by: Surya Sashank Nistala <[email protected]> * update comment for search alerts method Signed-off-by: Surya Sashank Nistala <[email protected]> --------- Signed-off-by: Surya Sashank Nistala <[email protected]> (cherry picked from commit 0f2dec7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

opensearch-trigger-bot · 2023-08-25T23:09:23Z

The backport to 2.9 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/alerting/backport-2.9 2.9
# Navigate to the new working tree
pushd ../.worktrees/alerting/backport-2.9
# Create a new branch
git switch --create backport-1079-to-2.9
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0f2dec7cb660f232e05846a754d871246851e94e
# Push it to GitHub
git push --set-upstream origin backport-1079-to-2.9
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/alerting/backport-2.9

Then, create a pull request where the base branch is 2.9 and the compare/head branch is backport-1079-to-2.9.

* chained alert behavior changes * address comments * update comment for search alerts method --------- (cherry picked from commit 0f2dec7) Signed-off-by: Surya Sashank Nistala <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

eirsep requested review from lezzago, AWSHurneyt, sbcd90, getsaurabh02, praveensameneni, qreshi, bowenlan-amzn and rishabhmaurya as code owners August 4, 2023 22:17

eirsep added backport 2.x backport 2.9 backports PRs to 2.9 labels Aug 4, 2023

eirsep force-pushed the currentmain branch from daedaa5 to 68e7685 Compare August 4, 2023 23:04

sbcd90 reviewed Aug 4, 2023

View reviewed changes

sbcd90 reviewed Aug 5, 2023

View reviewed changes

eirsep force-pushed the currentmain branch 2 times, most recently from 472bf28 to e1a7de7 Compare August 7, 2023 22:27

sbcd90 approved these changes Aug 9, 2023

View reviewed changes

lezzago reviewed Aug 11, 2023

View reviewed changes

eirsep force-pushed the currentmain branch from 7b8a2cc to 150fbf3 Compare August 15, 2023 18:44

This was referenced Aug 16, 2023

(Backports #1079)Chained Alert Behaviour Changes #1091

Closed

Backports #1079 (Chained Alert Behaviour Changes ) #1092

Merged

eirsep added 3 commits August 25, 2023 14:27

chained alert behavior changes

837cca7

Signed-off-by: Surya Sashank Nistala <[email protected]>

address comments

584184f

Signed-off-by: Surya Sashank Nistala <[email protected]>

update comment for search alerts method

b41a77e

Signed-off-by: Surya Sashank Nistala <[email protected]>

eirsep force-pushed the currentmain branch from 150fbf3 to b41a77e Compare August 25, 2023 21:27

lezzago approved these changes Aug 25, 2023

View reviewed changes

AWSHurneyt approved these changes Aug 25, 2023

View reviewed changes

eirsep merged commit 0f2dec7 into opensearch-project:main Aug 25, 2023
13 of 14 checks passed

opensearch-trigger-bot bot mentioned this pull request Aug 25, 2023

[Backport 2.x] Chained Alert Behaviour Changes #1105

Merged

opensearch-trigger-bot bot added the failed backport label Aug 25, 2023

eirsep removed backport 2.9 backports PRs to 2.9 failed backport labels Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chained Alert Behaviour Changes #1079

Chained Alert Behaviour Changes #1079

eirsep commented Aug 4, 2023

codecov bot commented Aug 4, 2023 •

edited

Loading

sbcd90 Aug 4, 2023

eirsep Aug 7, 2023

sbcd90 Aug 4, 2023 •

edited

Loading

eirsep Aug 7, 2023

sbcd90 Aug 5, 2023

lezzago Aug 11, 2023

lezzago Aug 11, 2023

lezzago Aug 11, 2023

lezzago Aug 11, 2023

lezzago Aug 11, 2023

eirsep Aug 15, 2023

lezzago Aug 11, 2023

eirsep Aug 15, 2023

opensearch-trigger-bot bot commented Aug 25, 2023

Chained Alert Behaviour Changes #1079

Chained Alert Behaviour Changes #1079

Conversation

eirsep commented Aug 4, 2023

Context:

Current behavior

New behavior introduced with this PR

codecov bot commented Aug 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbcd90 Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Aug 25, 2023

codecov bot commented Aug 4, 2023 •

edited

Loading

sbcd90 Aug 4, 2023 •

edited

Loading