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

Creates bucket level monitors for rules containing aggregations #92

Merged

Conversation

stevanbz
Copy link
Contributor

Description

Enables creation of bucket level monitors based on the aggregation rules.

Issues Resolved

[List any issues this PR will resolve]

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.

…c for creating bucket level monitors

Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
@stevanbz stevanbz requested a review from a team October 31, 2022 22:15
@eirsep eirsep marked this pull request as draft October 31, 2022 22:16
@stevanbz stevanbz force-pushed the feature/add-bucket-level-monitors branch 2 times, most recently from 357aaa0 to fd8c6f5 Compare November 2, 2022 15:40
@eirsep eirsep marked this pull request as ready for review November 2, 2022 17:24
Added integration tests for checking update of bucket level monitors

Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
@stevanbz stevanbz force-pushed the feature/add-bucket-level-monitors branch from 16b0781 to 658f353 Compare November 4, 2022 21:44
Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
…name

Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
// Build query string filter
.query(QueryBuilders.queryStringQuery(rule.getQueries().get(0).getValue()))
.aggregation(aggregationQueries.getAggBuilder())
.size(10000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

size(0)

@codecov-commenter
Copy link

Codecov Report

Merging #92 (9dfe138) into main (269be07) will decrease coverage by 0.92%.
The diff coverage is 14.88%.

@@             Coverage Diff              @@
##               main      #92      +/-   ##
============================================
- Coverage     40.71%   39.79%   -0.93%     
- Complexity      882      886       +4     
============================================
  Files           174      175       +1     
  Lines          6317     6551     +234     
  Branches        772      796      +24     
============================================
+ Hits           2572     2607      +35     
- Misses         3508     3704     +196     
- Partials        237      240       +3     
Impacted Files Coverage Δ
...a/org/opensearch/securityanalytics/model/Rule.java 0.00% <0.00%> (ø)
.../securityanalytics/rules/backend/QueryBackend.java 65.48% <ø> (ø)
...lytics/transport/TransportIndexDetectorAction.java 0.00% <0.00%> (ø)
...yanalytics/transport/TransportIndexRuleAction.java 0.00% <ø> (ø)
...ecurityanalytics/rules/backend/OSQueryBackend.java 66.37% <45.16%> (-8.91%) ⬇️
...tyanalytics/rules/backend/AggregationBuilders.java 50.00% <50.00%> (ø)
...g/opensearch/securityanalytics/model/Detector.java 70.62% <69.23%> (-0.65%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -90,6 +97,8 @@ public class Detector implements Writeable, ToXContentObject {

private List<String> monitorIds;

private Map<String, String> ruleIdMonitorId;
Copy link
Member

Choose a reason for hiding this comment

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

let's have the suffix map in variable name for better readability

mayberuleIdMonitorIdMap

@@ -73,7 +72,7 @@ public void testCountAggregationWithGroupBy() throws IOException, SigmaError {
String aggQuery = aggQueries.getAggQuery();
String bucketTriggerQuery = aggQueries.getBucketTriggerQuery();

Assert.assertEquals("\"aggs\":{\"result_agg\":{\"terms\":{\"field\":\"fieldB\"}}}", aggQuery);
Copy link
Member

Choose a reason for hiding this comment

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

how was this assert working if you needed to change the structuring of the fields

Copy link
Contributor Author

@stevanbz stevanbz Nov 7, 2022

Choose a reason for hiding this comment

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

You can see one line below that I just removed aggs part ie.
Assert.assertEquals("{"result_agg":{"terms":{"field":"fieldB"}}}", aggQuery);

aggregationBuilder = new ValueCountAggregationBuilder(name).field(name);
break;
default:
return null;
Copy link
Member

Choose a reason for hiding this comment

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

let's surface a not supported exception for better debugging.

Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
@eirsep eirsep changed the title WIP - Feature/add bucket level monitors Creates bucket level monitors for rules containing aggregations Nov 7, 2022
@eirsep
Copy link
Member

eirsep commented Nov 7, 2022

LGTM
but plz resolve merge conflicts
\

Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
@sbcd90 sbcd90 merged commit 2f0abe6 into opensearch-project:main Nov 9, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 9, 2022
Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
(cherry picked from commit 2f0abe6)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 9, 2022
Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
(cherry picked from commit 2f0abe6)
eirsep pushed a commit that referenced this pull request Nov 9, 2022
…#129)

Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
(cherry picked from commit 2f0abe6)

Co-authored-by: Stevan Buzejic <30922513+stevanbz@users.noreply.github.com>
eirsep pushed a commit that referenced this pull request Nov 9, 2022
…#130)

Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
(cherry picked from commit 2f0abe6)

Co-authored-by: Stevan Buzejic <30922513+stevanbz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants