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

fix bugs on configuration pages and refactor code #49

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

ansjcy
Copy link
Member

@ansjcy ansjcy commented Jan 18, 2025

Description

Currently when we fetch the settings from backend, there are 2 defects:

  • We are not considering any transient settings
  • When obtaining window size / top n sizes from settings, if a metric is enabled but no default settings are set, we are setting "undefined" value to a state, which makes the setState function fail - In this case, if there are values in the settings for other metrics, the old values will be used.

This PR resolved both of the issues by assigning default values when reading settings data from backend. Also make sure we pick up the transient settings as well

Issues Resolved

#31
#29

Results

  • Run a clean OpenSearch Cluster
  • Start OSD with query insights plugin
  • insert configurations
curl -X PUT 'localhost:9200/_cluster/settings' -H 'Content-Type: application/json' -d'
{
    "persistent" : {
         "search.insights.top_queries.latency.enabled" : true,
        "search.insights.top_queries.cpu.enabled" : true,
        "search.insights.top_queries.memory.enabled" : true,
        "search.insights.top_queries.group_by" : "none",
        "search.insights.top_queries.latency.window_size" : "1m",
        "search.insights.top_queries.latency.top_n_size" : 5
    }
}'
  • check configuration page, the latency setting is correctly picked up
image
  • Check CPU setting, the default values matches what's on the backend
image - Check Memory setting, the default values matches what's on the backend image
  • Override with transient settings to disable top queries by latency
curl -X PUT 'localhost:9200/_cluster/settings' -H 'Content-Type: application/json' -d'
{
    "transient" : {
        "search.insights.top_queries.latency.enabled" : "false"
    }
}'
  • Refresh the page and the transient settings are picked up
image
  • Override with transient settings to enable grouping
curl -X PUT 'localhost:9200/_cluster/settings' -H 'Content-Type: application/json' -d'
{
    "transient" : {
        "search.insights.top_queries.group_by" : "similarity"
    }
}'
  • Refresh the page and the transient settings are picked up for group by
image

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.

Signed-off-by: Chenyang Ji <cyji@amazon.com>
Copy link
Collaborator

@deshsidd deshsidd left a comment

Choose a reason for hiding this comment

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

Looks good overall and definitely a more elegant way to solve the 2 bugs. We should add some tests to make sure we don't regress in the future but can do it as a follow-up due to the tight timelines?

}

export function getTimeAndUnitFromString(time: string | undefined | null): string[] {
const defaultWindowSize = [DEFAULT_WINDOW_SIZE, TIME_UNIT_ABBREVIATION.MINUTES];
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can name this to make the code more readable.

@deshsidd deshsidd merged commit 116c751 into opensearch-project:main Jan 21, 2025
9 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 21, 2025
Signed-off-by: Chenyang Ji <cyji@amazon.com>
(cherry picked from commit 116c751)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
deshsidd pushed a commit that referenced this pull request Jan 21, 2025
(cherry picked from commit 116c751)

Signed-off-by: Chenyang Ji <cyji@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>
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.

2 participants