[Search] Search batching using bfetch #83418

lizozom · 2020-11-16T12:24:15Z

Summary

This PR was reverted and replaced with #84043

This PR introduces search request batching using bfetch.
It is especially beneficial to applications that send multiple requests in a session, like dashboard.
It should address customer complaints on loading taking long and using a lot of network due to making multiple connections.

Without bfetch

With bfetch, the dashboard is re-loaded using a single request.

Edge cases and things to pay attention to

Requests can be sent in more than one batch. Batches are limited by size and by time. In some cases, multiple batches might be sent

Query cancellation

Query cancellation still works!

In OSS we rely on a hangup like before. Since bfetch was adjusted not to wait for aborted requests, it will close the connection as all other requests in the batch are completed.

In xpack, the same logic applies, but we also send the cancellation request

Errors

Regular errors are returned as part of the response stream and are passed back to the issuing request.
If a bfetch stream is hung up for some reason, all requests are rejected properly.

Release Notes

This PR introduces search request batching using bfetch.
It should address performance issues due to opening multiple connections for search, especially in applications like dashboard.

Users who wish to opt-out and fallback to legacy behavior for any reason, can toggle the Use legacy search option in Advanced Settings.

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)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

…fetch-in-search

elasticmachine · 2020-11-17T17:16:27Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

src/plugins/bfetch/public/batching/batch_utils.ts

lukasolson

I would much prefer that we stick to the AbortSignal usage rather than introduce this getBatchDone$ which creates an observable that serves the same purpose.

…fetch-in-search

lizozom · 2020-11-19T13:49:07Z

@lukasolson in your patch, the batch signal fires if all requests that had an abort signal were aborted.
However, if there were any requests that were not aborted or didn't have a signal - you would kill the batch and prevent them from completing.

Also, using reference counting to track success, is not a good idea, as the same request might get resolved, then get aborted. (See the comment I gave to @streamich )

I really thing we need a single mechanism to track all requests, and I think mine is simple enough and having utility functions contributes to the testability of this code.

…fetch-in-search

streamich

bfetch changes LGTM. Added few suggestions below.

src/plugins/bfetch/public/batching/create_streaming_batched_function.ts

streamich · 2020-11-20T12:58:17Z

src/plugins/bfetch/public/batching/create_streaming_batched_function.ts

+        // abort when all items were either resolved, rejected or aborted
+        Promise.all(promises).then(() => abortController.abort());


There is a small problem, which practically might not be an issue. The abortController.abort() is called whenever all items in the batch have somehow completed—either resolved or rejected, or aborted; in any combination.

But my understanding is that we don't need to call abortController.abort() when, for example, all items successfully resolved. We need to call abortController.abort() only when all items have completed and at least one of them completed due to an abort signal.

Do you think we should be that specific? Or aborting the request every time, even when it already finished successfully is a noop?

src/plugins/bfetch/public/batching/create_streaming_batched_function.test.ts

src/plugins/bfetch/public/streaming/from_streaming_xhr.ts

lizozom · 2020-11-21T19:58:15Z

@elasticmachine merge upstream

…ana into enhance/use-bfetch-in-search

* Use bfetch for search (no abort behavior) * fix merge * Handle request abortion + unit tests * fix jest * shim totals in oss * proper formatting for errors * jest, types and docs * Fix doc * Remove old search code and rename UI Setting * jest mocks * jest * Solve unhanled error * Use AbortSignal * ts * code review - use abort controller instead of observable * Revert "Remove old search code and rename UI Setting" This reverts commit 17de9fa. * Remove old search code and rename UI Setting * revert search route * fix event unsubscribe * code review 2 * revert filter * simplify batch done logic * code review * filter items in the beginning * jest Co-authored-by: Kibana Machine <[email protected]>

* Use bfetch for search (no abort behavior) * fix merge * Handle request abortion + unit tests * fix jest * shim totals in oss * proper formatting for errors * jest, types and docs * Fix doc * Remove old search code and rename UI Setting * jest mocks * jest * Solve unhanled error * Use AbortSignal * ts * code review - use abort controller instead of observable * Revert "Remove old search code and rename UI Setting" This reverts commit 17de9fa. * Remove old search code and rename UI Setting * revert search route * fix event unsubscribe * code review 2 * revert filter * simplify batch done logic * code review * filter items in the beginning * jest Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

This reverts commit 5708c5d.

…ic#84037) This reverts commit 5708c5d.

) This reverts commit 5708c5d.

kibanamachine · 2020-11-23T11:34:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3fcac6e

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`bfetch`	20	21	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`bfetch`	18.6KB	19.9KB	+1.2KB
`data`	974.0KB	973.6KB	-417.0B
`dataEnhanced`	36.3KB	36.3KB	+28.0B
total			+862.0B

History

💚 Build #89467 succeeded 3fcac6e
💔 Build #89466 failed e9103b7
💔 Build #89464 failed 4bd2a96
💚 Build #89450 succeeded bcd894f
💚 Build #88970 succeeded 4a1e69c

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

* master: (67 commits) [Observability] Load hasData call asynchronously (elastic#80644) Implement AnonymousAuthenticationProvider. (elastic#79985) Deprecate `visualization:colorMapping` advanced setting (elastic#83372) [TSVB] [Rollup] Table tab not working with rollup indexes (elastic#83635) Revert "[Search] Search batching using bfetch (elastic#83418)" (elastic#84037) skip flaky suite (elastic#83772) skip flaky suite (elastic#69849) create kbn-legacy-logging package (elastic#77678) [Search] Search batching using bfetch (elastic#83418) [Security Solution] Refactor Timeline flyout to take a full page (elastic#82033) Drop use of console-stamp (elastic#83922) skip flaky suite (elastic#84011 , elastic#84012) Fixed usage of `isReady` for usage collection of alerts and actions (elastic#83760) [maps] support URL drilldowns (elastic#83732) Revert "Added default dedupKey value as an {{alertInstanceId}} to provide grouping functionality for PagerDuty incidents. (elastic#83226)" [code coverage] Update jest config to collect more data (elastic#83804) Added default dedupKey value as an {{alertInstanceId}} to provide grouping functionality for PagerDuty incidents. (elastic#83226) [Security Solution] Give notice when endpoint policy is out of date (elastic#83469) [Security Solution] Sync url state on any changes to query string (elastic#83314) [CI] Initial TeamCity implementation (elastic#81043) ...

* Revert "[Search] Search batching using bfetch (#83418)" This reverts commit 5708c5d. * chore(NA): remove yarn.lock from kbn-legacy-logging

* Revert "[Search] Search batching using bfetch (elastic#83418)" This reverts commit 5708c5d. * chore(NA): remove yarn.lock from kbn-legacy-logging

* Revert "[Search] Search batching using bfetch (#83418)" This reverts commit 5708c5d. * chore(NA): remove yarn.lock from kbn-legacy-logging

Liza K added 5 commits November 10, 2020 14:45

Use bfetch for search (no abort behavior)

20bb351

fix merge

8b4eb1c

Merge branch 'master' of github.com:elastic/kibana into enhance/use-b…

9eebded

…fetch-in-search

Merge branch 'master' of github.com:elastic/kibana into enhance/use-b…

812a06b

…fetch-in-search

Handle request abortion + unit tests

aaa3122

lizozom added v8.0.0 v7.11.0 labels Nov 16, 2020

lizozom requested a review from streamich November 16, 2020 12:24

lizozom self-assigned this Nov 16, 2020

lizozom added the Team:AppArch label Nov 16, 2020

Liza K added 10 commits November 16, 2020 20:08

Merge branch 'master' of github.com:elastic/kibana into enhance/use-b…

b1a2cf2

…fetch-in-search

fix jest

2cdfc47

shim totals in oss

e8fc98f

proper formatting for errors

993e385

jest, types and docs

e89a46f

Merge branch 'master' of github.com:elastic/kibana into enhance/use-b…

b727e49

…fetch-in-search

Fix doc

3de838c

Remove old search code and rename UI Setting

17de9fa

jest mocks

56e7a46

jest

5aaeefa

lizozom marked this pull request as ready for review November 17, 2020 17:16

lizozom requested a review from a team as a code owner November 17, 2020 17:16

lizozom added the release_note:enhancement label Nov 17, 2020

lukasolson reviewed Nov 17, 2020

View reviewed changes

src/plugins/bfetch/public/batching/batch_utils.ts Outdated Show resolved Hide resolved

botelastic bot added Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) labels Nov 17, 2020

lukasolson reviewed Nov 17, 2020

View reviewed changes

Liza K added 2 commits November 18, 2020 00:02

Solve unhanled error

c393416

Use AbortSignal

a1b2979

Merge branch 'master' of github.com:elastic/kibana into enhance/use-b…

c7aebdf

…fetch-in-search

Liza K added 4 commits November 19, 2020 16:00

code review 2

8abff48

Merge branch 'master' of github.com:elastic/kibana into enhance/use-b…

b4193e2

…fetch-in-search

revert filter

9d16439

Merge branch 'master' of github.com:elastic/kibana into enhance/use-b…

4a1e69c

…fetch-in-search

streamich approved these changes Nov 20, 2020

View reviewed changes

kibanamachine and others added 6 commits November 21, 2020 14:58

Merge branch 'master' into enhance/use-bfetch-in-search

bcd894f

simplify batch done logic

d57b8dc

code review

6b9a4f8

Merge branch 'enhance/use-bfetch-in-search' of github.com:lizozom/kib…

4bd2a96

…ana into enhance/use-bfetch-in-search

filter items in the beginning

e9103b7

jest

3fcac6e

lizozom merged commit 5708c5d into elastic:master Nov 22, 2020

lizozom mentioned this pull request Nov 22, 2020

[7.x] [Search] Search batching using bfetch (#83418) #84018

Merged

mistic added a commit to mistic/kibana that referenced this pull request Nov 22, 2020

Revert "[Search] Search batching using bfetch (elastic#83418)"

97ba8a7

This reverts commit 5708c5d.

mistic mentioned this pull request Nov 22, 2020

Revert "[Search] Search batching using bfetch (#83418)" #84037

Merged

mistic added a commit that referenced this pull request Nov 23, 2020

Revert "[Search] Search batching using bfetch (#83418)" (#84037)

d7cdcf6

This reverts commit 5708c5d.

mistic mentioned this pull request Nov 23, 2020

[7.x] Revert "[Search] Search batching using bfetch (#83418)" (#84037) #84038

Merged

mistic added a commit to mistic/kibana that referenced this pull request Nov 23, 2020

Revert "[Search] Search batching using bfetch (elastic#83418)" (elast…

3fb45c7

…ic#84037) This reverts commit 5708c5d.

mistic added a commit that referenced this pull request Nov 23, 2020

Revert "[Search] Search batching using bfetch (#83418)" (#84037) (#84038

b80f2a3

) This reverts commit 5708c5d.

lizozom mentioned this pull request Nov 23, 2020

Security solutions cypress tests should be triggered on PRs related to alerts and actions #83851

Open

mistic added a commit that referenced this pull request Nov 23, 2020

chore(NA): remove yarn lock from kbn legacy logging (#84096)

b2e01c5

* Revert "[Search] Search batching using bfetch (#83418)" This reverts commit 5708c5d. * chore(NA): remove yarn.lock from kbn-legacy-logging

mistic added a commit that referenced this pull request Nov 23, 2020

chore(NA): remove yarn lock from kbn legacy logging (#84096) (#84125)

450571e

* Revert "[Search] Search batching using bfetch (#83418)" This reverts commit 5708c5d. * chore(NA): remove yarn.lock from kbn-legacy-logging

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search] Search batching using bfetch #83418

[Search] Search batching using bfetch #83418

lizozom commented Nov 16, 2020 •

edited

Loading

elasticmachine commented Nov 17, 2020

lukasolson left a comment

lizozom commented Nov 19, 2020 •

edited

Loading

streamich left a comment

streamich Nov 20, 2020

lizozom commented Nov 21, 2020

kibanamachine commented Nov 23, 2020

		// abort when all items were either resolved, rejected or aborted
		Promise.all(promises).then(() => abortController.abort());

[Search] Search batching using bfetch #83418

[Search] Search batching using bfetch #83418

Conversation

lizozom commented Nov 16, 2020 • edited Loading

Summary

Edge cases and things to pay attention to

Query cancellation

Errors

Release Notes

Checklist

For maintainers

elasticmachine commented Nov 17, 2020

lukasolson left a comment

Choose a reason for hiding this comment

lizozom commented Nov 19, 2020 • edited Loading

streamich left a comment

Choose a reason for hiding this comment

streamich Nov 20, 2020

Choose a reason for hiding this comment

lizozom commented Nov 21, 2020

kibanamachine commented Nov 23, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Page load bundle

History

lizozom commented Nov 16, 2020 •

edited

Loading

lizozom commented Nov 19, 2020 •

edited

Loading