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

Update Frontend for Custom Result Index Query and Fix Issues #772

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

kaituo
Copy link
Collaborator

@kaituo kaituo commented Jun 7, 2024

Description

This PR finalizes the frontend changes related to PR #1225. The custom result index query now uses an index pattern instead of a single index.

Additionally, this PR addresses an issue where missing custom result indices would appear because the original code checked for the existence of an index name, but now we use it as a prefix. We have updated the logic to perform a prefix search instead of checking for index name equality.

This PR also resolves issue #765 by downgrading the version of jest-canvas-mock.

Testing Done:

  • Added unit tests.
  • Verified that the custom result index missing callout is not shown.
  • Confirmed that the frontend can still display old and new results after a rollover.

Issues Resolved

#765

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.

amitgalitz
amitgalitz previously approved these changes Jun 7, 2024
This PR finalizes the frontend changes related to PR #1225. The custom result index query now uses an index pattern instead of a single index.

Additionally, this PR addresses an issue where missing custom result indices would appear because the original code checked for the existence of an index name, but now we use it as a prefix. We have updated the logic to perform a prefix search instead of checking for index name equality.

This PR also resolves issue opensearch-project#765 by downgrading the version of jest-canvas-mock.

Testing Done:
* Added unit tests.
* Verified that the custom result index missing callout is not shown.
* Confirmed that the frontend can still display old and new results after a rollover.

Signed-off-by: Kaituo Li <kaituo@amazon.com>
Signed-off-by: Kaituo Li <kaituo@amazon.com>
@kaituo
Copy link
Collaborator Author

kaituo commented Jun 10, 2024

In the latest commit, I have made the following changes:

  • Rebased Code: Rebased my code from the main branch.
  • Alias Check: Updated the alias check to use equality instead of a prefix check. For more details, refer to this discussion.
  • Removed ISM Plugin in Custom Index callout: Removed the use of the ISM plugin in the custom index part of the create detector workflow. The min_age and similar parameters are required and cannot be omitted. Even if these parameters are made optional, they default to empty strings, which the backend AnomalyDetector' parse method expects to be integers. So cx cannot to choose ISM to manage their plugin.
    • Before
    Screenshot 2024-06-10 at 7 56 45 AM - After Screenshot 2024-06-08 at 11 36 09 AM
  • Configuration Review Page: Modified the configuration review page to exclude ISM parameter units when the custom result index is empty for a cleaner appearance.
    • Before:
      Screenshot 2024-06-08 at 11 18 17 AM
    • After:
      Screenshot 2024-06-08 at 11 29 45 AM

amitgalitz
amitgalitz previously approved these changes Jun 10, 2024
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, thanks for fixing the alias part

Signed-off-by: Kaituo Li <kaituo@amazon.com>
@jackiehanyang
Copy link
Collaborator

Thanks for the change!
There's another place that's also calling search result index - https://github.com/opensearch-project/anomaly-detection-dashboards-plugin/blob/main/public/expressions/helpers.ts#L32 that needs to be updated. I'm working on a MDS related fix on this part, I can update it

@kaituo kaituo merged commit 6513e1b into opensearch-project:main Jun 10, 2024
9 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 10, 2024
* Update Frontend for Custom Result Index Query and Fix Issues

This PR finalizes the frontend changes related to PR #1225. The custom result index query now uses an index pattern instead of a single index.

Additionally, this PR addresses an issue where missing custom result indices would appear because the original code checked for the existence of an index name, but now we use it as a prefix. We have updated the logic to perform a prefix search instead of checking for index name equality.

This PR also resolves issue #765 by downgrading the version of jest-canvas-mock.

Testing Done:
* Added unit tests.
* Verified that the custom result index missing callout is not shown.
* Confirmed that the frontend can still display old and new results after a rollover.

Signed-off-by: Kaituo Li <kaituo@amazon.com>

* change to check alias

Signed-off-by: Kaituo Li <kaituo@amazon.com>

* fix warning msg

Signed-off-by: Kaituo Li <kaituo@amazon.com>

---------

Signed-off-by: Kaituo Li <kaituo@amazon.com>
(cherry picked from commit 6513e1b)
jackiehanyang pushed a commit that referenced this pull request Jun 10, 2024
…774)

* Update Frontend for Custom Result Index Query and Fix Issues

This PR finalizes the frontend changes related to PR #1225. The custom result index query now uses an index pattern instead of a single index.

Additionally, this PR addresses an issue where missing custom result indices would appear because the original code checked for the existence of an index name, but now we use it as a prefix. We have updated the logic to perform a prefix search instead of checking for index name equality.

This PR also resolves issue #765 by downgrading the version of jest-canvas-mock.

Testing Done:
* Added unit tests.
* Verified that the custom result index missing callout is not shown.
* Confirmed that the frontend can still display old and new results after a rollover.

Signed-off-by: Kaituo Li <kaituo@amazon.com>

* change to check alias

Signed-off-by: Kaituo Li <kaituo@amazon.com>

* fix warning msg

Signed-off-by: Kaituo Li <kaituo@amazon.com>

---------

Signed-off-by: Kaituo Li <kaituo@amazon.com>
(cherry picked from commit 6513e1b)

Co-authored-by: Kaituo Li <kaituo@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