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

[ML] Data view loading refactor #116455

Merged

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Oct 27, 2021

Removes the cache of data views (indexPatternCache) which was populated on first page load of a number of ML pages. Loaded via the client side router.
Our recent change to the data view service's find function using a wildcard to load every data view was causing excessive calls to the _fields_for_wildcard, one for each requested data view.
Previously we had been using the saved object service's find function which did not also load all of the fields for each requested data view.

This PR removes this call to find as almost all places where the cache was being used, the full data view saved objects with fields were not needed, rather a list of data view names or a match of name to id.
These have been now all been changed to async calls to the real data view data via the data view service.

The rest of the changes in this PR are variable and function identifier changes, index pattern -> data view.
There are still a lot remaining "index patterns" in our code base that I have left be, so not to double the size of this PR and lose focus of the _fields_for_wildcard problem it is solving.

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic jgowdyelastic self-assigned this Nov 1, 2021
@jgowdyelastic jgowdyelastic added :ml review auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience release_note:fix v8.0.0 v8.1.0 labels Nov 1, 2021
@jgowdyelastic jgowdyelastic marked this pull request as ready for review November 1, 2021 15:15
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner November 1, 2021 15:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and overall looks good. Just left a few minor comments.

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM

@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #11 / detection engine api security and spaces enabled create_rules_with_exceptions creating rules with exceptions tests with auditbeat data rules with value list exceptions generates no signals when a value list exception is added for a query rule

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.5MB 3.5MB +538.0B
Unknown metric groups

References to deprecated APIs

id before after diff
ml 80 77 -3

History

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

cc @jgowdyelastic

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡

@jgowdyelastic jgowdyelastic merged commit ee63830 into elastic:main Nov 3, 2021
@jgowdyelastic jgowdyelastic deleted the data-view-loading-refactor branch November 3, 2021 09:03
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 3, 2021
* [ML] Data view loading refactor

* more renaming

* more renaming

* fixing tests

* fixing jest test

* small changes based on review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 5, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine added a commit that referenced this pull request Nov 9, 2021
* [ML] Data view loading refactor

* more renaming

* more renaming

* fixing tests

* fixing jest test

* small changes based on review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: James Gowdy <jgowdy@elastic.co>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 9, 2021
@peteharverson peteharverson added release_note:enhancement and removed release_note:fix bug Fixes for quality problems that affect the customer experience labels Jan 11, 2022
jgowdyelastic added a commit that referenced this pull request Jan 17, 2023
…147916)

Fixes #147886 by manually
checking whether the index behind a data view actually exists before
fetching it. This fixes the issue with the data view service which
displays a toast for every data view which it cannot load the fields
for.

This fix has the potential to increase the amount of requests to es by
quite a large amount.

For example, if a user has 100 data views, 80 of which have existing
indices.
1 request is made to load all data view ids and titles
100 requests are made to fetch the fields for each index. 80 succeed.
80 requests are then made to fetch each data view.

In 8.0 this behaviour has been
[changed](#116455). We no longer
request all of the data views up front on page load.
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 :ml release_note:enhancement review v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants