telemetry and reference extraction/injection for expression service #72438

ppisljar · 2020-07-20T14:11:47Z

Summary

every expression function can provide three additional functions:

telemetry: (state: ExpressionAstFunction, telemetryData: Record<String, any>) => Record<String, any>;

extract: (
    state: ExpressionAstFunction['arguments']
  ) => { args: ExpressionAstFunction['arguments']; references: SavedObjectReference[] };

inject: (
    state: ExpressionAstFunction['arguments'],
    references: SavedObjectReference[]
  ) => ExpressionAstFunction['arguments'];

also, a new flag disabled was added, which allows us to disable certain functions, yet still keep their state handling part in the registry.

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

elasticmachine · 2020-07-20T14:11:50Z

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

lukeelmers

Gave this an initial once over and added a few thoughts...

To do that you would have fn1 provide a migrate function which would return a fn2 function (new name) and fn1 would be marked as disabled.

I know we discussed this a bit before, but I still wish there were a better way for us to version functions so that we wouldn't need to create completely new ones. But I can't really think of a way to do this without requiring users to explicitly provide a version number or something similar in the expression.

src/plugins/expressions/common/expression_functions/types.ts

src/plugins/expressions/common/executor/executor.ts

src/plugins/expressions/common/expression_functions/types.ts

ppisljar · 2020-07-27T12:26:13Z

@elasticmachine merge upstream

timroes

Done some quick review (didn't do an in-depth code review on the walk function right now). Left a couple of minor comments. Overall it looks good to me, but have some general notes:

What is the version we pass into the migration method? Will we use the Kibana version for that? Since all functions need to use the same version in their migration there isn't anything else that really would make sense? If we anyway require this to be the Kibana version, would it make sense not to pass that into as a parameter but always have that function use the current Kibana version automatically?
While I understand who needs to call inject and extract, I think we need some clear documentation on when to call migrate on the executor service? (My current understand is, it needs to called EVERY time before you are executing an expression that was stored somewhere?)
In general I think we need to provide some more documentation on those functions, once we're going to merge it :-)

src/plugins/expressions/common/execution/execution.ts

src/plugins/expressions/common/expression_functions/types.ts

ppisljar · 2020-07-28T11:35:20Z

@timroes i agree regarding the docs, i added some jsdocs. in general, you should never need to use inject or extract unless you are creating a new saved object type (like lens does). you should never need to call migrate unless you are getting the state from some storage (url, localstorage, user input?, saved objects inside saved object migration scripts), so hopefully most of the code would not be doing it and we have services that perform this.

the version we pass in is the version of the state we are sending to migrate function. the migrate function will always return state of the latest version. open to suggestions here, we could remove the version and have the migrate function have to figure out the actual version of state passed in on its own ?

if we don't need to provide the version this gives us a nice benefit of not having to ever call migrate function, as we could call it internally when creating the execution. that would mean that above functions are only called from saved object types internally.

ppisljar · 2020-07-30T13:56:31Z

@elasticmachine merge upstream

lukeelmers

On the whole the updates here make sense to me, I still need to test though. Added a few other comments/questions.

lukeelmers · 2020-07-30T22:09:53Z

src/plugins/expressions/common/execution/execution.ts

@@ -220,6 +220,10 @@ export class Execution<
        return createError({ message: `Function ${fnName} could not be found.` });
      }

+      if (fn.disabled) {
+        return createError({ message: `Function ${fnName} is disabled.` });


I wonder if we should be running these through i18n as they are automatically surfaced in the UI on the client?

src/plugins/expressions/common/expression_functions/types.ts

src/plugins/expressions/common/service/expressions_services.ts

ppisljar · 2020-08-03T12:21:58Z

@elasticmachine merge upstream

lukeelmers

LGTM -- reviewed the updates & I think this all makes sense.

lukeelmers · 2020-08-06T22:54:18Z

src/plugins/expressions/common/executor/executor.ts

+        // if any of arguments are expressions we should migrate those first
+        link.arguments = mapValues(fnArgs, (asts, argName) => {
+          return asts.map((arg) => {
+            if (typeof arg === 'object') {


will the fact that typeof null === 'object' cause any problems here?

i guess it should be fine since null isn't a valid value for an AST arg anyway

ppisljar · 2020-08-10T10:26:51Z

@elasticmachine merge upstream

timroes

Code LGTM.

I would ask if we could please add a couple of tests still for the inject/extract functions, that will especially test, that not only the functions will be called, but do some tests where the inject/extract functions do actually a rewrite of the AST that this is rewritten correctly in the returned AST.

timroes · 2020-08-12T07:38:08Z

src/plugins/expressions/common/executor/executor.ts

+    const newAst = this.walkAst(ast, (fn, link) => {
+      const { args, references } = fn.extract(link.arguments);
+      link.arguments = args;
+      references.forEach((ref) => allReferences.push(ref));


🎨 allReferences.push(...references) maybe reads slightly nicer.

flash1293

Added a few nits

flash1293 · 2020-08-14T10:50:27Z

src/plugins/expressions/common/executor/executor.ts

+    ast: ExpressionAstExpression,
+    action: (fn: ExpressionFunction, link: ExpressionAstFunction) => void
+  ) {
+    const newAST = cloneDeep(ast);


Is it necessary to do this cloneDeep at every recursion step? It seems like it would be enough to clone the original ast once, then use that copy for the complete walk

flash1293 · 2020-08-14T10:57:08Z

src/plugins/expressions/common/service/expressions_services.ts

+   * @param state old version of expression AST
+   */
+  public readonly migrate = (state: ExpressionAstExpression) => {
+    return this.executor.migrate(state);


Should we pass a version identifier down here? Currently the migrate function has to work with clues in the ast to decide whether something needs to be migrated or not. This might not be possible in all cases.
E.g. There is an expression function search type="_myType" - the underscore has to be removed due to changes in the underlying system, but search type="__myType" is a valid value. If we know from what version we are migrating we can always decide to do the right thing, if we just get the current state without knowing what's the source version, we might not know whether to change something or not.

the problem is we might not know the version at all. we might add an optional version parameter, but we will still need to figure out by clues what version it actually is if that is not provided, so i would leave this out for now. we can always add it later.

flash1293 · 2020-08-14T11:00:45Z

src/plugins/expressions/common/expression_functions/types.ts

@@ -87,6 +94,32 @@ export interface ExpressionFunctionDefinition<
   */
  fn(input: Input, args: Arguments, context: Context): Output;

+  /**
+   * migrate function


I would suggest to include in these docs the order of how migrations/injections/extractions are applied.
Something like:
"This function will be executed for arguments of an expression function before being executed for the current expression function. This means all arguments will already be migrated when this method is called on an expression function"

ppisljar · 2020-09-28T10:22:16Z

@elasticmachine merge upstream

Co-authored-by: Luke Elmers <lukeelmers@gmail.com>

# Conflicts: # src/plugins/expressions/common/service/expressions_services.ts

kibanamachine · 2020-10-12T16:11:04Z

💚 Build Succeeded

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

Metrics [docs]

page load bundle size

id	before	after	diff
`expressions`	194.6KB	198.4KB	+3.7KB

History

💔 Build #81058 failed 53fec07
💔 Build #77804 failed a9d28d7
💔 Build #77813 failed 7bbb98d
💔 Build #77785 failed 8582a4e443a027654cc4e60853649a16129406c6
💔 Build #77749 failed 7e6862b4a811c98c91b46b375f9b113be78da120

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

…lastic#72438)

…72438) (#80289)

ppisljar added WIP Work in progress Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppArch labels Jul 20, 2020

ppisljar requested review from timroes and lukeelmers July 20, 2020 14:11

ppisljar requested a review from a team as a code owner July 20, 2020 14:11

ppisljar added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Jul 21, 2020

timroes mentioned this pull request Jul 21, 2020

[Lens] Saved object should register reference to index patterns to keep exports/imports consistent #55906

Closed

lukeelmers reviewed Jul 21, 2020

View reviewed changes

timroes reviewed Jul 27, 2020

View reviewed changes

src/plugins/expressions/common/execution/execution.ts Outdated Show resolved Hide resolved

src/plugins/expressions/common/expression_functions/types.ts Outdated Show resolved Hide resolved

ppisljar removed the WIP Work in progress label Jul 28, 2020

lukeelmers reviewed Jul 30, 2020

View reviewed changes

ppisljar force-pushed the expressions/migrations branch from a5e4b3d to 1980d16 Compare August 6, 2020 13:10

lukeelmers mentioned this pull request Aug 6, 2020

[skip-ci] POC: Using expression types to "migrate" state passed in args. #57354

Closed

lukeelmers approved these changes Aug 6, 2020

View reviewed changes

timroes approved these changes Aug 12, 2020

View reviewed changes

flash1293 reviewed Aug 14, 2020

View reviewed changes

ppisljar force-pushed the expressions/migrations branch from 695b496 to 49b03d0 Compare September 14, 2020 10:32

ppisljar force-pushed the expressions/migrations branch 2 times, most recently from 884d127 to 7e6862b Compare September 29, 2020 07:42

ppisljar and others added 2 commits September 29, 2020 02:44

migration and reference extraction/injection for expression service

ace72ce

Update src/plugins/expressions/common/expression_functions/types.ts

c3d5acd

Co-authored-by: Luke Elmers <lukeelmers@gmail.com>

ppisljar and others added 10 commits September 29, 2020 02:44

adding jsdocs and improving types

9d626ee

adding unit tests

2ff26e8

removing version argument from migrate

1023a16

fix

dad5b11

Update src/plugins/expressions/common/expression_functions/types.ts

3f1c01f

Co-authored-by: Luke Elmers <lukeelmers@gmail.com>

Update src/plugins/expressions/common/service/expressions_services.ts

c055fdc

Co-authored-by: Luke Elmers <lukeelmers@gmail.com>

translating errors

1012597

adding telemetry function

0e0aaab

updating based on review

3a2c38d

switching to PersistableState types

6d12cfb

ppisljar force-pushed the expressions/migrations branch from 7e6862b to 8582a4e Compare September 29, 2020 09:45

fixes

a9d28d7

ppisljar force-pushed the expressions/migrations branch from 8582a4e to a9d28d7 Compare September 29, 2020 10:29

ppisljar changed the title ~~migration and reference extraction/injection for expression service~~ telemetry and reference extraction/injection for expression service Sep 29, 2020

ppisljar added 3 commits September 29, 2020 04:16

making debug info serializable

7bbb98d

Merge branch 'master' into expressions/migrations

53fec07

# Conflicts: # src/plugins/expressions/common/service/expressions_services.ts

updating error types

eddaa48

ppisljar added v7.11.0 v8.0.0 and removed v7.10.0 labels Oct 12, 2020

ppisljar merged commit 1a3c059 into elastic:master Oct 12, 2020

ppisljar mentioned this pull request Oct 13, 2020

[7.x] telemetry and reference extraction/injection for expression service (#72438) #80289

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Oct 13, 2020

telemetry and reference extraction/injection for expression service (e…

b925d75

…lastic#72438)

ppisljar added a commit that referenced this pull request Oct 13, 2020

telemetry and reference extraction/injection for expression service (#…

1ff65d0

…72438) (#80289)

ppisljar mentioned this pull request Nov 4, 2020

Handling saved object references in expressions #59960

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

telemetry and reference extraction/injection for expression service #72438

telemetry and reference extraction/injection for expression service #72438

ppisljar commented Jul 20, 2020 •

edited

Loading

elasticmachine commented Jul 20, 2020

lukeelmers left a comment

ppisljar commented Jul 27, 2020

timroes left a comment

ppisljar commented Jul 28, 2020

ppisljar commented Jul 30, 2020

lukeelmers left a comment

lukeelmers Jul 30, 2020

ppisljar commented Aug 3, 2020

lukeelmers left a comment

lukeelmers Aug 6, 2020

lukeelmers Aug 6, 2020

ppisljar commented Aug 10, 2020

timroes left a comment

timroes Aug 12, 2020

flash1293 left a comment

flash1293 Aug 14, 2020

flash1293 Aug 14, 2020

ppisljar Sep 14, 2020

flash1293 Aug 14, 2020

ppisljar commented Sep 28, 2020

kibanamachine commented Oct 12, 2020

telemetry and reference extraction/injection for expression service #72438

telemetry and reference extraction/injection for expression service #72438

Conversation

ppisljar commented Jul 20, 2020 • edited Loading

Summary

Checklist

elasticmachine commented Jul 20, 2020

lukeelmers left a comment

Choose a reason for hiding this comment

ppisljar commented Jul 27, 2020

timroes left a comment

Choose a reason for hiding this comment

ppisljar commented Jul 28, 2020

ppisljar commented Jul 30, 2020

lukeelmers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppisljar commented Aug 3, 2020

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

ppisljar commented Aug 10, 2020

timroes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 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

ppisljar commented Sep 28, 2020

kibanamachine commented Oct 12, 2020

💚 Build Succeeded

Metrics [docs]

page load bundle size

History

ppisljar commented Jul 20, 2020 •

edited

Loading