-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add a search AD results tool #52
Conversation
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
=============================================
+ Coverage 44.41% 81.57% +37.16%
- Complexity 30 78 +48
=============================================
Files 6 7 +1
Lines 358 456 +98
Branches 42 63 +21
=============================================
+ Hits 159 372 +213
+ Misses 178 45 -133
- Partials 21 39 +18 ☔ View full report in Codecov by Sentry. |
final String detectorId = parameters.getOrDefault("detectorId", null); | ||
final Boolean realTime = parameters.containsKey("realTime") ? Boolean.parseBoolean(parameters.get("realTime")) : null; | ||
final Double anomalyGradeThreshold = parameters.containsKey("anomalyGradeThreshold") | ||
? Double.parseDouble(parameters.get("anomalyGradeThreshold")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add sufficient test for this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we add more info in the test like anomalyGradeThreshold
, dataStartTime
, dataEndTime
etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added for other valid cases with the other params. But there won't be any other invalid cases since the other params are tested with isNumeric
, which if it fails, will default to null
values and not throw exceptions.
I don't use isNumeric
specifically because of #52 (comment)
final String detectorId = parameters.getOrDefault("detectorId", null); | ||
final Boolean realTime = parameters.containsKey("realTime") ? Boolean.parseBoolean(parameters.get("realTime")) : null; | ||
final Double anomalyGradeThreshold = parameters.containsKey("anomalyGradeThreshold") | ||
? Double.parseDouble(parameters.get("anomalyGradeThreshold")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check if the parameters.get("anomalyGradeThreshold") is a number before parsing to Double?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isNumeric
requires each character to be a digit, but users may want to pass -1
for example. Prefer to leave as-is, as -1
may be a common value (within LLM prompt potentially) to indicate they want to fetch all anomaly results, regardless if they are actual anomalies with grades > 0.
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
* Set up results tool boilerplate Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Finish AD result tool implementation Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Add AD result tool Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * remove TODO Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Add UT for invalid anomaly grade Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Add more UT for params Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Fix typo Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> --------- Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> (cherry picked from commit 18445e6) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 18445e6) Signed-off-by: Tyler Ohlsen <ohltyler@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>
…ect#67) (cherry picked from commit 18445e6) Signed-off-by: Tyler Ohlsen <ohltyler@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> Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Description
This PR adds a search AD results tool. It exposes several different parameters to search and sort the desired results. Note these are subject to change as more integration testing is done with agents configured with the tool.
Testing:
Issues Resolved
opensearch-project/ml-commons#1548
Check List
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.