fix alert normalization #50

atammy-narmi · 2023-07-05T16:52:27Z

Description

Fix normalization of alerts and triggers. The new API for opensearch rearranged the JSON so we need to update the normalization code.

I took the trigger_types from https://github.com/opensearch-project/alerting-dashboards-plugin/blob/353aa258fb9a2f031964427a45da765092c7cf6d/public/pages/CreateTrigger/containers/CreateTrigger/utils/constants.js#L6

Issues Resolved

N/A

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.

phillbaker · 2023-07-10T01:10:48Z

provider/util.go

 	for _, t := range triggers {
 		if trigger, ok := t.(map[string]interface{}); ok {
 			delete(trigger, "id")
-
-			if actions, ok := trigger["actions"].([]interface{}); ok {
-				normalizeMonitorTriggerActions(actions)


Why are you removing this part of the normalization?

I didn't see it present in any of the alerts that we had in our system, so it seemed spurious to keep it there. Do you want me to add it back?

It was introduced here: phillbaker/terraform-provider-elasticsearch#164 can you see if that's still an issue?

Checked it out and it's not an issue with the newer opensearch monitors.

The opensearch alerting plugin changes aren't using the same syntax/representation as the elasticsearch, so we can delete this without worries.

prudhvigodithi · 2023-08-08T22:58:52Z

Hey @atammy-narmi we just merged the PR for provider OpenSearch 2.x support, can you please fetch upstream and modify the changes accordingly.
Thank you
@bbarani @peterzhuamazon

atammy-narmi · 2023-08-09T03:26:48Z

OK, I've rebased the PR.

prudhvigodithi · 2023-08-09T15:53:42Z

Hey @phillbaker can we move forward with this PR?
Thanks

prudhvigodithi · 2023-09-08T14:49:04Z

Hey @phillbaker just checking back, are we good to move forward with this PR?
Thank you

atammy-narmi · 2023-09-26T16:16:25Z

is there anything that's blocking this PR from being merged?

prudhvigodithi · 2023-09-26T21:22:46Z

Hey @atammy-narmi thanks for holding on this, does this change require for 2.x version of OpenSearch ? If need only for 1.x, please raise a PR to 1.x branch. Thank you
@peterzhuamazon @bbarani

atammy-narmi · 2023-09-26T21:36:54Z

Hey @atammy-narmi thanks for holding on this, does this change require for 2.x version of OpenSearch ? If need only for 1.x, please raise a PR to 1.x branch. Thank you @peterzhuamazon @bbarani

This works with the 2.x version of opensearch 😸

prudhvigodithi · 2023-09-26T22:16:01Z

This works with the 2.x version of opensearch 😸

Got it, then please change the base branch to 1.x as this an issue for only OpenSearch 1.x series. Thanks

atammy-narmi · 2023-09-26T22:35:07Z

This works with the 2.x version of opensearch 😸

Got it, then please change the base branch to 1.x as this an issue for only OpenSearch 1.x series. Thanks

No, I said it's for the the 2.x branch.

prudhvigodithi · 2023-09-26T22:59:24Z

No, I said it's for the the 2.x branch.

Do we need to backport this to 1.x?

atammy-narmi · 2023-09-27T11:41:56Z

No, I said it's for the the 2.x branch.

Do we need to backport this to 1.x?

I don't think so.

prudhvigodithi · 2023-10-05T19:40:16Z

Cool, thanks LGTM.
@atammy-narmi Can you please add some unit tests, we can ship this in next release.

atammy-narmi · 2023-10-17T17:25:31Z

I don't see any tests defined for other functions in the provider_test.go, could you point to where I should add the tests?

prudhvigodithi · 2023-10-17T17:29:08Z

I don't see any tests defined for other functions in the provider_test.go, could you point to where I should add the tests?

Hey @atammy-narmi may be it was started that way, would it be better to add in provider_test.go or create a file util_test.go, WDYT? Just want to ensure your change is tested before we the provider is shipped :)
Thank you

prudhvigodithi · 2023-10-26T20:44:58Z

Hey @atammy-narmi just following back on this PR? can you please add some unit tests so that we merge this PR.
Thanks

prudhvigodithi · 2023-11-01T20:59:48Z

Hey @atammy-narmi just following back again on this PR? can you please add some unit tests so that we merge this PR.
Thanks

atammy-narmi · 2023-11-01T22:18:31Z

Sorry, it will take me a couple more days, will try to get it done by next week 😄

rblcoder · 2023-11-14T18:57:55Z

@atammy-narmi Could you please create an issue and specify examples so that I can test and review the PR?

prudhvigodithi · 2024-01-04T17:17:01Z

Hey @atammy-narmi just following back on this PR? can you please add some unit tests so that we merge this PR.
Thanks

atammy-narmi · 2024-01-08T18:30:12Z

I've added something which looks like it should test this out.
I don't have a development environment setup for a local opensearch cluster, so I haven't managed to test this fully, can you approve the workflow so I can try testing this out?

atammy-narmi · 2024-01-09T01:44:16Z

Should be working now.

atammy-narmi · 2024-01-09T01:45:50Z

provider/resource_opensearch_monitor_test.go

@@ -71,52 +94,99 @@ func testCheckOpensearchMonitorDestroy(s *terraform.State) error {
 	return nil
 }

+var testAccOpensearchOpenDistroMonitorJSON = `


had to duplicate this cuz the linting was complaining if I did it the smart way by embedding the variable.

bbarani · 2024-01-22T21:54:43Z

@prudhvigodithi @phillbaker @rblcoder can we move forward on this PR?

prudhvigodithi · 2024-01-23T03:51:21Z

provider/util.go

@@ -55,17 +56,26 @@ func normalizeMonitor(tpl map[string]interface{}) {
 }

 func normalizeMonitorTriggers(triggers []interface{}) {
+	trigger_types := []string{"alerting_trigger", "anomaly_detector_trigger", "bucket_level_trigger", "document_level_trigger", "query_level_trigger"}
 	for _, t := range triggers {


Just checking is there a way identify all these actions using client actions rather than hardcoding ?

I see a new one chained_alert_trigger.

Sadly I don't know of any :(

prudhvigodithi · 2024-01-23T03:53:27Z

Thanks @atammy-narmi other than my above two comments the change LGTM.

rblcoder · 2024-01-26T13:08:37Z

@atammy-narmi @prudhvigodithi Can the code for normalizeMonitorTriggers be as follows? I checked, the tests are also passing.

func normalizeMonitorTriggers(triggers []interface{}) {
	// trigger_types := []string{"alerting_trigger", "anomaly_detector_trigger", "bucket_level_trigger", "chained_alert_trigger", "document_level_trigger", "query_level_trigger"}
	for _, t := range triggers {
		if trigger, ok := t.(map[string]interface{}); ok {
			delete(trigger, "id")
			var trigger_type_key string
			for key := range trigger {
				if strings.HasSuffix(key, "_trigger") {
					trigger_type_key = key
					break
				}
			}

			if qlt, ok := trigger[trigger_type_key].(map[string]interface{}); ok {
				normalizeMonitorLevelTriggers(qlt)
			}

		}
	}
}

atammy-narmi · 2024-01-26T13:29:10Z

While that seems OK with me, it seems to be relying implicitly on all triggers being named *_trigger. I don't have a problem with that, but might be worth checking if you want to introduce that kind of pattern.

Another is that this implementation assumes only one trigger, you could potentially have multiple triggers. Removing the break and moving the normalize to inside the block should work for that scenario.

rblcoder · 2024-01-27T08:15:18Z

@atammy-narmi I agree that the code above is not robust.
Adding links for sample json documents if anyone going through this issue later would like to refer -
https://opensearch.org/docs/latest/observing-your-data/alerting/per-cluster-metrics-monitors/#painless-triggers
https://opensearch.org/docs/latest/observing-your-data/alerting/composite-monitors/#example-request

atammy-narmi · 2024-02-14T23:00:22Z

Bump, can I ask whats the holdup now?

Signed-off-by: Abhinav Tamaskar <[email protected]>

prudhvigodithi · 2024-03-11T15:56:15Z

Thanks @atammy-narmi, @rblcoder since you are reviewing, can you please check and see if we are good to move forward.
Thanks

rblcoder · 2024-03-12T04:51:01Z

@atammy-narmi As mentioned in #50 (comment), can you try to avoid hard-coding the trigger types?
Can you create a REST API to get list of trigger types?
opensearch-project/alerting#1422

atammy-narmi requested review from peterzhuamazon, prudhvigodithi, jackson-theisen and phillbaker as code owners July 5, 2023 16:52

phillbaker reviewed Jul 10, 2023

View reviewed changes

prudhvigodithi mentioned this pull request Aug 4, 2023

[BUG] Alerting API throws 405 error opensearch-project/alerting#1078

Open

atammy-narmi force-pushed the alerts branch from 88d45bd to 5a877d1 Compare August 9, 2023 03:25

atammy-narmi mentioned this pull request Oct 17, 2023

opensearch_monitor reports changes in query_level_trigger.id and query_level_trigger.actions.*.id after creation #100

Open

atammy-narmi force-pushed the alerts branch from c8103ef to 60209bd Compare January 8, 2024 18:28

atammy-narmi force-pushed the alerts branch from 894c501 to 81c3427 Compare January 9, 2024 00:56

atammy-narmi force-pushed the alerts branch 6 times, most recently from 75bc90a to fd20db0 Compare January 9, 2024 01:38

atammy-narmi commented Jan 9, 2024

View reviewed changes

prudhvigodithi reviewed Jan 23, 2024

View reviewed changes

atammy-narmi force-pushed the alerts branch from fe2e75d to 0d66b97 Compare January 23, 2024 04:43

atammy-narmi added 5 commits March 4, 2024 12:12

fix alert normalization

4cc3b32

Signed-off-by: Abhinav Tamaskar <[email protected]>

format

6e14af3

Signed-off-by: Abhinav Tamaskar <[email protected]>

try adding some tests

6c931a3

Signed-off-by: Abhinav Tamaskar <[email protected]>

testing

a6b02a1

Signed-off-by: Abhinav Tamaskar <[email protected]>

chained_alert_trigger

d37115e

Signed-off-by: Abhinav Tamaskar <[email protected]>

atammy-narmi force-pushed the alerts branch from 0d66b97 to d37115e Compare March 4, 2024 17:12

fix alert normalization #50

Are you sure you want to change the base?

fix alert normalization #50

Conversation

atammy-narmi commented Jul 5, 2023

Description

Issues Resolved

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

prudhvigodithi commented Aug 8, 2023

atammy-narmi commented Aug 9, 2023

prudhvigodithi commented Aug 9, 2023

prudhvigodithi commented Sep 8, 2023

atammy-narmi commented Sep 26, 2023

prudhvigodithi commented Sep 26, 2023

atammy-narmi commented Sep 26, 2023

prudhvigodithi commented Sep 26, 2023

atammy-narmi commented Sep 26, 2023

prudhvigodithi commented Sep 26, 2023

atammy-narmi commented Sep 27, 2023

prudhvigodithi commented Oct 5, 2023 • edited Loading

atammy-narmi commented Oct 17, 2023

prudhvigodithi commented Oct 17, 2023

prudhvigodithi commented Oct 26, 2023

prudhvigodithi commented Nov 1, 2023

atammy-narmi commented Nov 1, 2023

rblcoder commented Nov 14, 2023

prudhvigodithi commented Jan 4, 2024

atammy-narmi commented Jan 8, 2024

atammy-narmi commented Jan 9, 2024

Choose a reason for hiding this comment

bbarani commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prudhvigodithi commented Jan 23, 2024

rblcoder commented Jan 26, 2024

atammy-narmi commented Jan 26, 2024 • edited Loading

rblcoder commented Jan 27, 2024 • edited Loading

atammy-narmi commented Feb 14, 2024

prudhvigodithi commented Mar 11, 2024

rblcoder commented Mar 12, 2024

prudhvigodithi commented Oct 5, 2023 •

edited

Loading

atammy-narmi commented Jan 26, 2024 •

edited

Loading

rblcoder commented Jan 27, 2024 •

edited

Loading