Adds support for using findings from list of monitors as input data for a monitor in workflow #1112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…or in workflow Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
eirsep
requested review from
lezzago,
AWSHurneyt,
sbcd90,
getsaurabh02,
praveensameneni,
qreshi,
bowenlan-amzn and
rishabhmaurya
as code owners
eirsep
changed the title
|
Please add the description piece for the PR |
Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
lezzago
changed the title
lezzago
reviewed
Sep 1, 2023
| try { | ||
| val existsResponse: IndicesExistsResponse = client.admin().indices().suspendUntil { | ||
| exists(IndicesExistsRequest(chainedMonitor.dataSources.findingsIndex).local(true), it) | ||
| exists(IndicesExistsRequest(dataSources.findingsIndex).local(true), it) | ||
| } | ||
| if (existsResponse.isExists == false) return emptyMap() | ||
| // Search findings index per monitor and workflow execution id |
There was a problem hiding this comment.
nitpick: update description since this is not for a single monitor anymore.
| @@ -38,17 +38,26 @@ class WorkflowService( | |||
| * Returns finding doc ids per index for the given workflow execution | |||
| * Used for pre-filtering the dataset in the case of creating a workflow with chained findings | |||
| * | |||
| * @param chainedMonitor Monitor that is previously executed | |||
| * @param chainedMonitors Monitor that is previously executed | |||
There was a problem hiding this comment.
nitpick: update comment to indicate Monitors that have previously executed.
| indexDoc(index, "2", testDoc2) | ||
|
|
||
| testTime = DateTimeFormatter.ISO_OFFSET_DATE_TIME.format(ZonedDateTime.now().truncatedTo(ChronoUnit.MILLIS)) | ||
| // Doesn't match |
There was a problem hiding this comment.
this would still match monitor 2, right?
| assertEquals(3, monitorsRunResults.size) | ||
| assertFindings(monitorResponse.id, customFindingsIndex1, 2, 2, listOf("1", "2")) | ||
| assertFindings(monitorResponse2.id, customFindingsIndex1, 2, 2, listOf("2", "3")) | ||
| assertFindings(monitorResponse3.id, customFindingsIndex1, 3, 3, listOf("1", "2", "3")) |
There was a problem hiding this comment.
Shouldn't monitor 3 only execute on document 2 since it takes an input from monitor 1 and 2 and they only both would generate findings from document 2?
There was a problem hiding this comment.
We would do union of 2 sets here not intersection.
Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
lezzago
approved these changes
Sep 2, 2023
sbcd90
approved these changes
Sep 2, 2023
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Sep 2, 2023
…or a monitor in workflow (#1112) * support using findings from list ofmonitors as input data for a monitor in workflow Signed-off-by: Surya Sashank Nistala <snistala@amazon.com> * update exception message assertion in test Signed-off-by: Surya Sashank Nistala <snistala@amazon.com> * fix code comments in tests and source code Signed-off-by: Surya Sashank Nistala <snistala@amazon.com> * fix grammar in code comment Signed-off-by: Surya Sashank Nistala <snistala@amazon.com> --------- Signed-off-by: Surya Sashank Nistala <snistala@amazon.com> (cherry picked from commit 7b9584c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
eirsep
pushed a commit
that referenced
this pull request
Sep 7, 2023
…or a monitor in workflow (#1112) (#1113) * support using findings from list ofmonitors as input data for a monitor in workflow * update exception message assertion in test * fix code comments in tests and source code * fix grammar in code comment --------- (cherry picked from commit 7b9584c) Signed-off-by: Surya Sashank Nistala <snistala@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds support for using a list monitor ids in chained monitor findings. This helps use input data for a monitor in workflow based on doc ids from findings of multiple monitors instead of a single monitor.
If workflow sequence looks like:
[m1] -> [m2] -> [m3 (chained findings of m1, m2)]
this means m3's data input would be the related doc ids from findings of m1 and m2.
In a given workflow execution the following scenarios may occur:
Scenario 1:
if m1 has findings related to docs 1,2,3 of "index1"
if m2 has findings related to docs 3,4,5 of "index1"
m3's source data would be docs 1,2,3,4,5 of index 1
Scenario 2:
if m1 has findings related to docs 1,2,3 of "index1"
if m2 has findings related to docs 3,4,5 of "index2"
m3's source data would be docs 1,2,3 of "index1" and "index2"
Scenario 3:
if m1 has findings related to docs 1,2,3 of "index1"
if m2 has no findings
m3's source data would be docs 1,2,3 of "index1"
for backward compatibility reasons, the monitorId field would be given preference over the new list type field, monitorIds, if both are provided.