Fix get alerts api to also return chained alerts with default params. and skip audit state alerts #1020

eirsep · 2023-07-13T01:17:42Z

Fix get alerts api to also return chained alerts with default params. and skip audit state alerts
If audit state alerts are required, need to pass monitor id, workflow id and state=audit in params.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…kip audit alerts Signed-off-by: Surya Sashank Nistala <[email protected]>

lezzago · 2023-07-13T01:28:15Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetAlertsAction.kt

+            // But if the API is called with query_params: state=AUDIT,monitor_id=<123>,workflow_id=<abc>, this api
+            // will return audit alerts generated by delegate monitor <123> in workflow <abc>


If we are still letting this API call get audit alerts, it should be included in ALL. It should not be just for the dashboard use case.

Otherwise, we should not let users see audit alerts through this API and only from the workflow alerts API

If not supporting it in ALL is temporary until the front end can fix it, then I am fine with going with this logic as long as we have an open github issue about it

AUDIT alerts is a state designed to ållow us to create events which we don't want to get users notified about. That's why they rely on chained alerts to be actionable(notifications/acknowledgement etc)

When we return alerts in the api it would clutter the response.

Created issue to discuss further and reconcile with front end #1021

I think we should make a call on if we want audit alerts here or not now.

lezzago · 2023-07-13T01:32:03Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetAlertsAction.kt

@@ -107,16 +115,23 @@ class TransportGetAlertsAction @Inject constructor(

        if (getAlertsRequest.monitorId != null) {
            queryBuilder.filter(QueryBuilders.termQuery("monitor_id", getAlertsRequest.monitorId))
+            if (getAlertsRequest.workflowIds.isNullOrEmpty()) {
+                val noWorklfowIdQuery = QueryBuilders.boolQuery()


noWorklfowIdQuery is misspelled.

lezzago · 2023-07-13T01:34:21Z

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

@@ -310,7 +310,7 @@ object CompositeWorkflowRunner : WorkflowRunner() {
                .should(boolQuery().mustNot(existsQuery(Alert.ERROR_MESSAGE_FIELD)))
                .should(termsQuery(Alert.ERROR_MESSAGE_FIELD, ""))
            queryBuilder.must(noErrorQuery)
-            searchRequest.source().query(queryBuilder)
+            searchRequest.source().query(queryBuilder).size(9999)


to make sure we fetch as many alerts as we can
we don't intend to evaluate with just 10 alerts which the default if no size is mentioned

We should be paginating through them then. Also isnt there a max of 25 delegate monitors to a workflow?

let's revisit this soon
#1022 and take a quick follow up.

…lerts and fetch only chained alerts Signed-off-by: Surya Sashank Nistala <[email protected]>

codecov · 2023-07-13T01:36:30Z

Codecov Report

Merging #1020 (de80840) into 2.x (3ba65f2) will increase coverage by 0.04%.
The diff coverage is 90.90%.

@@             Coverage Diff              @@
##                2.x    #1020      +/-   ##
============================================
+ Coverage     72.56%   72.60%   +0.04%     
  Complexity      119      119              
============================================
  Files           160      160              
  Lines         10424    10434      +10     
  Branches       1569     1571       +2     
============================================
+ Hits           7564     7576      +12     
+ Misses         1971     1964       -7     
- Partials        889      894       +5

Impacted Files	Coverage Δ
...rch/alerting/transport/TransportGetAlertsAction.kt	`73.98% <85.71%> (+0.07%)`	⬆️
.../org/opensearch/alerting/MonitorMetadataService.kt	`64.92% <100.00%> (ø)`
...ting/transport/TransportGetWorkflowAlertsAction.kt	`66.66% <100.00%> (+0.59%)`	⬆️
...earch/alerting/workflow/CompositeWorkflowRunner.kt	`70.76% <100.00%> (+0.51%)`	⬆️

... and 6 files with indirect coverage changes

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

lezzago · 2023-07-13T01:59:05Z

Approving for now, but we have to follow up on these issues called out.

If audit alerts should be retrieved in the get Alerts API (Revisit how get alerts API deals with AUDIT state alerts #1021) (@mnkugler)
Pagination on fetching alerts for workflow

… and skip audit state alerts (#1020) * fix get alerts api with default params to return chained alerts and skip audit alerts Signed-off-by: Surya Sashank Nistala <[email protected]> * fix alert state query filter for get workflow alerts to avoid audit alerts and fetch only chained alerts Signed-off-by: Surya Sashank Nistala <[email protected]> * fix typo Signed-off-by: Surya Sashank Nistala <[email protected]> --------- Signed-off-by: Surya Sashank Nistala <[email protected]> (cherry picked from commit 0add91f)

… and skip audit state alerts (#1020) (#1023) * fix get alerts api with default params to return chained alerts and skip audit alerts Signed-off-by: Surya Sashank Nistala <[email protected]> * fix alert state query filter for get workflow alerts to avoid audit alerts and fetch only chained alerts Signed-off-by: Surya Sashank Nistala <[email protected]> * fix typo Signed-off-by: Surya Sashank Nistala <[email protected]> --------- Signed-off-by: Surya Sashank Nistala <[email protected]> (cherry picked from commit 0add91f) Co-authored-by: Surya Sashank Nistala <[email protected]>

… and skip audit state alerts (#1020) * fix get alerts api with default params to return chained alerts and skip audit alerts Signed-off-by: Surya Sashank Nistala <[email protected]> * fix alert state query filter for get workflow alerts to avoid audit alerts and fetch only chained alerts Signed-off-by: Surya Sashank Nistala <[email protected]> * fix typo Signed-off-by: Surya Sashank Nistala <[email protected]> --------- Signed-off-by: Surya Sashank Nistala <[email protected]> (cherry picked from commit 0add91f)

… and skip audit state alerts (#1020) (#1056) * fix get alerts api with default params to return chained alerts and skip audit alerts Signed-off-by: Surya Sashank Nistala <[email protected]> * fix alert state query filter for get workflow alerts to avoid audit alerts and fetch only chained alerts Signed-off-by: Surya Sashank Nistala <[email protected]> * fix typo Signed-off-by: Surya Sashank Nistala <[email protected]> --------- Signed-off-by: Surya Sashank Nistala <[email protected]> (cherry picked from commit 0add91f) Co-authored-by: Surya Sashank Nistala <[email protected]>

eirsep added the backport 2.9 backports PRs to 2.9 label Jul 13, 2023

fix get alerts api with default params to return chained alerts and s…

8a0b349

…kip audit alerts Signed-off-by: Surya Sashank Nistala <[email protected]>

eirsep force-pushed the fixes2x branch from c53ff40 to 8a0b349 Compare July 13, 2023 01:19

lezzago reviewed Jul 13, 2023

View reviewed changes

fix alert state query filter for get workflow alerts to avoid audit a…

b2d9b46

…lerts and fetch only chained alerts Signed-off-by: Surya Sashank Nistala <[email protected]>

fix typo

de80840

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

lezzago approved these changes Jul 13, 2023

View reviewed changes

sbcd90 approved these changes Jul 13, 2023

View reviewed changes

eirsep merged commit 0add91f into opensearch-project:2.x Jul 13, 2023
12 of 16 checks passed

opensearch-trigger-bot bot mentioned this pull request Jul 13, 2023

[Backport 2.9] Fix get alerts api to also return chained alerts with default params. and skip audit state alerts #1023

Merged

prudhvigodithi mentioned this pull request Jul 13, 2023

[RELEASE] Release version 2.9.0 opensearch-project/opensearch-build#3616

Closed

42 tasks

lezzago added the backport main label Jul 26, 2023

opensearch-trigger-bot bot mentioned this pull request Jul 26, 2023

[Backport main] Fix get alerts api to also return chained alerts with default params. and skip audit state alerts #1056

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix get alerts api to also return chained alerts with default params. and skip audit state alerts #1020

Fix get alerts api to also return chained alerts with default params. and skip audit state alerts #1020

eirsep commented Jul 13, 2023

lezzago Jul 13, 2023 •

edited

Loading

lezzago Jul 13, 2023

eirsep Jul 13, 2023

eirsep Jul 13, 2023

lezzago Jul 13, 2023

lezzago Jul 13, 2023

eirsep Jul 13, 2023

lezzago Jul 13, 2023

eirsep Jul 13, 2023

lezzago Jul 13, 2023

eirsep Jul 13, 2023

codecov bot commented Jul 13, 2023 •

edited

Loading

lezzago commented Jul 13, 2023

		// But if the API is called with query_params: state=AUDIT,monitor_id=<123>,workflow_id=<abc>, this api
		// will return audit alerts generated by delegate monitor <123> in workflow <abc>

Fix get alerts api to also return chained alerts with default params. and skip audit state alerts #1020

Fix get alerts api to also return chained alerts with default params. and skip audit state alerts #1020

Conversation

eirsep commented Jul 13, 2023

lezzago Jul 13, 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

codecov bot commented Jul 13, 2023 • edited Loading

Codecov Report

lezzago commented Jul 13, 2023

lezzago Jul 13, 2023 •

edited

Loading

codecov bot commented Jul 13, 2023 •

edited

Loading