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

bfetch (2) #53711

Merged
merged 59 commits into from
Jan 16, 2020
Merged

bfetch (2) #53711

merged 59 commits into from
Jan 16, 2020

Conversation

streamich
Copy link
Contributor

@streamich streamich commented Dec 20, 2019

Summary

Closes #44326
Partially addresses: #44393

  • Moves batched_fetch to the NP, now it is plugins.bfetch.batchedFunction.
  • Introduces plugins.bfetch.addBatchProcessingRoute on server-side which works with client-side plugins.bfetch.batchedFunction.
  • Moves legacy interpreter server-side function execution to NP expressions plugin.
  • Improves README for bfetch plugin.
  • Adds bfetch_explorer example plugin.

To see the bfetch_explorer example plugin run

yarn start --run-examples

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers


Dev Docs

Request batching and response streaming functionality of legacy Interpreter plugin has been moved out into a separate bfetch Kibana platform plugin. Now every plugin can create server endpoints and browser wrappers that can batch HTTP requests and stream responses back.

As an example, we will create a batch processing endpoint that receives a number then doubles it
and streams it back. We will also consider the number to be time in milliseconds
and before streaming the number back the server will wait for the specified number of
milliseconds.

To do that, first create server-side batch processing route using addBatchProcessingRoute.

plugins.bfetch.addBatchProcessingRoute<{ num: number }, { num: number }>(
  '/my-plugin/double',
  () => ({
    onBatchItem: async ({ num }) => {
      // Validate inputs.
      if (num < 0) throw new Error('Invalid number');
      // Wait number of specified milliseconds.
      await new Promise(r => setTimeout(r, num));
      // Double the number and send it back.
      return { num: 2 * num };
    },
  })
);

Now on client-side create double function using batchedFunction.
The newly created double function can be called many times and it
will package individual calls into batches and send them to the server.

const double = plugins.bfetch.batchedFunction<{ num: number }, { num: number }>({
  url: '/my-plugin/double',
});

Note: the created double must accept a single object argument ({ num: number } in this case)
and it will return a promise that resolves into an object, too (also { num: number } in this case).

Use the double function.

double({ num: 1 }).then(console.log, console.error); // { num: 2 }
double({ num: 2 }).then(console.log, console.error); // { num: 4 }
double({ num: 3 }).then(console.log, console.error); // { num: 6 }

@streamich 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. Team:AppArch v7.6.0 v7.7.0 v8.0.0 labels Dec 20, 2019
@elasticmachine
Copy link
Contributor

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

@streamich streamich changed the title Bfetch 2 bfetch (2) Dec 20, 2019
@elastic elastic deleted a comment from elasticmachine Dec 30, 2019
@lukasolson lukasolson mentioned this pull request Jan 7, 2020
4 tasks
src/plugins/expressions/server/legacy.ts Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
src/plugins/bfetch/README.md Outdated Show resolved Hide resolved
src/plugins/bfetch/README.md Outdated Show resolved Hide resolved
@streamich streamich marked this pull request as ready for review January 8, 2020 10:42
@streamich streamich requested a review from a team as a code owner January 8, 2020 10:42
@streamich
Copy link
Contributor Author

streamich commented Jan 10, 2020

Question about fetchStreaming: Why does it return both an observable and a promise? Couldn't we just return the observable and any consumers can call toPromise on it if they want the promise behavior?

Yes, it does not have to return a promise, I removed it. It was for legacy reasons, because prior ajax_stream would return a promise and collect individual messages through a callback, now that the callback is replaced by an observable we don't need the promise.

Regarding TimedItemBuffer and ItemBuffer, is there any reason these classes need to be separate or can we have only a TimedItemBuffer that implements all of the behavior? As far as I can tell there's no place that one is used independently (but I could be wrong).

They are separate because they provide different functionality, they could be combined into one class, but since its already written separately, would like to leave it as is.

Also, I know you had mentioned adding in functional tests from the example plugin, is that something you're wanting to tackle in this PR or in a future one?

Yes, I added functional tests here.

@streamich
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Recent code changes LGTM but I didn't pull down and test (although I did yesterday and everything looked good)

@streamich
Copy link
Contributor Author

@elasticmachine merge upstream

@streamich streamich removed the v7.6.0 label Jan 14, 2020
@streamich
Copy link
Contributor Author

@elasticmachine merge upstream

@streamich streamich merged commit 5c19c82 into elastic:master Jan 16, 2020
streamich added a commit that referenced this pull request Jan 16, 2020
* feat: 🎸 implement ItemBuffer

* test: 💍 add tests for ItemBuffer

* feat: 🎸 add TimedItemBuffer

* test: 💍 add TimedItemBuffer tests

* feat: 🎸 add createBatchedFunction

* chore: 🤖 save wip on higher level batching

* test: 💍 add createBatchedFunction tests

* feat: 🎸 implement createStreamingBatchedFunction() method

* refactor: 💡 rename "data" key to "result"

* feat: 🎸 return error in "error" key in legacy protocol

* feat: 🎸 add server-side to Expressions plugin

* refactor: 💡 move interpreter server-side registries to NP

* feat: 🎸 implement bfetch.addBatchProcessingRoute

* feat: 🎸 improve streaming and batching func to pass request

* feat: 🎸 initial setup of new expressions batching endpoint

* feat: 🎸 expose bfetch.batchedFunction() function

* feat: 🎸 add of() function

of() function awaits a promise and converts it to a 3-tuple representing
its state.

* refactor: 💡 move normalizeError() to /common

* feat: 🎸 improve createStreamingBatchedFunction() function

* refactor: 💡 move GET /api/interpreter/fns to the New Platform

* feat: 🎸 move batched_fetch to the New Platform

* feat: 🎸 implement legacy interpreter batching on server in NP

* feat: 🎸 switch legacy interpreter server functions to NP

* chore: 🤖 remove unused import

* fix: 🐛 correct expressions mocks

* test: 💍 fix batching tests after refactor

* test: 💍 stub bfetch plugin explorer

* test: 💍 add routing and app structure to bfetch_explorer

* test: 💍 add server-side to bfetch_explorer

* test: 💍 create <DoubleInteger> component in bfetch_explorer

* test: 💍 improve bfetch_explorer

* test: 💍 add <CountUntil> demo to bfetch_explorer

* test: 💍 by default redirect to first bfetch_explorer example

* test: 💍 add error example to bfetch_explorer

* docs: ✏️ improve bfetch docs

* docs: ✏️ improve bfetch server-side docs

* chore: 🤖 address self-review comments

* fix: 🐛 use new core ES data client, remove unuseed import

* fix: 🐛 remove unused interface import

* Update examples/bfetch_explorer/public/components/count_until/index.tsx

Co-Authored-By: Lukas Olson <[email protected]>

* Update examples/bfetch_explorer/public/components/double_integers/index.tsx

Co-Authored-By: Lukas Olson <[email protected]>

* Update src/plugins/bfetch/common/buffer/item_buffer.ts

Co-Authored-By: Lukas Olson <[email protected]>

* Update src/plugins/kibana_utils/common/of.ts

Co-Authored-By: Lukas Olson <[email protected]>

* docs: ✏️ add batchedFunction params to README

* refactor: 💡 rename onflush to onFlush

* feat: 🎸 make maxItemAge optional in TimedItemBuffer

* refactor: 💡 remove promise from fetchStreaming

* test: 💍 add bfetch_explorer functional tests

* test: 💍 rename test plugin to kbn_tp_bfetch_explorer

* fix: 🐛 use stream instead of removed promise

* fix: 🐛 use correct tsconfig.json in bfetch test plugin

* feat: 🎸 add kbn_tp_bfetch_explorer server-side files to tsconfi

Co-authored-by: Lukas Olson <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Lukas Olson <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020
* feat: 🎸 implement ItemBuffer

* test: 💍 add tests for ItemBuffer

* feat: 🎸 add TimedItemBuffer

* test: 💍 add TimedItemBuffer tests

* feat: 🎸 add createBatchedFunction

* chore: 🤖 save wip on higher level batching

* test: 💍 add createBatchedFunction tests

* feat: 🎸 implement createStreamingBatchedFunction() method

* refactor: 💡 rename "data" key to "result"

* feat: 🎸 return error in "error" key in legacy protocol

* feat: 🎸 add server-side to Expressions plugin

* refactor: 💡 move interpreter server-side registries to NP

* feat: 🎸 implement bfetch.addBatchProcessingRoute

* feat: 🎸 improve streaming and batching func to pass request

* feat: 🎸 initial setup of new expressions batching endpoint

* feat: 🎸 expose bfetch.batchedFunction() function

* feat: 🎸 add of() function

of() function awaits a promise and converts it to a 3-tuple representing
its state.

* refactor: 💡 move normalizeError() to /common

* feat: 🎸 improve createStreamingBatchedFunction() function

* refactor: 💡 move GET /api/interpreter/fns to the New Platform

* feat: 🎸 move batched_fetch to the New Platform

* feat: 🎸 implement legacy interpreter batching on server in NP

* feat: 🎸 switch legacy interpreter server functions to NP

* chore: 🤖 remove unused import

* fix: 🐛 correct expressions mocks

* test: 💍 fix batching tests after refactor

* test: 💍 stub bfetch plugin explorer

* test: 💍 add routing and app structure to bfetch_explorer

* test: 💍 add server-side to bfetch_explorer

* test: 💍 create <DoubleInteger> component in bfetch_explorer

* test: 💍 improve bfetch_explorer

* test: 💍 add <CountUntil> demo to bfetch_explorer

* test: 💍 by default redirect to first bfetch_explorer example

* test: 💍 add error example to bfetch_explorer

* docs: ✏️ improve bfetch docs

* docs: ✏️ improve bfetch server-side docs

* chore: 🤖 address self-review comments

* fix: 🐛 use new core ES data client, remove unuseed import

* fix: 🐛 remove unused interface import

* Update examples/bfetch_explorer/public/components/count_until/index.tsx

Co-Authored-By: Lukas Olson <[email protected]>

* Update examples/bfetch_explorer/public/components/double_integers/index.tsx

Co-Authored-By: Lukas Olson <[email protected]>

* Update src/plugins/bfetch/common/buffer/item_buffer.ts

Co-Authored-By: Lukas Olson <[email protected]>

* Update src/plugins/kibana_utils/common/of.ts

Co-Authored-By: Lukas Olson <[email protected]>

* docs: ✏️ add batchedFunction params to README

* refactor: 💡 rename onflush to onFlush

* feat: 🎸 make maxItemAge optional in TimedItemBuffer

* refactor: 💡 remove promise from fetchStreaming

* test: 💍 add bfetch_explorer functional tests

* test: 💍 rename test plugin to kbn_tp_bfetch_explorer

* fix: 🐛 use stream instead of removed promise

* fix: 🐛 use correct tsconfig.json in bfetch test plugin

* feat: 🎸 add kbn_tp_bfetch_explorer server-side files to tsconfi

Co-authored-by: Lukas Olson <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move server-side interpreter code to Canvas
4 participants