Adds findings in bucket level monitor #636
Merged
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
sbcd90
approved these changes
Nov 3, 2022
Signed-off-by: Surya Sashank Nistala <[email protected]>
Signed-off-by: Surya Sashank Nistala <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #636 +/- ##
============================================
- Coverage 76.35% 76.13% -0.23%
Complexity 116 116
============================================
Files 124 124
Lines 6445 6569 +124
Branches 942 972 +30
============================================
+ Hits 4921 5001 +80
- Misses 1044 1070 +26
- Partials 480 498 +18
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
getsaurabh02
reviewed
Nov 4, 2022
There was a problem hiding this comment.
Thanks @eirsep . Overall approach looks good to me. Few comments/clarifications to consider.
Comment on lines
+339
to
+341
| if (grouByFields > 0) { | ||
| return listOf() | ||
| } |
There was a problem hiding this comment.
Why do we need to return from here?
There was a problem hiding this comment.
If there are more groupby fields then aggregation buckets would become tuples. We can develop iteratively and first begin by supporting only for a single field(buckets with single field aggregated on) to filter on.
Comment on lines
410
to
411
| monitorCtx.client!!.suspendUntil<Client, IndexResponse> { | ||
| monitorCtx.client!!.index(indexRequest, it) |
There was a problem hiding this comment.
Are we indexing each finding document, one at a time? Wondering if we should use bulk request instead?
| findings.add(finding.id) | ||
| } | ||
| } | ||
| return findings |
There was a problem hiding this comment.
Do we need to return the findings from here, as indexing is already done?
There was a problem hiding this comment.
we are returning finding ids. finding ids list is populated in the alert.
| @@ -1321,6 +1323,71 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() { | |||
| ) | |||
| } | |||
|
|
|||
| fun `test bucket-level monitor with findings enabled`() { | |||
There was a problem hiding this comment.
Lets validate other scenarios as well:
- Where findings will not be created - such as compound aggregation.
- Findings index unavailable, or exception in index.
Signed-off-by: Surya Sashank Nistala <[email protected]>
sbcd90
approved these changes
Nov 5, 2022
getsaurabh02
approved these changes
Nov 5, 2022
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Nov 5, 2022
* bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> * add test to verify bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> * added tests. fixed document ids in bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> Signed-off-by: Surya Sashank Nistala <[email protected]> (cherry picked from commit 5b451b9)
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Nov 5, 2022
* bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> * add test to verify bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> * added tests. fixed document ids in bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> Signed-off-by: Surya Sashank Nistala <[email protected]> (cherry picked from commit 5b451b9)
eirsep
added a commit
that referenced
this pull request
Nov 6, 2022
* bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> * add test to verify bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> * added tests. fixed document ids in bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> Signed-off-by: Surya Sashank Nistala <[email protected]> (cherry picked from commit 5b451b9) Co-authored-by: Surya Sashank Nistala <[email protected]>
eirsep
added a commit
that referenced
this pull request
Nov 7, 2022
* bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> * add test to verify bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> * added tests. fixed document ids in bucket level monitor findings Signed-off-by: Surya Sashank Nistala <[email protected]> Signed-off-by: Surya Sashank Nistala <[email protected]> (cherry picked from commit 5b451b9) Co-authored-by: Surya Sashank Nistala <[email protected]>
Merged
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.
This change surfaces out the list of documents that become a part of bucket level monitor aggregation buckets as findings for the monitor.
Signed-off-by: Surya Sashank Nistala [email protected]
CheckList:
[x] 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.