-
Notifications
You must be signed in to change notification settings - Fork 96
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
API to enable/disable a monitor #770
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: vikhy-aws <[email protected]>
…ate api in alerting Signed-off-by: vikhy-aws <[email protected]>
3053c64
to
32c7c84
Compare
} | ||
|
||
override fun validate(): ActionRequestValidationException? { | ||
return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this function about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an abstract method defined in the ActionRequest class, so need to provide an implementation here. Based on it's usage in other classes, it is used to validate the data members, which is not required in this case.
Signed-off-by: vikhy-aws <[email protected]>
4128a1f
to
e3805a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are any changes needed in the alerting plugin interface? It may be necessary for the security analytics plugin to enable/disable monitors.
https://github.com/opensearch-project/common-utils/blob/main/src/main/kotlin/org/opensearch/commons/alerting/AlertingPluginInterface.kt
this.version = version | ||
this.seqNo = seqNo | ||
this.primaryTerm = primaryTerm | ||
this.monitor = monitor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to consider excluding the monitor
from the response. Security analytics creates per document monitors that can have thousands of queries (a.k.a., rules) in them. That can make the response size pretty big, which may be extraneous for this kind of API. Perhaps we can instead just return the updated enabled
value, instead of the entire monitor? We can discuss with the team.
Description
This PR adds request and response classes to common-utils to create an API that enables or disables the monitor using the following:
PUT _plugins/_alerting/monitors/{monitor_id}/enable
PUT _plugins/_alerting/monitors/{monitor_id}/disable
Related Issues
Resolves opensearch-project/alerting#1058
Check List
--signoff
.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.