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.

Sign up for GitHub

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

Jump to bottom

support storing detector result to customr result index #110

Merged
merged 7 commits into from
Nov 1, 2021
Merged

support storing detector result to customr result index #110

merged 7 commits into from
Nov 1, 2021

Conversation

ylwu-amzn
Copy link
Contributor

@ylwu-amzn ylwu-amzn commented Oct 28, 2021 •
edited
Loading

Signed-off-by: Yaliang Wu ylwu@amazon.com

Description

Now we store anomaly result in system index. User can user search anomaly result API to query data in result system index, but user can't search that index to build their own dashboard. This PR add option to let user specify their own custom result index and AD plugin will store detector's result to that index. So user can get detector result by searching that index directly.

Main changes:

  1. Add result index option on detector creation page
  2. Show result index on detector configuration review page
  3. Read anomaly result from custom result index on detector realtime/historical result page.
  4. Read anomaly result from custom result indices on AD dashboard and detector list page.

TODO: fix some test ; add more test for new code

Screen Shot 2021-10-29 at 11 42 55 AM

Screen Shot 2021-10-28 at 4 17 11 PM

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.

@ylwu-amzn
support storing detector result to customr result index
6a97796 
Signed-off-by: Yaliang Wu <ylwu@amazon.com>
@ylwu-amzn
Copy link
Contributor Author

This PR formats some code, suggest open "Hide whitespace" when review code.

@ylwu-amzn
reformat with prettier
27adb2f 
Signed-off-by: Yaliang Wu <ylwu@amazon.com>
@ylwu-amzn
make prettier version consistent with dashboard; fix AD dashboard liv…
9858cb7 
…e chart not showing results from custom result index
@ohltyler
Copy link
Member

ohltyler commented Oct 29, 2021 •
edited
Loading

Couple high-level things:

  1. How does a user default to not using a custom result index, should there be a checkbox or something to select/de-select?
  2. What shows on the detector settings page if no custom index was selected?
  3. Can the user update this later on? If so, how do we handle showing results from multiple possible source indices? EDIT: I see this isn't possible
  4. Is preview / sample results supported? EDIT: I see it is
  5. If the user is creating a new index via specifying custom result index, how is error handling shown? Suppose that index already exists with other data, or user doesn't have permissions, etc. Should there be a toast when trying to create, or will the creation still show as successful but fail silently? Or should there be an additional toast saying the success/failure of index creation? EDIT: I see any error is propagated fine to the frontend via toast, & creation fails.

@ylwu-amzn
tune result index filed description based on tech writer's comments
6ef26f7 
Signed-off-by: Yaliang Wu <ylwu@amazon.com>
@ylwu-amzn ylwu-amzn force-pushed the custom_result_index_pr branch from 5a97048 to 6ef26f7 Compare October 29, 2021 18:42
ohltyler
ohltyler reviewed Oct 29, 2021
View reviewed changes
public/pages/DetectorResults/containers/AnomalyHistory.tsx
@@ -347,7 +351,7 @@ export const AnomalyHistory = (props: AnomalyHistoryProps) => {
props.isHistorical,
taskId.current
);
return dispatch(searchResults(entityResultQuery));
return dispatch(searchResults(entityResultQuery, resultIndex));
Copy link
Member

Choose a reason for hiding this comment

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

most of the queries / results parsing has been changed as part of multi-category filtering PR. Will need to keep an eye on that when merging.

ohltyler
ohltyler reviewed Oct 29, 2021
View reviewed changes
@ohltyler ohltyler added feature A new feature for the plugin v1.2.0 Version 1.2.0 labels Oct 29, 2021
@ylwu-amzn
Copy link
Contributor Author

New custom result index UI
Screen Shot 2021-10-30 at 9 17 05 PM

@ohltyler
Copy link
Member

Another minor question, maybe it's a detail in the backend PR, need to look more. But what would happen if a user starts a fresh cluster and only creates custom result indices detectors first? Does the default result system index still get inited? And if not, does either frontend or backend handle any errors properly if that index is missing?

@ohltyler
Copy link
Member

ohltyler commented Nov 1, 2021

May need to fix some broken tests and update snapshots with yarn test:jest <files-to-test> -u. I'm ok with adding tests as a separate PR - if so, can you create an issue to track?

@ylwu-amzn
Copy link
Contributor Author

Another minor question, maybe it's a detail in the backend PR, need to look more. But what would happen if a user starts a fresh cluster and only creates custom result indices detectors first? Does the default result system index still get inited? And if not, does either frontend or backend handle any errors properly if that index is missing?

Default result index will not be inited if just create detector with custom result index on a fresh cluster.Tested locally, no issue found.

amitgalitz
amitgalitz reviewed Nov 1, 2021
View reviewed changes
public/utils/utils.tsx
@@ -63,6 +69,19 @@ export const validateName = (
}
};

export const validateCustomResultIndex = (name: string): string | undefined => {
Copy link
Member

Choose a reason for hiding this comment

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

In future this method can also be used when user tries inputing custom data source index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we allow user to choose concrete index or input index pattern like "abc*". So can't reuse this part directly. We can revisit/refactor this part in future if needed.

ohltyler
ohltyler approved these changes Nov 1, 2021
View reviewed changes
Copy link
Member

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

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

LGTM!

amitgalitz
amitgalitz approved these changes Nov 1, 2021
View reviewed changes
Copy link
Member

@amitgalitz amitgalitz left a comment

Choose a reason for hiding this comment

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

LGTM, make sure you sign off all commits in the future. Will merge DCO check after this PR is merged so there will be no issue for this PR at the moment.

@ylwu-amzn ylwu-amzn merged commit 67cb4eb into opensearch-project:main Nov 1, 2021
@ohltyler
Copy link
Member

ohltyler commented Nov 1, 2021

Shouldn't the backend have been merged before this one? I guess all will be merged soon enough, so no big issue

ohltyler pushed a commit to ohltyler/anomaly-detection-dashboards-plugin-1 that referenced this pull request Nov 8, 2021
@ylwu-amzn @ohltyler
support storing detector result to customr result index (opensearch-p…
8030628 
…roject#110)

* support storing detector result to customr result index

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* reformat with prettier

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* make prettier version consistent with dashboard; fix AD dashboard live chart not showing results from custom result index

* tune result index filed description based on tech writer's comments

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* tune custom result index ux; add more comments

* show custom result index when update detector; fix failed test

* fix typo
@ohltyler ohltyler mentioned this pull request Nov 8, 2021
Merged
1 task
ohltyler pushed a commit that referenced this pull request Nov 9, 2021
@ylwu-amzn @ohltyler
support storing detector result to customr result index (#110)
76f649f 
* support storing detector result to customr result index

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* reformat with prettier

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* make prettier version consistent with dashboard; fix AD dashboard live chart not showing results from custom result index

* tune result index filed description based on tech writer's comments

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* tune custom result index ux; add more comments

* show custom result index when update detector; fix failed test

* fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ohltyler ohltyler ohltyler approved these changes

@amitgalitz amitgalitz amitgalitz approved these changes

Assignees
No one assigned
Labels
feature A new feature for the plugin v1.2.0 Version 1.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ylwu-amzn @ohltyler @amitgalitz