-
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 visualization tool #41
Conversation
Signed-off-by: Hailong Cui <[email protected]>
please help to add backport 2.x label also |
Signed-off-by: Hailong Cui <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
============================================
+ Coverage 44.41% 50.35% +5.94%
- Complexity 30 38 +8
============================================
Files 6 7 +1
Lines 358 423 +65
Branches 42 49 +7
============================================
+ Hits 159 213 +54
- Misses 178 187 +9
- Partials 21 23 +2 ☔ View full report in Codecov by Sentry. |
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.
LGTM with some nits.
src/main/java/org/opensearch/agent/tools/VisualizationsTool.java
Outdated
Show resolved
Hide resolved
src/test/resources/org/opensearch/agent/tools/visualization.json
Outdated
Show resolved
Hide resolved
src/test/resources/org/opensearch/agent/tools/visualization_not_found.json
Outdated
Show resolved
Hide resolved
Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
src/main/java/org/opensearch/agent/tools/VisualizationsTool.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
Need to resolve the conflict |
|
||
public static final String SAVED_OBJECT_TYPE = "visualization"; | ||
private static final String DEFAULT_DESCRIPTION = | ||
"Use this tool to find user created visualizations. This tool takes the visualization name as input and returns the first 3 matching visualizations"; |
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.
Does 3
needs to be static? Can't we do like 3 by default but the input is dynamic?
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.
good suggestion, update the number to dynamic setting.
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.
May be update the description as well?
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.
the specific number in description has been removed. now it's "Use this tool to find user created visualizations. This tool takes the visualization name as input and returns the matching visualizations"
src/main/java/org/opensearch/agent/tools/SearchAnomalyDetectorsTool.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
resolved |
* Visualization Tool Signed-off-by: Hailong Cui <[email protected]> * fix build failure due to forbiddenApis Signed-off-by: Hailong Cui <[email protected]> * Address review comments Signed-off-by: Hailong Cui <[email protected]> * spotlessApply Signed-off-by: Hailong Cui <[email protected]> * update default tool name Signed-off-by: Hailong Cui <[email protected]> * update number of visualization be dynamic Signed-off-by: Hailong Cui <[email protected]> --------- Signed-off-by: Hailong Cui <[email protected]> (cherry picked from commit 3774eb9) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Visualization Tool * fix build failure due to forbiddenApis * Address review comments * spotlessApply * update default tool name * update number of visualization be dynamic --------- (cherry picked from commit 3774eb9) Signed-off-by: Hailong Cui <[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>
* Visualization Tool * fix build failure due to forbiddenApis * Address review comments * spotlessApply * update default tool name * update number of visualization be dynamic --------- (cherry picked from commit 3774eb9) Signed-off-by: Hailong Cui <[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]>
Description
Visualization Tool, coming from opensearch-project/ml-commons#1649
Issues Resolved
[List any issues this PR will resolve]
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.