add should_create_single_alert_for_findings field to security-analytics #757
Conversation
Signed-off-by: Subhobrata Dey <[email protected]>
Signed-off-by: Subhobrata Dey <[email protected]>
Signed-off-by: Subhobrata Dey <[email protected]>
sbcd90
requested review from
lezzago,
bowenlan-amzn,
rishabhmaurya,
getsaurabh02,
eirsep,
AWSHurneyt,
engechas,
riysaxen-amzn and
jowg-amazon
as code owners
| @@ -21,7 +21,8 @@ data class IndexExecutionContext( | |||
| val updatedIndexNames: List<String>, | |||
| val concreteIndexNames: List<String>, | |||
| val conflictingFields: List<String>, | |||
| val docIds: List<String>? = emptyList() | |||
| val docIds: List<String>? = emptyList(), | |||
| val findingIds: List<String>? = emptyList() | |||
There was a problem hiding this comment.
just to add findingIds to IndexExecutionContext.
| @@ -43,6 +43,7 @@ data class Monitor( | |||
| val uiMetadata: Map<String, Any>, | |||
| val dataSources: DataSources = DataSources(), | |||
| val deleteQueryIndexInEveryRun: Boolean? = false, | |||
| val shouldPersistFindingsAndAlerts: Boolean? = false, | |||
There was a problem hiding this comment.
shouldn't this be true by default?
There was a problem hiding this comment.
the field is named now as shouldCreateSingleAlertForFindings. & its set to False by default.
| @@ -332,6 +338,11 @@ data class Monitor( | |||
| } else { | |||
| xcp.booleanValue() | |||
| } | |||
| SHOULD_PERSIST_FINDINGS_AND_ALERTS_FIELD -> delegateMonitor = if (xcp.currentToken() == XContentParser.Token.VALUE_NULL) { | |||
There was a problem hiding this comment.
thinking of a scenario where this field is null
shouldn't we persist alerts and findings by default but we have initilalized var delegateMonitor = false and wont end up persisting
There was a problem hiding this comment.
same as #757 (comment)
| @@ -17,7 +17,7 @@ data class WorkflowRunContext( | |||
| val workflowId: String, | |||
| val workflowMetadataId: String, | |||
| val chainedMonitorId: String?, | |||
| val matchingDocIdsPerIndex: Map<String, List<String>>, | |||
| val matchingDocIdsPerIndex: Pair<Map<String, List<String>>, List<String>>, | |||
There was a problem hiding this comment.
is this change backward compatible?
can we revert this?
|
this PR seems to be making multiple minor changes and additions |
+1 is there a github issue or something we can track to see why we're making these changes? |
|
as discussed we seem to be creating a single alert for all findings |
Signed-off-by: Subhobrata Dey <[email protected]>
only |
Signed-off-by: Subhobrata Dey <[email protected]>
sbcd90
changed the title
| @@ -43,6 +43,7 @@ data class Monitor( | |||
| val uiMetadata: Map<String, Any>, | |||
| val dataSources: DataSources = DataSources(), | |||
| val deleteQueryIndexInEveryRun: Boolean? = false, | |||
| val shouldCreateSingleAlertForFindings: Boolean? = false, | |||
There was a problem hiding this comment.
Since alerts are generated by triggers, would it be worthwhile to make this configurable at the trigger-level instead of the monitor-level?
There was a problem hiding this comment.
Description
findingIdstoIndexExecutionContext. https://github.com/opensearch-project/common-utils/pull/757/files#diff-74f48ba337f4ed529bbc3b631ff66bf2d2e95a657c6238b1ecf018785db97f50R25should_create_single_alert_for_findingsfield to security-analytics https://github.com/opensearch-project/common-utils/pull/757/files#diff-74f48ba337f4ed529bbc3b631ff66bf2d2e95a657c6238b1ecf018785db97f50R25findingIdstoWorkflowRunContext. https://github.com/opensearch-project/common-utils/pull/757/files#diff-d955843c0c82617b06bce8aa4393c1d947e6930202daaf84c022e5c1ea2dba10R22continuing from pr add ignore_findings_and_alerts field to monitors #756
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
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.