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

Add suggest anomaly detector action to discover page #849

Merged
merged 11 commits into from
Sep 9, 2024

Conversation

gaobinlong
Copy link
Contributor

@gaobinlong gaobinlong commented Aug 23, 2024

Description

This PR adds a suggest anomaly detector button to the discover page, when users click the button, a flyout pops up, and the index pattern extracted from the discover page is sent to a backend API which will return some suggested parameters from LLM, the suggested parameters contain features list and a optional category field used for creating anomaly detector, users can click the create detector button to create a new detector after they confirm the suggested parameters or update that if need.

The code change include:

  • Register a new DiscoverAction into Discover, the register function is provided by OSD core, in this PR:Add registrable DiscoverActions to discover page OpenSearch-Dashboards#7793
  • Implement a new flyout for generating anomaly detector, we're not able to use the existing feature anywhere flyout because there're are many difference between the UI and existing feature anywhere flyout is associated with a visualization which is different from our case.
  • Add a new node API _generate_parameters which calls some backend APIs in the ml-commons plugin.
  • Some common API calls in the frontend use relative path, for example, ..${BASE_NODE_API_PATH}/_mappings, which is not available for this feature, because the URI of the discover page is app/data-explorer/discover#, different from the URI in anomaly detection plugin: app/anomaly-detection-dashboards#, so I changed it to ${BASE_NODE_API_PATH}/_mappings to avoid 404 error.

Some screenshots:

  1. Entry point
image
  1. Flyout
image

A demo video:

generate_anomaly_detector_480.mov

Issues Resolved

#816

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
  • 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.

@gaobinlong
Copy link
Contributor Author

gaobinlong commented Aug 23, 2024

This PR depends on the PR in core which is still in reviewing, I'll add more tests for this PR during the reviewing process, @ohltyler @kaituo @jackiehanyang @owaiskazi19 @joshpalis @SuZhou-Joe @ruanyl could you help to review this PR? Thank you!

Copy link
Collaborator

@jackiehanyang jackiehanyang left a comment

Choose a reason for hiding this comment

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

Thanks for the change! Here's my first pass of the review, I raised some minor issues and will review it again once there are more tests.

My main concern here is that the flyout code is exactly the same as the one used in the feature anywhere detector creation flyout. Can we reuse the feature anywhere detector creation flyout here instead of copying the code into a different folder? This would help avoid adding duplicate code and the need to maintain two sets of code

public/redux/reducers/ad.ts Show resolved Hide resolved
opensearch_dashboards.json Show resolved Hide resolved
opensearch_dashboards.json Outdated Show resolved Hide resolved
Signed-off-by: gaobinlong <gbinlong@amazon.com>
@gaobinlong
Copy link
Contributor Author

My main concern here is that the flyout code is exactly the same as the one used in the feature anywhere detector creation flyout. Can we reuse the feature anywhere detector creation flyout here instead of copying the code into a different folder? This would help avoid adding duplicate code and the need to maintain two sets of code

@jackiehanyang Thanks for your review, I meant to leverage the existing feature anywhere flyout, but failed to do that because there're many difference between this case and the feature anywhere:

  1. Feature anywhere flyout is associated with a visualization which is used to extract the key information for creating anomaly detector, but for the case in this PR, information is extracted from the Discover page, no visualization exists, so it's hard to use the AddAnomalyDetector component directly in this case.
  2. The UI is also different from feature anywhere, Associate existing detector and show visualization is not needed in this case, and a different Categorical field and a new Timestamp field are added to Advanced options:
image vs image Pasted Graphic vs image

@xluo-aws
Copy link
Member

Thanks, Jackie. Can we get one more maintainer help review this PR? We plan to release it in 2.17.

Signed-off-by: gaobinlong <gbinlong@amazon.com>
@gaobinlong gaobinlong changed the title Add generate anomaly detector action to discover page Add suggest anomaly detector action to discover page Aug 28, 2024
Signed-off-by: gaobinlong <gbinlong@amazon.com>
Signed-off-by: gaobinlong <gbinlong@amazon.com>
Signed-off-by: gaobinlong <gbinlong@amazon.com>
@jackiehanyang
Copy link
Collaborator

The code change looks good to me, just I see failed CI. Could you please fix that?

@gaobinlong
Copy link
Contributor Author

The code change looks good to me, just I see failed CI. Could you please fix that?

Thank you, this PR is waiting a PR in OSD core repo to be merged, I'll get all checks passed after that PR is merged.

@gaobinlong gaobinlong force-pushed the discover_action branch 2 times, most recently from b35019c to f9bdcfb Compare September 3, 2024 10:14
Signed-off-by: gaobinlong <gbinlong@amazon.com>
@jackiehanyang
Copy link
Collaborator

@gaobinlong is this pr still tracking 2.17 release?

@gaobinlong
Copy link
Contributor Author

@gaobinlong is this pr still tracking 2.17 release?

Actually yes, I know today is the code freeze day, but the two PRs in other repos that this PR depends on have not been merged yet(still have some UI change), so please be lenient with this PR, thank you!

@jackiehanyang
Copy link
Collaborator

@gaobinlong is this pr still tracking 2.17 release?

Actually yes, I know today is the code freeze day, but the two PRs in other repos that this PR depends on have not been merged yet(still have some UI change), so please be lenient with this PR, thank you!

Could you link the dependency PRs here. Also please address the conflicts on this pr so that we could quickly merge in this one after the dependency PRs are merged and CI all passed. The first RC build is on 11:00am PST, Sept 5th

@gaobinlong
Copy link
Contributor Author

@gaobinlong is this pr still tracking 2.17 release?

Actually yes, I know today is the code freeze day, but the two PRs in other repos that this PR depends on have not been merged yet(still have some UI change), so please be lenient with this PR, thank you!

Could you link the dependency PRs here. Also please address the conflicts on this pr so that we could quickly merge in this one after the dependency PRs are merged and CI all passed. The first RC build is on 11:00am PST, Sept 5th

This PR depends on opensearch-project/OpenSearch-Dashboards#7936 and opensearch-project/dashboards-assistant#265.

Refactor unit test code

Signed-off-by: gaobinlong <gbinlong@amazon.com>
Signed-off-by: gaobinlong <gbinlong@amazon.com>
Signed-off-by: gaobinlong <gbinlong@amazon.com>
Signed-off-by: gaobinlong <gbinlong@amazon.com>
@gaobinlong
Copy link
Contributor Author

@jackiehanyang thanks, I've resolved the conflicts and linked the dependency PRs yet, and also made some change for this PR, that includes:

  1. Move the entry point to the query editor in Discover 2.0 page.
  2. Remove the newly introduced node API _generate_parameters, call the node API in dashboard-assistant plugin instead.

, now the implementation is simplified, please help to take a look, thank you!

Signed-off-by: gaobinlong <gbinlong@amazon.com>
@gaobinlong
Copy link
Contributor Author

@jackiehanyang the direct dependency PR has been merged, so now all checks have passed yet in this PR, please help to approve and merge if no issue exists, thank you!

@jackiehanyang jackiehanyang merged commit ec02b63 into opensearch-project:main Sep 9, 2024
9 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 9, 2024
* Add generate anomaly detector action to discover page

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Add more test code and rename the file

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Modify flyout header

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Make the detectorName follow the convention

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Truncate the index pattern name if it's too long

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Move entry point to query editor

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Call the node API in dashboard-assistant plugin to generate parameters

Refactor unit test code

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Fix test failure

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Revert the code format

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Remove some empty lines

Signed-off-by: gaobinlong <gbinlong@amazon.com>

---------

Signed-off-by: gaobinlong <gbinlong@amazon.com>
(cherry picked from commit ec02b63)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 9, 2024
* Add generate anomaly detector action to discover page

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Add more test code and rename the file

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Modify flyout header

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Make the detectorName follow the convention

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Truncate the index pattern name if it's too long

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Move entry point to query editor

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Call the node API in dashboard-assistant plugin to generate parameters

Refactor unit test code

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Fix test failure

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Revert the code format

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Remove some empty lines

Signed-off-by: gaobinlong <gbinlong@amazon.com>

---------

Signed-off-by: gaobinlong <gbinlong@amazon.com>
(cherry picked from commit ec02b63)
jackiehanyang pushed a commit that referenced this pull request Sep 9, 2024
* Add generate anomaly detector action to discover page

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Add more test code and rename the file

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Modify flyout header

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Make the detectorName follow the convention

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Truncate the index pattern name if it's too long

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Move entry point to query editor

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Call the node API in dashboard-assistant plugin to generate parameters

Refactor unit test code

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Fix test failure

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Revert the code format

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Remove some empty lines

Signed-off-by: gaobinlong <gbinlong@amazon.com>

---------

Signed-off-by: gaobinlong <gbinlong@amazon.com>
(cherry picked from commit ec02b63)

Co-authored-by: gaobinlong <gbinlong@amazon.com>
jackiehanyang pushed a commit that referenced this pull request Sep 9, 2024
* Add generate anomaly detector action to discover page

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Add more test code and rename the file

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Modify flyout header

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Make the detectorName follow the convention

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Truncate the index pattern name if it's too long

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Move entry point to query editor

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Call the node API in dashboard-assistant plugin to generate parameters

Refactor unit test code

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Fix test failure

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Revert the code format

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Remove some empty lines

Signed-off-by: gaobinlong <gbinlong@amazon.com>

---------

Signed-off-by: gaobinlong <gbinlong@amazon.com>
(cherry picked from commit ec02b63)

Co-authored-by: gaobinlong <gbinlong@amazon.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.

3 participants