[Graph] Paginate through index patterns #120972

majagrubic · 2021-12-09T17:02:57Z

Summary

Addresses: #93770

While in the original issue refers to pagination on the server side, we were tagged in that issue, although Graph was making a large request for index patterns on the client side. Since saved object client already supports pagination, this PR breaks one big request into paginated smaller requests.

Checklist

Delete any items that are not applicable to this PR.

~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)~~
~~- [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)~~
~~- [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list~~
~~- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)~~
~~- [ ] This was checked for cross-browser compatibility~~

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

majagrubic · 2021-12-10T15:39:07Z

@elasticmachine merge upstream

elasticmachine · 2021-12-13T15:30:46Z

Pinging @elastic/kibana-graph (Feature:Graph)

elasticmachine · 2021-12-13T15:30:47Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

majagrubic · 2021-12-13T15:31:10Z

@elasticmachine merge upstream

dimaanj

LGTM, looks stylish.

x-pack/plugins/graph/public/helpers/use_workspace_loader.ts

kertal · 2021-12-14T09:24:07Z

x-pack/plugins/graph/public/helpers/use_workspace_loader.ts

+          .then((response) => {
+            perPage = response.perPage;
+            total = response.total;
+            page = response.page;


so this function is starting let with page = 1, which is requesting the first page, then you assign the value page = response.page; which is I think also the first page, since it was requested. It seems to me that in the case when there is a bigger total number than 1000, it would trigger makeRequest forever, since page is not incremented?

x-pack/plugins/graph/public/helpers/use_workspace_loader.ts

Co-authored-by: Matthias Wilhelm <ankertal@gmail.com>

kertal

Tested a-la-carte in Chrome, by setting perPage to 1, works as expected 👍

kertal · 2021-12-16T10:30:18Z

x-pack/plugins/graph/public/helpers/use_workspace_loader.ts

+            perPage = response.perPage;
+            total = response.total;


Nit: shouldn't be necessary to set perPage since it shouldn't change, which means it could be a const

kertal · 2021-12-16T10:32:58Z

@elasticmachine merge upstream

kibana-ci · 2021-12-16T11:50:54Z

💚 Build Succeeded

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
`graph`	444.5KB	444.7KB	+190.0B

History

💛 Build #13362 was flaky 520f24a
💛 Build #13151 was flaky 387e2c4
💚 Build #12889 succeeded 6e46ee6
💚 Build #12678 succeeded 54c9ce8
💔 Build #12657 failed c5ebda8
💔 Build #12581 failed 3556202

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

* [Graph] Paginate through index patterns * Extract logic into a generator function * Fix failing unit test * Fix page initialization * Update x-pack/plugins/graph/public/helpers/use_workspace_loader.ts Co-authored-by: Matthias Wilhelm <ankertal@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Matthias Wilhelm <ankertal@gmail.com>

Maja Grubic added 4 commits December 9, 2021 18:02

[Graph] Paginate through index patterns

9bc0044

Merge branch 'main' into graph-load-indexpatterns

12d0292

Extract logic into a generator function

3556202

Fix failing unit test

c5ebda8

Merge branch 'main' into graph-load-indexpatterns

54c9ce8

majagrubic added v8.1.0 release_note:fix Feature:Graph Graph application feature Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) labels Dec 13, 2021

majagrubic marked this pull request as ready for review December 13, 2021 15:30

majagrubic requested a review from a team as a code owner December 13, 2021 15:30

majagrubic requested a review from dimaanj December 13, 2021 15:30

Merge branch 'main' into graph-load-indexpatterns

6e46ee6

dimaanj approved these changes Dec 14, 2021

View reviewed changes

x-pack/plugins/graph/public/helpers/use_workspace_loader.ts Outdated Show resolved Hide resolved

kertal reviewed Dec 14, 2021

View reviewed changes

Fix page initialization

387e2c4

kertal reviewed Dec 14, 2021

View reviewed changes

x-pack/plugins/graph/public/helpers/use_workspace_loader.ts Outdated Show resolved Hide resolved

Update x-pack/plugins/graph/public/helpers/use_workspace_loader.ts

520f24a

Co-authored-by: Matthias Wilhelm <ankertal@gmail.com>

kertal approved these changes Dec 16, 2021

View reviewed changes

Merge branch 'main' into graph-load-indexpatterns

5acc9cc

majagrubic merged commit 8c9f847 into elastic:main Dec 16, 2021

kibanamachine added the backport:skip This commit does not require backporting label Dec 16, 2021

majagrubic deleted the graph-load-indexpatterns branch December 16, 2021 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Graph] Paginate through index patterns #120972

[Graph] Paginate through index patterns #120972

majagrubic commented Dec 9, 2021 •

edited

Loading

majagrubic commented Dec 10, 2021

elasticmachine commented Dec 13, 2021

elasticmachine commented Dec 13, 2021

majagrubic commented Dec 13, 2021

dimaanj left a comment

kertal Dec 14, 2021

kertal left a comment •

edited

Loading

kertal Dec 16, 2021

kertal commented Dec 16, 2021

kibana-ci commented Dec 16, 2021

[Graph] Paginate through index patterns #120972

[Graph] Paginate through index patterns #120972

Conversation

majagrubic commented Dec 9, 2021 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

majagrubic commented Dec 10, 2021

elasticmachine commented Dec 13, 2021

elasticmachine commented Dec 13, 2021

majagrubic commented Dec 13, 2021

dimaanj left a comment

Choose a reason for hiding this comment

kertal Dec 14, 2021

Choose a reason for hiding this comment

kertal left a comment • edited Loading

Choose a reason for hiding this comment

kertal Dec 16, 2021

Choose a reason for hiding this comment

kertal commented Dec 16, 2021

kibana-ci commented Dec 16, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

majagrubic commented Dec 9, 2021 •

edited

Loading

kertal left a comment •

edited

Loading