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

[Index Management] Optimize ES response for indices list #123629

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented Jan 24, 2022

Fixes #106041

Summary

When loading all indices from ES for Index Management page, the request GET *?expand_wildcards=hidden,all includes all indices of the cluster with their settings and mappings. The response to this request can surpass the limit of 536MB and that would crash Kibana server.

This PR updates the request used in Index Management to GET *?expand_wildcards=hidden,all&filter_path=*.aliases,*.settings.index.hidden,*.data_stream that would only return specific index properties from the ES.

This change reduces the risk of Index Management requests to reach the limit of 536MB.

How to test

Check if Index Management is loading and there are no regressions in the indices table.

@yuliacech yuliacech added bug Fixes for quality problems that affect the customer experience Feature:Index Management Index and index templates UI v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.17.0 v8.0.1 labels Jan 24, 2022
@yuliacech yuliacech requested a review from a team as a code owner January 24, 2022 16:42
@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

@cjcenizal
Copy link
Contributor

@yuliacech Does this impact the behavior of opening the flyout for an index? I created #106041 to propose similar behavior to what you've implemented, but in that issue I note that we'd need to load the mappings and other information for an index opened in the flyout.

Copy link
Contributor

@alisonelizabeth alisonelizabeth 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 working on this @yuliacech! Changes LGTM. Verified table and flyout still render as expected.

I didn't do an in-depth look at the current test coverage - are there any additional tests we should add to ensure there are no regressions, or do we already have adequate coverage for the table/flyout?

@yuliacech Does this impact the behavior of opening the flyout for an index? I created #106041 to propose similar behavior to what you've implemented, but in that issue I note that we'd need to load the mappings and other information for an index opened in the flyout.

It looks like there are separate requests made when a user clicks on the following tabs in the flyout: "Settings", "Mappings", "Stats", and "Edit settings" so this change should not impact the flyout.

I did notice the "Docs Deleted" and "Primary Storage Size" details are missing on the "Summary" tab in the flyout. From what I can tell, this is a pre-existing bug. I don't see either property in the response. I tried deleting a doc from an index and it was still missing. I can open up a separate issue once you confirm this is not a regression. Thanks!

Index-Management-Elastic

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@yuliacech
Copy link
Contributor Author

Thank you for the review, @alisonelizabeth! And great catch with the flyout values, I opened this issue to track that. It seems like it's not a regression but an existing bug on main.
We don't have enough test coverage for that part of the code base and I added more tests in this PR that is waiting for an ES snapshot update to be merged.
@cjcenizal As Alison mentioned and I confirmed in the code, the flyout is loading the settings, mappings and stats separately. Also thanks for linking #106041, I guess it can be closed by this PR.

@yuliacech yuliacech added the auto-backport Deprecated - use backport:version if exact versions are needed label Jan 25, 2022
@yuliacech yuliacech merged commit f5e66d6 into elastic:main Jan 25, 2022
@kibanamachine
Copy link
Contributor

The following labels were identified as gaps in your version labels and will be added automatically:

  • v8.1.0

If any of these should not be on your pull request, please manually remove them.

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.0 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.0:
- [Index Management] Add data streams functionality to indices tab (#67940)
7.17 Backport failed because of merge conflicts

How to fix

Re-run the backport manually:

node scripts/backport --pr 123629

Questions ?

Please refer to the Backport tool documentation

yuliacech added a commit to yuliacech/kibana that referenced this pull request Jan 25, 2022
…rom ES (elastic#123629)

(cherry picked from commit f5e66d6)

# Conflicts:
#	x-pack/plugins/index_management/server/lib/fetch_indices.ts
yuliacech added a commit to yuliacech/kibana that referenced this pull request Jan 25, 2022
…rom ES (elastic#123629)

(cherry picked from commit f5e66d6)

# Conflicts:
#	x-pack/plugins/index_management/server/lib/fetch_indices.ts
@yuliacech yuliacech removed the v8.0.1 label Jan 25, 2022
yuliacech added a commit that referenced this pull request Jan 25, 2022
…rom ES (#123629) (#123716)

(cherry picked from commit f5e66d6)

# Conflicts:
#	x-pack/plugins/index_management/server/lib/fetch_indices.ts
yuliacech added a commit that referenced this pull request Jan 25, 2022
…rom ES (#123629) (#123718)

(cherry picked from commit f5e66d6)

# Conflicts:
#	x-pack/plugins/index_management/server/lib/fetch_indices.ts

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@@ -19,6 +19,8 @@ async function fetchIndicesCall(
const { body: indices } = await client.asCurrentUser.indices.get({
index: indexNamesString,
expand_wildcards: ['hidden', 'all'],
// only get specified properties in the response
Copy link
Contributor

@cjcenizal cjcenizal Jan 26, 2022

Choose a reason for hiding this comment

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

@yuliacech Could you add some more details to this comment to make a note that we want to avoid bloating this response, and that if we include mappings we risk exceeding the 500MB (or whatever it is) limit on Node payloads? The goal we want to achieve is to ensure someone making changes to this code doesn't accidentally reintroduce this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience Feature:Index Management Index and index templates UI release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.17.0 v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize Index Management's indices list network performance
6 participants