Fix SearchMonitorsTool bugs; add corresponding ITs #151

ohltyler · 2024-01-26T19:20:25Z

Description

This PR fixes few bugs related to the SearchMonitorsTool:

result parsing bug where monitor fields being null due to discrepancy on alerting plugin's transport vs. REST action for the search monitor API. TLDR is the REST action does post-processing by removing the nested monitor field, hence we need to do the same thing in the tool.
search by monitorID bug due to classloading issue with the getMonitor() fn via common-utils. Simplified the tool to only use the search monitors transport action which improves maintainability as well

Also fixes a bug in SearchAnomalyDetectorsTool where the mapping should be detector_type instead of type.

Also adds several IT cases to cover the bugs. Completes part of #136

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.

Signed-off-by: Tyler Ohlsen <[email protected]>

ohltyler · 2024-01-26T19:54:22Z

Known failures from neural sparse tool. Added IT and all UT pass on CI and locally.

30 tests completed, 2 failed
Tests with failures:
 - org.opensearch.integTest.NeuralSparseSearchToolIT.testNeuralSparseSearchToolInFlowAgent_withIllegalEmbeddingField_thenThrowException
 - org.opensearch.integTest.NeuralSparseSearchToolIT.testNeuralSparseSearchToolInFlowAgent_withIllegalIndexField_thenThrowException

codecov · 2024-01-26T20:06:06Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (d7945d5) 81.69% compared to head (fb5ea3a) 81.73%.

Files	Patch %	Lines
...org/opensearch/agent/tools/SearchMonitorsTool.java	88.63%	5 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #151      +/-   ##
============================================
+ Coverage     81.69%   81.73%   +0.04%     
+ Complexity      199      196       -3     
============================================
  Files            13       13              
  Lines          1027     1002      -25     
  Branches        133      132       -1     
============================================
- Hits            839      819      -20     
+ Misses          138      133       -5     
  Partials         50       50

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/test/java/org/opensearch/integTest/SearchMonitorsToolIT.java

lezzago · 2024-01-26T20:38:00Z

src/main/java/org/opensearch/agent/tools/SearchMonitorsTool.java

+            mustList.add(new TermQueryBuilder("_id", monitorId));
+        }
+        if (monitorName != null) {
+            mustList.add(new TermQueryBuilder("monitor.name.keyword", monitorName));
+        }
+        if (monitorNamePattern != null) {
+            mustList.add(new WildcardQueryBuilder("monitor.name.keyword", monitorNamePattern));
+        }
+        if (enabled != null) {
+            mustList.add(new TermQueryBuilder("monitor.enabled", enabled));
+        }
+        if (hasTriggers != null) {
+            NestedQueryBuilder nestedTriggerQuery = new NestedQueryBuilder(
+                "monitor.triggers",
+                new ExistsQueryBuilder("monitor.triggers"),
+                ScoreMode.None
+            );
+
+            BoolQueryBuilder triggerQuery = new BoolQueryBuilder();
+            if (hasTriggers) {
+                triggerQuery.must(nestedTriggerQuery);
+            } else {
+                triggerQuery.mustNot(nestedTriggerQuery);


If we are adding these filters to find the monitors, I think it would be beneficial to also search by monitor type as well.

This PR isn't adding any filters, it's just shown as updated due to tabbing since the overall if/else was removed. Prefer to leave the tool with the current set of params and add/remove/tune later on as more testing is done and we capture the breadth of questions and functional responses.

Especially for initial release, we need to be weary about how many knobs we allow the LLM to tune. Ideally we cover as many questions as possible in a confident/consistent/accurate way, and slowly adjust or expand over time after we have a good gauge of performance.

Fair enough, we can set this as a further enhancement, down the road.

Signed-off-by: Tyler Ohlsen <[email protected]>

ohltyler · 2024-01-27T00:42:48Z

^ latest commit fixes a bug found in integ tests where the mapping is detector_type instead of type, causing null values set for it. This commit updates that.

* Fix search monitor bugs; add search monitor ITs Signed-off-by: Tyler Ohlsen <[email protected]> * Remove unused fn Signed-off-by: Tyler Ohlsen <[email protected]> * Clean up UT Signed-off-by: Tyler Ohlsen <[email protected]> * Change to beforeEach Signed-off-by: Tyler Ohlsen <[email protected]> * Fix detector_type bug Signed-off-by: Tyler Ohlsen <[email protected]> --------- Signed-off-by: Tyler Ohlsen <[email protected]> (cherry picked from commit 722bfd2) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Fix search monitor bugs; add search monitor ITs * Remove unused fn * Clean up UT * Change to beforeEach * Fix detector_type bug --------- (cherry picked from commit 722bfd2) Signed-off-by: Tyler Ohlsen <[email protected]> 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>

…t#151) * Fix search monitor bugs; add search monitor ITs Signed-off-by: Tyler Ohlsen <[email protected]> * Remove unused fn Signed-off-by: Tyler Ohlsen <[email protected]> * Clean up UT Signed-off-by: Tyler Ohlsen <[email protected]> * Change to beforeEach Signed-off-by: Tyler Ohlsen <[email protected]> * Fix detector_type bug Signed-off-by: Tyler Ohlsen <[email protected]> --------- Signed-off-by: Tyler Ohlsen <[email protected]>

…t#151) (opensearch-project#153) * Fix search monitor bugs; add search monitor ITs * Remove unused fn * Clean up UT * Change to beforeEach * Fix detector_type bug --------- (cherry picked from commit 722bfd2) Signed-off-by: Tyler Ohlsen <[email protected]> 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> Signed-off-by: yuye-aws <[email protected]>

ohltyler added 2 commits January 26, 2024 19:09

Fix search monitor bugs; add search monitor ITs

671edad

Signed-off-by: Tyler Ohlsen <[email protected]>

Remove unused fn

0c41898

Signed-off-by: Tyler Ohlsen <[email protected]>

ohltyler requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, xinyual, amitgalitz, jackiehanyang, owaiskazi19, joshpalis, dbwiddis, kaituo, lezzago, eirsep, sbcd90, zhichao-aws and joshuali925 as code owners January 26, 2024 19:20

opensearch-trigger-bot bot added the backport 2.x label Jan 26, 2024

Clean up UT

42d5e33

Signed-off-by: Tyler Ohlsen <[email protected]>

lezzago reviewed Jan 26, 2024

View reviewed changes

src/test/java/org/opensearch/integTest/SearchMonitorsToolIT.java Outdated Show resolved Hide resolved

lezzago reviewed Jan 26, 2024

View reviewed changes

ohltyler added 2 commits January 26, 2024 21:32

Change to beforeEach

2aa7f31

Signed-off-by: Tyler Ohlsen <[email protected]>

Fix detector_type bug

fb5ea3a

Signed-off-by: Tyler Ohlsen <[email protected]>

lezzago approved these changes Jan 27, 2024

View reviewed changes

ohltyler merged commit 722bfd2 into opensearch-project:main Jan 27, 2024
10 of 13 checks passed

ohltyler deleted the search-monitor-fixes branch January 27, 2024 01:09

opensearch-trigger-bot bot mentioned this pull request Jan 27, 2024

[Backport 2.x] Fix SearchMonitorsTool bugs; add corresponding ITs #153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SearchMonitorsTool bugs; add corresponding ITs #151

Fix SearchMonitorsTool bugs; add corresponding ITs #151

ohltyler commented Jan 26, 2024 •

edited

Loading

ohltyler commented Jan 26, 2024

codecov bot commented Jan 26, 2024 •

edited

Loading

lezzago Jan 26, 2024

ohltyler Jan 26, 2024

ohltyler Jan 26, 2024

lezzago Jan 27, 2024

ohltyler commented Jan 27, 2024

Fix SearchMonitorsTool bugs; add corresponding ITs #151

Fix SearchMonitorsTool bugs; add corresponding ITs #151

Conversation

ohltyler commented Jan 26, 2024 • edited Loading

Description

Check List

ohltyler commented Jan 26, 2024

codecov bot commented Jan 26, 2024 • edited Loading

Codecov Report

lezzago Jan 26, 2024

Choose a reason for hiding this comment

ohltyler Jan 26, 2024

Choose a reason for hiding this comment

ohltyler Jan 26, 2024

Choose a reason for hiding this comment

lezzago Jan 27, 2024

Choose a reason for hiding this comment

ohltyler commented Jan 27, 2024

ohltyler commented Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading