moving over data models from alerting to common-utils by sbcd90 · Pull Request #227 · opensearch-project/common-utils

sbcd90 · 2022-08-30T19:46:26Z

Signed-off-by: Subhobrata Dey sbcd90@gmail.com

Description

moving over data models from alerting to common-utils.
this will help in making alerting a common framework for other plugins to be able to create and execute monitors.

Issues Resolved

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --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.

getsaurabh02 · 2022-08-30T21:22:51Z

+import org.opensearch.commons.utils.BaseResponse
+import org.opensearch.commons.utils.recreateObject
+
+object AlertingPluginInterface {


nit: Let's add Java doc with the intent that all the transport action interfaces for Alerting Plugin should be added here.

added javadoc

getsaurabh02 · 2022-08-30T21:27:21Z

+            AlertingActions.INDEX_MONITOR_ACTION_TYPE,
+            request,
+            wrapActionListener(listener) { response ->
+                recreateObject(response) {


is reified necessary here ?

this operation is needed when making inter-plugin transport requests. Otherwise, we get into this known issue: https://discuss.elastic.co/t/how-to-share-classloader-bewteen-different-plugins/181076/6
Alerting to Notifications follow similar pattern.

getsaurabh02 · 2022-08-30T21:28:17Z

+        )
+    }
+
+    @Suppress("UNCHECKED_CAST")


lets remove uncheck casting

this check suppresses 2 warnings. So, will keep it as Alerting to Notifications follow similar pattern.

getsaurabh02 · 2022-08-30T21:36:21Z

+        // Ensure that trigger ids are unique within a monitor
+        val triggerIds = mutableSetOf<String>()
+        triggers.forEach { trigger ->
+            require(triggerIds.add(trigger.id)) { "Duplicate trigger id: ${trigger.id}. Trigger ids must be unique." }
+            // Verify Trigger type based on Monitor type
+            when (monitorType) {
+                MonitorType.QUERY_LEVEL_MONITOR ->
+                    require(trigger is QueryLevelTrigger) { "Incompatible trigger [${trigger.id}] for monitor type [$monitorType]" }
+                MonitorType.BUCKET_LEVEL_MONITOR ->
+                    require(trigger is BucketLevelTrigger) { "Incompatible trigger [${trigger.id}] for monitor type [$monitorType]" }
+                MonitorType.CLUSTER_METRICS_MONITOR ->
+                    require(trigger is QueryLevelTrigger) { "Incompatible trigger [${trigger.id}] for monitor type [$monitorType]" }
+                MonitorType.DOC_LEVEL_MONITOR ->
+                    require(trigger is DocumentLevelTrigger) { "Incompatible trigger [${trigger.id}] for monitor type [$monitorType]" }


Can we take an attempt and decouple the Monitor model to separate out the pojo and execution logic. That way we can have a simpler (leaner) model here, required for consumers, whole the core logic is still held by the Alerting.
This will prevent unnecessary dependencies being pulled in such as BucketLevelTriggers and BucketSelectorExt.

I am open to doing this incrementally as long we can add ToDos and open a github issue to do this as a followup

getsaurabh02 · 2022-08-30T21:38:33Z

+import java.io.IOException
+import java.time.Instant
+
+interface ScheduledJob : BaseModel {


Similar to previous comment, looks like some of these could be avoided if we could segregate code and structure in the monitor model

lezzago · 2022-09-02T15:24:11Z

This commit should be named something not specific to security analytics. In the description, it can be mentioned this is for security analytics to use the Alerting plugin. The commit name can be like Move Alerting data models over to common-utils.

getsaurabh02

Thanks @sbcd90 . Couple of notes:

Lets update the intent of the PR and link with an Alerting issue - To make alerting as a common framework for other plugins to be able to create and execute monitors.
Open an quick issue for refactoring if not being addressed in this PR - As the heavy objects here can be modeled as lightweight pojo to decouple the logic.

sbcd90 · 2022-09-07T01:06:45Z

Thanks @sbcd90 . Couple of notes:

Lets update the intent of the PR and link with an Alerting issue - To make alerting as a common framework for other plugins to be able to create and execute monitors.

Open an quick issue for refactoring if not being addressed in this PR - As the heavy objects here can be modeled as lightweight pojo to decouple the logic.

updated the intent.
created a new issue for refactoring. [FEATURE]Refactor data model migration from alerting to common-utils #236

sbcd90 · 2022-09-07T01:07:17Z

This commit should be named something not specific to security analytics. In the description, it can be mentioned this is for security analytics to use the Alerting plugin. The commit name can be like Move Alerting data models over to common-utils.

updated commit message.

Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>

sbcd90 requested a review from a team August 30, 2022 19:46

sbcd90 force-pushed the alerting_integ_final branch from ef01686 to 2bad41e Compare August 30, 2022 20:05

sbcd90 requested a review from getsaurabh02 August 30, 2022 20:09

sbcd90 force-pushed the alerting_integ_final branch from 2bad41e to 65c9270 Compare August 30, 2022 20:13

getsaurabh02 reviewed Aug 30, 2022

View reviewed changes

sbcd90 force-pushed the alerting_integ_final branch from 65c9270 to 5f21125 Compare August 31, 2022 23:11

sbcd90 mentioned this pull request Aug 31, 2022

moving over data models from alerting to common-utils opensearch-project/alerting#535

Closed

sbcd90 force-pushed the alerting_integ_final branch from 5f21125 to dd45de0 Compare September 1, 2022 16:09

sbcd90 changed the title ~~security-analytics alerting integration via common-utils~~ moving over data models from alerting to common-utils Sep 2, 2022

sbcd90 force-pushed the alerting_integ_final branch from dd45de0 to 1f58401 Compare September 2, 2022 00:49

getsaurabh02 previously approved these changes Sep 7, 2022

View reviewed changes

sbcd90 mentioned this pull request Sep 7, 2022

[FEATURE]Refactor data model migration from alerting to common-utils #236

Closed

sbcd90 dismissed getsaurabh02’s stale review via 2b03f87 September 7, 2022 00:46

sbcd90 force-pushed the alerting_integ_final branch from 1f58401 to 2b03f87 Compare September 7, 2022 00:46

getsaurabh02 previously approved these changes Sep 7, 2022

View reviewed changes

Move Alerting data models over to common-utils

aa15e70

Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>

sbcd90 dismissed getsaurabh02’s stale review via aa15e70 September 7, 2022 20:37

sbcd90 force-pushed the alerting_integ_final branch from 2b03f87 to aa15e70 Compare September 7, 2022 20:37

getsaurabh02 approved these changes Sep 7, 2022

View reviewed changes

lezzago approved these changes Sep 7, 2022

View reviewed changes

sbcd90 merged commit 40290ac into opensearch-project:main Sep 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

moving over data models from alerting to common-utils#227

moving over data models from alerting to common-utils#227
sbcd90 merged 1 commit intoopensearch-project:mainfrom
sbcd90:alerting_integ_final

sbcd90 commented Aug 30, 2022 •

edited

Loading

Uh oh!

getsaurabh02 Aug 30, 2022

Uh oh!

sbcd90 Sep 1, 2022

Uh oh!

getsaurabh02 Aug 30, 2022

Uh oh!

sbcd90 Sep 1, 2022

Uh oh!

getsaurabh02 Aug 30, 2022

Uh oh!

sbcd90 Sep 1, 2022

Uh oh!

getsaurabh02 Aug 30, 2022

Uh oh!

getsaurabh02 Aug 30, 2022

Uh oh!

lezzago commented Sep 2, 2022

Uh oh!

getsaurabh02 left a comment

Uh oh!

sbcd90 commented Sep 7, 2022

Uh oh!

sbcd90 commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

sbcd90 commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lezzago commented Sep 2, 2022

Uh oh!

getsaurabh02 left a comment

Choose a reason for hiding this comment

Uh oh!

sbcd90 commented Sep 7, 2022

Uh oh!

sbcd90 commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sbcd90 commented Aug 30, 2022 •

edited

Loading