Skip to content
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

initial commit for remote monitor support #1547

Merged
merged 13 commits into from
May 31, 2024

Conversation

sbcd90
Copy link
Collaborator

@sbcd90 sbcd90 commented May 15, 2024

Issue #, if available:
#1546

Description of changes:

CheckList:

  • 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.

}

@Override
public String getMonitorType() {
Copy link
Member

@eirsep eirsep May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plugins should be allowed to add multiple types of monitors.
SAP itself probably will implement

  • threat intel ioc scan monitor
  • ueba monitor
  • rules monitor
  • bucket level monitor with findings and pipeline aggs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@eirsep eirsep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we plz add the skeleton for a fan-out and monitor metadata capabilities of doc level monitor
also how to define new triggers? triggers seem tightly coupled with the existing defintions.

@@ -3,6 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import com.github.jengelman.gradle.plugins.shadow.ShadowPlugin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the same dependency used by jobscheduler or notification for shadow plugin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

@sbcd90 sbcd90 marked this pull request as ready for review May 22, 2024 03:47
@sbcd90 sbcd90 force-pushed the remote_monitor branch 2 times, most recently from 8317f29 to abcc7ce Compare May 22, 2024 03:57
@@ -185,10 +187,16 @@ object MonitorMetadataService :

suspend fun recreateRunContext(metadata: MonitorMetadata, monitor: Monitor): MonitorMetadata {
try {
val monitorIndex = if (monitor.monitorType == Monitor.MonitorType.DOC_LEVEL_MONITOR)
val monitorIndex = if (
monitor.isMonitorOfStandardType() &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just check string value is doc level monitor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just want to use a single string constant, so just reusing the enum value.

(monitor.inputs[0] as DocLevelMonitorInput).indices[0]
else null
val runContext = if (monitor.monitorType == Monitor.MonitorType.DOC_LEVEL_MONITOR && createWithRunContext)
val runContext = if (
monitor.isMonitorOfStandardType() &&
Copy link
Member

@eirsep eirsep May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract into method - isDocLevelMonitor()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


import org.opensearch.alerting.spi.RemoteMonitorRunner

class RemoteMonitorRegistry(val monitorType: String, val monitorRunner: RemoteMonitorRunner)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java docs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok updated it.

@sbcd90 sbcd90 marked this pull request as draft May 25, 2024 03:41
@sbcd90
Copy link
Collaborator Author

sbcd90 commented May 28, 2024

Signed-off-by: Subhobrata Dey <[email protected]>
@sbcd90 sbcd90 marked this pull request as ready for review May 28, 2024 18:50
Signed-off-by: Subhobrata Dey <[email protected]>
@@ -185,10 +185,10 @@ object MonitorMetadataService :

suspend fun recreateRunContext(metadata: MonitorMetadata, monitor: Monitor): MonitorMetadata {
try {
val monitorIndex = if (monitor.monitorType == Monitor.MonitorType.DOC_LEVEL_MONITOR)
val monitorIndex = if (monitor.monitorType.endsWith(Monitor.MonitorType.DOC_LEVEL_MONITOR.value))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just have a separate flag instead of depending on name

check can be monitor.monitorType == Monitor.MonitorType.DOC_LEVEL_MONITOR || "use_doc_level_feature_bool" == true ?

@@ -400,7 +402,9 @@ class TransportIndexWorkflowAction @Inject constructor(
log.warn("Metadata doc id:${monitorMetadata.id} exists, but it shouldn't!")
}

if (monitor.monitorType == Monitor.MonitorType.DOC_LEVEL_MONITOR) {
if (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can workflow add any kind of remote monitor?

@eirsep
Copy link
Member

eirsep commented May 29, 2024

how will index validate mapping of the custom trigger and custom input

@eirsep
Copy link
Member

eirsep commented May 29, 2024

@sbcd90
Copy link
Collaborator Author

sbcd90 commented May 30, 2024

@@ -240,6 +241,7 @@ internal class AlertingPlugin : PainlessExtension, ActionPlugin, ScriptPlugin, R
ClusterMetricsInput.XCONTENT_REGISTRY,
DocumentLevelTrigger.XCONTENT_REGISTRY,
ChainedAlertTrigger.XCONTENT_REGISTRY,
RemoteMonitorTrigger.XCONTENT_REGISTRY,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we also need RemoteMonitorInput xcontent registry?

if yes, can you add appropriate tests to serialize

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@eirsep eirsep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you check xcontent registry

Signed-off-by: Subhobrata Dey <[email protected]>
@sbcd90
Copy link
Collaborator Author

sbcd90 commented May 31, 2024

can you check xcontent registry

responded here. #1547 (comment)

Signed-off-by: Subhobrata Dey <[email protected]>
Signed-off-by: Subhobrata Dey <[email protected]>
Signed-off-by: Subhobrata Dey <[email protected]>
Signed-off-by: Subhobrata Dey <[email protected]>
@sbcd90
Copy link
Collaborator Author

sbcd90 commented May 31, 2024

all builds pass. merging pr.

@sbcd90 sbcd90 merged commit 5ed4db9 into opensearch-project:main May 31, 2024
18 checks passed
AWSHurneyt pushed a commit to AWSHurneyt/OpenSearch-Alerting that referenced this pull request Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants