Expression executor service #36885

flash1293 · 2019-05-22T13:29:43Z

Summary

Adds a simple version of the expression executor service to the data plugin because this functionality will be used in a wide variety of plugins and apps in Kibana.

Context

This will be used in Lens to implement #36187
Over time this service is expected to grow (hopefully in a backwards compatible manner) into all the things outlined here #25374

The first simple API run(expression, domElement) will become a convenience function on top of these functions. It only provides a really simple API to run and render given expressions. Comments in the code mark unfinished parts which are not necessary to get this basic functionality.

Integration

This is part of the data plugin which is currently a shimmed NP plugin. To get as close as possible to the NP way of doing things, the stateful dependencies (getInterpreter and renderersRegistry) are passed down from the main plugin entry point src/legacy/core_plugins/data/public/index.ts.

As types are not available yet because the interpreter plugin itself is still JS, this integration uses local types for now. These are expected to be removed once the interpreter plugin is moved to TS.

elasticmachine · 2019-05-22T13:29:47Z

Pinging @elastic/kibana-app-arch

elasticmachine · 2019-05-22T14:47:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-22T17:18:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…or-service

elasticmachine · 2019-05-23T12:06:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar

some initial comments, will take a deeper look tomorrow

src/legacy/core_plugins/data/public/expression_executor/expression_executor_service.tsx

ppisljar · 2019-05-23T13:07:10Z

src/legacy/core_plugins/data/public/index.ts

  }

  public setup() {
    return {
      indexPatterns: this.indexPatterns.setup(),
      search: this.search.setup(),
      query: this.query.setup(),
+      expressionExecutor: this.expressionExecutor.setup(null, {


lets name it interpreter service

Sounds good - this means we will merge it with the current interpreter plugin, right?

I'd also be in favor of simply calling it expressions instead, since it will also house stuff that isn't explicitly related to the interpreter. (e.g. the fromExpression and toExpression methods are technically more related to the parser than the interpreter)

Then usage could look like this

data.expressions.toExpression(myAst); data.expressions.fromExpression(myString); data.expressions.interpreter.run(myExpression); (or maybe data.expressions.interpret(myExpression) ?) data.expressions.interpreter.getInterpreter(); data.expressions.interpreter.getRegistries(); // or whatever

I'm not against calling it interpreter service either, I'm just thinking about what would be the easiest thing to explain to people and most understandable for a dev.

++
data.expressions.fromAst() and data.expression.toAst() probably make more sense then
data.expressions.run() is also fine and data.expressions.ExpressionRenderer component
it would be nice if we could have data.expressions.interpreter instead of data.expressions.getInterpreter()
and then the registries:
data.expressions.functionRegistry, data.expressions.renderersRegistry, data.expressions.typesRegistry

src/legacy/core_plugins/data/public/index.ts

ppisljar · 2019-05-23T13:10:51Z

src/legacy/core_plugins/data/public/expression_executor/expression_executor_service.tsx

+
+import { CoreSetup } from 'kibana/public';
+import { useRef, useEffect } from 'react';
+import React from 'react';


react shouldn't be part of the API, we can have react components wrapping the api but pls put in separate file

So we don't do stateful components at all? Or is this just about the React import in the service itself?

The nice thing about delivering components like that is that they are usable directly (<ExpressionRenderer expression="..." />) instead of having to pass the stateful thing in (e.g. <ExpressionRenderer runFn={plugins.expressionExecutor.run} expression="..." />)

We can postpone this and I just remove the react component for now - there is already a pretty similar component in Lens which can handle the integration.

Yeah agreed with @ppisljar here -- I'd suggest providing a run function to just run the expression, and a in a separate file a "convenience" method for React, which uses run under the hood.

Then both can be returned from setup as part of the public contract. This way people have the option of using the React wrapper or the run method directly. Also some people might want to run an expression and get the output without having to use a renderer.

elasticmachine · 2019-05-23T13:25:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers

Haven't tested, but did a pass through the code and added some ideas.

lukeelmers · 2019-05-23T14:05:32Z

src/legacy/core_plugins/data/public/index.ts

  }

  public setup() {
    return {
      indexPatterns: this.indexPatterns.setup(),
      search: this.search.setup(),
      query: this.query.setup(),
+      expressionExecutor: this.expressionExecutor.setup(null, {


I'd also be in favor of simply calling it expressions instead, since it will also house stuff that isn't explicitly related to the interpreter. (e.g. the fromExpression and toExpression methods are technically more related to the parser than the interpreter)

Then usage could look like this

data.expressions.toExpression(myAst); data.expressions.fromExpression(myString); data.expressions.interpreter.run(myExpression); (or maybe data.expressions.interpret(myExpression) ?) data.expressions.interpreter.getInterpreter(); data.expressions.interpreter.getRegistries(); // or whatever

I'm not against calling it interpreter service either, I'm just thinking about what would be the easiest thing to explain to people and most understandable for a dev.

src/legacy/core_plugins/data/public/index.ts

lukeelmers · 2019-05-23T14:13:10Z

src/legacy/core_plugins/data/public/index.ts

  search: SearchSetup;
  query: QuerySetup;
 }

+export { ExpressionExecutorSetup } from './expression_executor';
+


I don't think this needs to be exported directly since downstream consumers will just need the DataSetup interface, unless there was a reason you needed it by itself @flash1293?

I don't necessarily need it by itself, I just planned to save the instance of this service in my consumer plugin like private expressionsService: ExpressionsServiceSetup;.
If this isn't exported directly I have to plug it out of the DataSetup like private expressionsService: DataSetup['expressionsService'];.

But that would also work, I very happy to follow best practices here. Reading your other comments, these best practices are exporting only the leaf types which are actually passed around, so I will change it to that (the react component and its props plus the signature of the "raw" run function).

lukeelmers · 2019-05-23T14:15:37Z

src/legacy/ui/public/visualize/loader/pipeline_helpers/run_pipeline.ts

@@ -25,15 +25,15 @@ import { getInterpreter } from 'plugins/interpreter/interpreter';
 import { Adapters } from 'ui/inspector';
 import { Filters, Query, TimeRange } from 'ui/visualize';

-interface InitialContextObject {
+export interface InitialContextObject {


Where are these being consumed? Are you exporting so that you can get access to them from lens directly?

I'm working on moving expression types to OSS so there might be some overlap here... initially I will expose these types from the interpreter plugin, but that will ultimately get moved to data as well.

They are used in the expressions service inside data to help with the local interim types there because atm there are not other types available. +1 for more complete expression types. As soon as your PR is merged I will fix the type references in the expressions service.

lukeelmers · 2019-05-23T14:21:43Z

src/legacy/core_plugins/data/public/expression_executor/expression_executor_service.tsx

+  onDestroy: (fn: () => void) => void;
+}
+
+export interface RenderFunction {


Are these just exported for the purposes of testing, or is the intent to allow them to be imported by downstream consumers?

If we want them to be imported downstream, the convention we & core have been using is to re-export the types from the top-level plugin definition, so that people don't need to know the path to the directory to import from.

e.g.

import { data } from 'plugins/data'; // we want to avoid this as it makes your directory structure part of your public contract import { RenderFunction } from 'plugins/data/expression_executor'; // so this is how we've been solving the issue so far import { data, RenderFunction } from 'plugins/data'; // (the platform team has also discussed eventually namespacing any types by service if it becomes confusing. e.g. Expressions.RenderFunction)

Makes sense, thanks. These types are only exported for the tests, I changed my code to re-export public stuff in index

lukeelmers · 2019-05-23T14:25:21Z

src/legacy/core_plugins/data/public/expression_executor/expression_executor_service.tsx

+export class ExpressionExecutorService {
+  private interpreterInstance: Interpreter | null = null;
+
+  // TODO core won't ever be null once this is switched to the new platform


If _core isn't actually needed as part of this service, I wonder if we should just be passing it into setup as explicitly null or undefined? That way you know at a glance that the expression executor doesn't rely on any core services, without digging into the file.

This was a misunderstanding from my side, sorry. I thought the individual services were planned to become plugins in the future because of the very similar call signature. I removed the core parameter completely, this also should make clear the service isn't depending on it

lukeelmers · 2019-05-23T15:28:10Z

src/legacy/core_plugins/data/public/expression_executor/expression_executor_service.tsx

+
+import { CoreSetup } from 'kibana/public';
+import { useRef, useEffect } from 'react';
+import React from 'react';


Yeah agreed with @ppisljar here -- I'd suggest providing a run function to just run the expression, and a in a separate file a "convenience" method for React, which uses run under the hood.

Then both can be returned from setup as part of the public contract. This way people have the option of using the React wrapper or the run method directly. Also some people might want to run an expression and get the output without having to use a renderer.

flash1293 · 2019-05-24T07:39:37Z

@ppisljar @lukeelmers Thanks for your review, I think I addressed all the points.

About naming: I renamed the service to "ExpressionsService", the functions are still called run and ExpressionRenderer. I have no strong opinion about them - if you come up with better names please let me know. It should somehow say "Interpret and execute an expression and render the result if a separate dom element was also passed to the function". run might be too generic, not sure here. I know about the previous discussion of passing just the expression and getting a Handler object which does the other things, but I would veto doing this now, this function is meant to be a shortcut for that multi step process.

ppisljar · 2019-05-24T08:05:44Z

src/legacy/core_plugins/data/public/expressions/expressions_service.ts

+    interpreter: { renderersRegistry, getInterpreter },
+  }: ExpressionsServiceDependencies) {
+    const run = createRunFn(
+      renderersRegistry,


we should also expose interpreter and renderersRegistry but no need to do it in this PR.

src/legacy/core_plugins/data/public/index.ts

ppisljar · 2019-05-24T08:10:54Z

src/legacy/core_plugins/data/public/expressions/expression_runner.ts

+    ast,
+    { type: 'null' },
+    {
+      getInitialContext: () => ({}),


this should be one of the properties to the function, whoever runs the expression might want to provide initial context.

ppisljar · 2019-05-24T08:12:20Z

src/legacy/core_plugins/data/public/expressions/expression_runner.ts

+    typeof expressionOrAst === 'string' ? fromExpression(expressionOrAst) : expressionOrAst;
+  const response = await interpreter.interpretAst(
+    ast,
+    { type: 'null' },


context should be one of the properties, needs to be provided from outside and we should default to null.

src/legacy/core_plugins/data/public/expressions/expressions_service.ts

ppisljar

code LGTM, some comments, but we can also apply all those later when we migrate visualize embeddable handler to use this.

elasticmachine · 2019-05-24T08:51:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

flash1293 · 2019-05-24T09:49:05Z

@ppisljar @lukeelmers I added an options parameter for context and getInitialContext if you want to take a final look.

elasticmachine · 2019-05-24T11:10:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lizozom · 2019-05-29T07:23:00Z

@flash1293 Will this be backported to 7.x?

@flash1293

Merged after syncing with @flash1293

start work on expression executor service

3c74514

flash1293 added the :AppArch label May 22, 2019

add react component encapsulating the same thing

af85958

flash1293 added 2 commits May 23, 2019 12:49

add tests and improve typings and docs

935b008

add experimental notice

0d99f0a

flash1293 changed the title ~~[WIP] Expression executor service~~ Expression executor service May 23, 2019

flash1293 added 2 commits May 23, 2019 13:45

add expression executor to data plugin setup type

37a6d55

Merge remote-tracking branch 'upstream/master' into expression-execut…

c211b74

…or-service

flash1293 requested review from lukeelmers, lizozom, ppisljar and timroes May 23, 2019 12:06

flash1293 marked this pull request as ready for review May 23, 2019 13:01

ppisljar reviewed May 23, 2019

View reviewed changes

lukeelmers reviewed May 23, 2019

View reviewed changes

flash1293 added 3 commits May 24, 2019 09:28

Review fixes

802b6f1

change old service name

ff1d950

fix types and enforce correctness

0eeceb2

ppisljar reviewed May 24, 2019

View reviewed changes

src/legacy/core_plugins/data/public/index.ts Show resolved Hide resolved

ppisljar reviewed May 24, 2019

View reviewed changes

src/legacy/core_plugins/data/public/expressions/expressions_service.ts Outdated Show resolved Hide resolved

ppisljar approved these changes May 24, 2019

View reviewed changes

flash1293 added 2 commits May 24, 2019 11:40

make context and getInitialContext configurable

f1a59e0

import run_pipeline types

a2ac4f6

flash1293 merged commit 3232fc8 into elastic:master May 24, 2019

flash1293 deleted the expression-executor-service branch May 24, 2019 12:07

flash1293 mentioned this pull request May 29, 2019

[7.x] Expressions service (#36885) #37315

Merged

flash1293 added a commit to flash1293/kibana that referenced this pull request May 29, 2019

Expressions service (elastic#36885)

ce5ac0f

lizozom pushed a commit that referenced this pull request May 29, 2019

Expressions service (#36885) (#37315)

38d74c4

Merged after syncing with @flash1293

flash1293 added non-issue Indicates to automation that a pull request should not appear in the release notes v7.3.0 v8.0.0 labels Jun 3, 2019

lukeelmers mentioned this pull request Jun 24, 2019

「AppArch」Interpreter 👉 New Platform #39329

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expression executor service #36885

Expression executor service #36885

flash1293 commented May 22, 2019 •

edited

Loading

elasticmachine commented May 22, 2019

elasticmachine commented May 22, 2019

elasticmachine commented May 22, 2019

elasticmachine commented May 23, 2019

ppisljar left a comment

ppisljar May 23, 2019

flash1293 May 23, 2019

lukeelmers May 23, 2019

ppisljar May 24, 2019

ppisljar May 23, 2019

flash1293 May 23, 2019 •

edited

Loading

lukeelmers May 23, 2019

elasticmachine commented May 23, 2019

lukeelmers left a comment

lukeelmers May 23, 2019

lukeelmers May 23, 2019

flash1293 May 24, 2019 •

edited

Loading

lukeelmers May 23, 2019

flash1293 May 24, 2019

lukeelmers May 23, 2019

flash1293 May 24, 2019

lukeelmers May 23, 2019

flash1293 May 24, 2019

lukeelmers May 23, 2019

flash1293 commented May 24, 2019

ppisljar May 24, 2019

ppisljar May 24, 2019

ppisljar May 24, 2019

ppisljar left a comment

elasticmachine commented May 24, 2019

flash1293 commented May 24, 2019

elasticmachine commented May 24, 2019

lizozom commented May 29, 2019

Expression executor service #36885

Expression executor service #36885

Conversation

flash1293 commented May 22, 2019 • edited Loading

Summary

Context

Integration

elasticmachine commented May 22, 2019

elasticmachine commented May 22, 2019

💚 Build Succeeded

elasticmachine commented May 22, 2019

💚 Build Succeeded

elasticmachine commented May 23, 2019

💚 Build Succeeded

ppisljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 23, 2019

💔 Build Failed

lukeelmers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented May 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

elasticmachine commented May 24, 2019

💚 Build Succeeded

flash1293 commented May 24, 2019

elasticmachine commented May 24, 2019

💚 Build Succeeded

lizozom commented May 29, 2019

flash1293 commented May 22, 2019 •

edited

Loading

flash1293 May 23, 2019 •

edited

Loading

flash1293 May 24, 2019 •

edited

Loading