bfetch #52888

streamich · 2019-12-12T14:53:56Z

Summary

Ports ajax_stream to the New Platform
Creates bfetch plugin to host all batching and streaming services
Switches Interpreter to use bfetch for response streaming
Adds bfetch.addStreamingResponseRoute() on the server side, which will be used in the future for implementing batch request processing on the server.
There will be further work on this to create a higher level abstraction to allow any plugin to easily create a batched endpoint, e.g. batched_fetch will be moved to NP and integreated into bfetch plugin such that any plugin can create a batching endpoint.

Partially adresses: #44393
Part of: #46909

Checklist

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] 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
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

Dev Docs

ajax_stream has been ported to the New Platform. Use fetchStreaming() method of bfetch plugin instead.

import { npStart } from 'ui/new_platform';

const { stream, promise } = npStart.plugins.bfetch.fetchStreaming({ url: 'http://elastic.co' });

elasticmachine · 2019-12-12T14:55:12Z

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

chrisdavies

Everything looks pretty good to me. Switching to rxjs instead of some random, ad-hoc thing is definitely an improvement. I'm a bit concerned about the way errors are swallowed on the server here, though.

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

src/legacy/core_plugins/interpreter/public/canvas/batched_fetch.ts

src/plugins/bfetch/server/plugin.ts

src/plugins/bfetch/server/streaming/create_streaming_response_stream.ts

src/plugins/kibana_utils/common/defer.ts

src/plugins/bfetch/server/plugin.ts

src/plugins/bfetch/public/streaming/split.test.ts

src/plugins/bfetch/public/plugin.ts

src/plugins/bfetch/README.md

src/legacy/core_plugins/interpreter/public/canvas/load_legacy_server_function_wrappers.ts

src/plugins/bfetch/public/plugin.ts

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

src/plugins/bfetch/server/plugin.ts

src/plugins/bfetch/server/types.ts

src/plugins/kibana_utils/public/index.ts

src/plugins/bfetch/server/streaming/create_streaming_response_stream.ts

chrisdavies

LGTM. w/ tweaks (logging should use the Kibana logger vs console)

streamich

@chrisdavies I've changed it to use Kibana logger.

With regards to errors, we agreed with @chrisdavies that those are lowest level protocol errors and there is no benefit to streaming them back. Instead a higher level abstraction will implement its own application errors that will indeed be streamed back.

@lizozom I'm not sure what you mean, what are you proposing to retry?

lukeelmers

My only main concern here is using the export * in our public/index and server/index files -- we are already exporting internal things here, and this is a brand new plugin. Having just started the process of cleaning these up in the data plugin, I can testify that it is incredibly time consuming to retroactively go back and figure out what pieces of a plugin are truly public. So I think we should be explicit from the start on this one so we don't create more work for ourselves later.

One broader question I'm interested in: In the original POC, this was introduced as part of the data plugin. What was the motivation for changing the approach to making this a standalone plugin? Are there technical or design reasons why this is necessary?

It feels like a relatively small domain to justify its own plugin, but I'm sure there are pros and cons which we could discuss further.

src/plugins/kibana_utils/public/index.ts

src/plugins/bfetch/server/index.ts

src/plugins/bfetch/public/index.ts

streamich · 2019-12-13T21:40:59Z

@lukeelmers with regards bfetch being a separate plugin, I always thought it should be a separate plugin thus it says so in the issue. In size it is actually comparable to any of our plugins. For example expressions plugin is just over 5,000 lines of code, inspector plugin is also around 5,000 line of code, etc.. This first PR adds about 1,500 lines to the bfetch plugin and once it is fully complete it will be at least 3-4x larger. In terms of domain it seems like a perfect fit for a plugin. If it was part of data plugin, we would have a circular dependency between data plugin and expressions plugin.

lukeelmers

Thanks for making those export changes! Rest of the code LGTM.

If it was part of data plugin, we would have a circular dependency between data plugin and expressions plugin.

IMO this is good enough reason to keep bfetch separate for now.

elasticmachine · 2019-12-13T22:59:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: bd99173

History

💚 Build #15478 succeeded 95956fe
💔 Build #15457 failed a7d75f8
💚 Build #15127 succeeded c0d2027

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

* feat: 🎸 add bfetch plugin stub * feat: 🎸 add sample routes * feat: 🎸 implement streaming response * feat: 🎸 add Defer class * refactor: 💡 move Defer inot /common folder * feat: 🎸 add fromStreamingXhr() method * feat: 🎸 add split method * feat: 🎸 add fetchStreaming() function * test: 💍 fix test after refactor * test: 💍 add tests for fetStreaming() method * refactor: 💡 move removeLeadingSlash() to /common folder * feat: 🎸 expor stateful fetchStreaming() throuh plugin contract * chore: 🤖 clean up bfetch * chore: 🤖 prepare to replace ajax_stream by bfetch * Change ajax_stream to use new-line delimited JSON * refactor: 💡 move batched_fetch to use bfetch service * refactor: 💡 make use of defer() utility from kibana_utils * chore: 🤖 remove ajax_stream library * fix: 🐛 fix tests and inject fetchStreaming() method as dep * refactor: 💡 make split() operator more readable * refactor: 💡 improvee PR according to feedback * docs: ✏️ add fetchStreaming() reference * refactor: 💡 use NP logger, rename to createNDJSONStream() * chore: 🤖 adress Luke's review comments * chore: 🤖 add missing type

streamich and others added 19 commits December 12, 2019 14:53

feat: 🎸 add bfetch plugin stub

927866c

feat: 🎸 add sample routes

ea17a77

feat: 🎸 implement streaming response

0aea21e

feat: 🎸 add Defer class

b68137a

refactor: 💡 move Defer inot /common folder

d161fc5

feat: 🎸 add fromStreamingXhr() method

3aeb16c

feat: 🎸 add split method

6032cc5

feat: 🎸 add fetchStreaming() function

997e826

test: 💍 fix test after refactor

54f65af

test: 💍 add tests for fetStreaming() method

64d0398

refactor: 💡 move removeLeadingSlash() to /common folder

07488f9

feat: 🎸 expor stateful fetchStreaming() throuh plugin contract

91e9a59

chore: 🤖 clean up bfetch

eff350a

chore: 🤖 prepare to replace ajax_stream by bfetch

d66599b

Change ajax_stream to use new-line delimited JSON

5720ff7

refactor: 💡 move batched_fetch to use bfetch service

1add02b

refactor: 💡 make use of defer() utility from kibana_utils

115f6ed

chore: 🤖 remove ajax_stream library

377b6d0

fix: 🐛 fix tests and inject fetchStreaming() method as dep

c0d2027

streamich requested a review from a team as a code owner December 12, 2019 14:53

streamich assigned chrisdavies and lukasolson Dec 12, 2019

streamich added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review Team:AppArch v7.6.0 v7.7.0 v8.0.0 labels Dec 12, 2019

streamich changed the title ~~Bfetch~~ bfetch Dec 12, 2019

chrisdavies suggested changes Dec 12, 2019

View reviewed changes

lukasolson mentioned this pull request Dec 12, 2019

NP batching/streaming service #44393

Closed

4 tasks

streamich commented Dec 12, 2019

View reviewed changes

lizozom reviewed Dec 13, 2019

View reviewed changes

streamich added 4 commits December 13, 2019 15:48

chore: 🤖 catch up with master

d602fed

refactor: 💡 make split() operator more readable

8caf7b7

refactor: 💡 improvee PR according to feedback

b55f6b4

docs: ✏️ add fetchStreaming() reference

896c93a

chrisdavies approved these changes Dec 13, 2019

View reviewed changes

refactor: 💡 use NP logger, rename to createNDJSONStream()

a7d75f8

streamich commented Dec 13, 2019

View reviewed changes

chore: 🤖 catch up with master

95956fe

lukeelmers requested changes Dec 13, 2019

View reviewed changes

streamich added 3 commits December 13, 2019 22:16

chore: 🤖 catch up with master

ca1915b

chore: 🤖 adress Luke's review comments

d42e93e

chore: 🤖 add missing type

bd99173

streamich requested a review from lukeelmers December 13, 2019 21:41

lukeelmers approved these changes Dec 13, 2019

View reviewed changes

streamich merged commit 5fee62b into elastic:master Dec 14, 2019

streamich mentioned this pull request Dec 14, 2019

[7.x] bfetch (#52888) #53065

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bfetch #52888

bfetch #52888

streamich commented Dec 12, 2019 •

edited

Loading

elasticmachine commented Dec 12, 2019

chrisdavies left a comment

chrisdavies left a comment

streamich left a comment •

edited

Loading

lukeelmers left a comment •

edited

Loading

streamich commented Dec 13, 2019 •

edited

Loading

lukeelmers left a comment

elasticmachine commented Dec 13, 2019

bfetch #52888

bfetch #52888

Conversation

streamich commented Dec 12, 2019 • edited Loading

Summary

Checklist

For maintainers

Dev Docs

elasticmachine commented Dec 12, 2019

chrisdavies left a comment

Choose a reason for hiding this comment

chrisdavies left a comment

Choose a reason for hiding this comment

streamich left a comment • edited Loading

Choose a reason for hiding this comment

lukeelmers left a comment • edited Loading

Choose a reason for hiding this comment

streamich commented Dec 13, 2019 • edited Loading

lukeelmers left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 13, 2019

💚 Build Succeeded

History

streamich commented Dec 12, 2019 •

edited

Loading

streamich left a comment •

edited

Loading

lukeelmers left a comment •

edited

Loading

streamich commented Dec 13, 2019 •

edited

Loading