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

Fix detector state params in SearchAnomalyDetectorsTool #235

Merged
merged 4 commits into from
Feb 26, 2024

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Feb 21, 2024

Description

This PR fixes two issues related to fetching and filtering detector state with the SearchAnomalyDetectorsTool.

NamedWriteableRegistry error

This fix coincides with opensearch-project/anomaly-detection#1164 - see there for more details on the original issue.

This change injects the NamedWriteableRegistry to the AD-related tools, in order to construct the AD node client with its updated constructor params that now requires a NamedWriteableRegistry. At a high level, this is used in the node client for being able to successfully serialize/deserialize the GetAnomalyDetectorResponse used when fetching detector profiles, when the SearchAnomalyDetectorsTool is invoked where any state-related param is passed down - e.g., running/failed.

Detectors filtering bug

There is an issue (uncovered after the above fix was made causing the tests to fail) of detectors with mismatching states not being filtered out. The root cause was that the profile detector transport action response does not include the detector ID by default in the anomalyDetector field, hence it was always null and nothing was removed from the hits map. This changes that by using the detector name as the key in the hits map, which is returned by default.

Miscellaneous

  • Removes disabled as a parameter. This caused LLM confusion, since a question like "how many stopped detectors do i have?" could lead to setting the parameters as "disabled": "true", or "running": "false". This simplifies that by only having one knob to turn instead of two. Additionally, this changes simplifies the filtering logic and the matrix combination of values shrink.
  • Other filtering logic cleanup
  • Adds integration tests to test out the detector state params and enforces filtering works as expected
  • Cleans up existing integration tests to handle individual-test-failures more cleanly and less error logs when running the tests
  • Cleans up existing unit tests
  • Minor cleanup to search AD results ITs to handle various flakiness

The overall impact of these changes is that detector-state-related questions passed to an agent provisioned with the SearchAnomalyDetectorsTool now works as expected.

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.

@ohltyler
Copy link
Member Author

Leaving as draft until the related AD PR is merged so the CI can pick up the updated sonatype dependency.

@ohltyler ohltyler changed the title Inject NamedWriteableRegistry into AD tools Fix detector state params in SearchAnomalyDetectorsTool Feb 22, 2024
Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 81.57%. Comparing base (bd510a5) to head (bd4b3ae).

Files Patch % Lines
...search/agent/tools/SearchAnomalyDetectorsTool.java 82.60% 0 Missing and 4 partials ⚠️
src/main/java/org/opensearch/agent/ToolPlugin.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #235      +/-   ##
============================================
- Coverage     81.69%   81.57%   -0.13%     
- Complexity      206      211       +5     
============================================
  Files            13       13              
  Lines          1060     1069       +9     
  Branches        141      146       +5     
============================================
+ Hits            866      872       +6     
  Misses          140      140              
- Partials         54       57       +3     

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

@ohltyler ohltyler marked this pull request as ready for review February 23, 2024 01:24
@ohltyler ohltyler merged commit 44dc232 into opensearch-project:main Feb 26, 2024
17 of 18 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/skills/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/skills/backport-2.x
# Create a new branch
git switch --create backport/backport-235-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 44dc2326e0da81923e8129453304a2308a361065
# Push it to GitHub
git push --set-upstream origin backport/backport-235-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/skills/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-235-to-2.x.

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.

3 participants