Expressions/migrations2 #81281

ppisljar · 2020-10-21T07:49:06Z

Summary

adds migrate and migrateToLatest functions to persistableState type and expression service

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

ppisljar · 2020-10-21T10:10:38Z

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

@@ -257,6 +257,21 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
    return telemetryData;
  }

+  migrate(ast: SerializableState, version: string) {
+    return this.walkAst(cloneDeep(ast) as ExpressionAstExpression, (fn, link) => {


we know the input is ExpressionAstExpression as we don't allow migrating that structure but just the actual functions within

ppisljar · 2020-10-21T10:11:51Z

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

@@ -257,6 +257,21 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
    return telemetryData;
  }

+  migrate(ast: SerializableState, version: string) {
+    return this.walkAst(cloneDeep(ast) as ExpressionAstExpression, (fn, link) => {
+      link = fn.migrations[version](link) as ExpressionAstFunction;


and we expect the migration to return ExpressionAstFunction. thats not the general case with migrations, we might be working with unknown states where the only thing we know is that its serializable. When we do have more knowledge we'll need to do the casting.

ps: open for suggestions on how to better type safe this

Yeah the only other option that comes to mind is instead casting fn.migrations[version] as MigrateFunction<SerializableState, ExpressionAstFunction>, but that doesn't really make a difference here in terms of type safety, so I think we can go with the existing implementation for now

ppisljar · 2020-10-21T10:16:17Z

src/plugins/expressions/common/expression_functions/specs/kibana.ts

@@ -20,6 +20,7 @@
 import { i18n } from '@kbn/i18n';
 import { ExpressionFunctionDefinition } from '../types';
 import { ExpressionValueSearchContext } from '../../expression_types';
+import { MigrateFunction } from '../../../../kibana_utils/common/persistable_state';


all changes to this file will be reverted, its just an example

ppisljar · 2020-10-21T10:17:18Z

src/plugins/expressions/common/expression_functions/specs/kibana.ts

@@ -44,6 +62,10 @@ export const kibana: ExpressionFunctionKibana = {

  args: {},

+  migrations: {
+    '7.10.1': (MIGRATE7_10_1 as any) as MigrateFunction,


this is the part i really don't like. any suggestions ?

Why doesn't TS like this? Looks as if it should work without casting to me, but need to test it

Maybe part of the issue is that in expression_function.ts you are re-defining the migration type instead of reusing MigrateFunction? (I assume that's what's being used behind the scenes here?)

the problem is that MigrateFunction there is <SerializableSTate,SerializableState> where MIGRATE7_10_1 actually work with well defined types

the problem is that MigrateFunction there is <SerializableSTate,SerializableState> where MIGRATE7_10_1 actually work with well defined types

Okay, after looking into this more, there are actually a couple problems. First, migrations isn't in ExpressionFunctionDefinition at all, which is masked by the casting.

More problematic is that ExpressionFunctionDefinition would need to provide a generic type param so you can specify the actual type(s) of your migrations, that way TS doesn't complain because the functions you provide are too specific.

The bummer is that this makes things more verbose and makes ExpressionFunctionDefinition even more complex, but usage would essentially look like:

interface Migrations extends Record<string, any> { '7.10.1': MigrateFunction<MYSTATE_7_10_0, MYSTATE_7_10_1>; } export type ExpressionFunctionKibana = ExpressionFunctionDefinition< 'kibana', ExpressionValueSearchContext | null, object, ExpressionValueSearchContext, ExecutionContext, Migrations >;

And then ExpressionFunctionDefinition would need to take a generic param along the lines of:

Migrations extends Record<string, MigrateFunction> = Record<string, MigrateFunction>

I tested this and it worked for me locally, but LMK what you think. Either way if we are adding migrations to the functions, we should update ExpressionFunctionDefinition

elasticmachine · 2020-10-21T14:03:01Z

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

lukeelmers

Added some initial comments. I think I might be able to provide some more productive suggestions once I actually test this locally and look at the TS errors, but ran out of time to do that. Thought I'd post what I had so far anyway.

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

lukeelmers · 2020-10-21T23:27:30Z

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

@@ -257,6 +257,21 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
    return telemetryData;
  }

+  migrate(ast: SerializableState, version: string) {
+    return this.walkAst(cloneDeep(ast) as ExpressionAstExpression, (fn, link) => {
+      link = fn.migrations[version](link) as ExpressionAstFunction;


Yeah the only other option that comes to mind is instead casting fn.migrations[version] as MigrateFunction<SerializableState, ExpressionAstFunction>, but that doesn't really make a difference here in terms of type safety, so I think we can go with the existing implementation for now

lukeelmers · 2020-10-21T23:32:36Z

src/plugins/kibana_utils/common/persistable_state/index.ts

+   * @param state
+   * @param version
+   */
+  migrateToLatest?: (state: SerializableState, version: string) => P;


I'm a little confused about why migrateToLatest requires a version at all? Shouldn't that just be something that the function knows about internally? Or maybe version is used differently here from migrate and that's what I am getting mixed up about

yeah, this is the version of your state, where in migrate() its the version of migration you want to run. i will make that clear in the jsdocs

Ah okay - yeah, I think the fact that version means two things is just a naming difficulty then

The doc comments clear up most of the confusion, though I think something like toVersion and fromVersion would be even more intuitive.

lukeelmers · 2020-10-21T23:36:23Z

src/plugins/kibana_utils/common/persistable_state/index.ts

+   * @param state
+   * @param version
+   */
+  migrate?: (state: SerializableState, version: string) => SerializableState;


I'm wondering if it would help clean up some of the casting if we provided a FromVersion and ToVersion here as well (as in MigrateFunction). Then in executor you wouldn't need to cast to ExpressionAstFunction and could instead provide it in the generic type params.

i think we should keep this one as it is, as we want to keep types for old versions of state internal (nobody should be messing with that) and migrate really doesn't know anything more about the state coming in apart that it needs to be serializable. (nor does the consumer)

lukeelmers · 2020-10-21T23:37:41Z

src/plugins/expressions/common/expression_functions/specs/kibana.ts

@@ -44,6 +62,10 @@ export const kibana: ExpressionFunctionKibana = {

  args: {},

+  migrations: {
+    '7.10.1': (MIGRATE7_10_1 as any) as MigrateFunction,


Why doesn't TS like this? Looks as if it should work without casting to me, but need to test it

lukeelmers · 2020-10-21T23:39:24Z

src/plugins/expressions/common/expression_functions/specs/kibana.ts

@@ -44,6 +62,10 @@ export const kibana: ExpressionFunctionKibana = {

  args: {},

+  migrations: {
+    '7.10.1': (MIGRATE7_10_1 as any) as MigrateFunction,


Maybe part of the issue is that in expression_function.ts you are re-defining the migration type instead of reusing MigrateFunction? (I assume that's what's being used behind the scenes here?)

ppisljar · 2020-10-22T04:55:13Z

also can we ignore the metrics failled test ? i think its lefit that the bundle grew 27kb as i used semver package which is now added to the bundle. Should i use something else (more lightweight) as the only thing i need is compare two semvers ?

lukeelmers · 2020-10-22T23:45:41Z

also can we ignore the metrics failled test ? i think its lefit that the bundle grew 27kb as i used semver package which is now added to the bundle. Should i use something else (more lightweight) as the only thing i need is compare two semvers ?

Yeah, that's a bit annoying. I guess you could try to minimize the bundle increase by manually implementing gte, since our case is pretty simple in that we can always expect our versions to be number.number.number and therefore don't need to handle strings and some of the other cases that semver does. But I also wouldn't be surprised if we start needing this in other plugins too, in which case it might be worth it to just use the library.

# Conflicts: # src/plugins/expressions/common/executor/executor.ts # src/plugins/expressions/common/expression_functions/specs/kibana.ts # src/plugins/expressions/common/service/expressions_services.ts

kibanamachine · 2020-10-28T15:00:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 134e82c

Metrics [docs]

page load bundle size

id	before	after	diff
`expressions`	202.0KB	204.2KB	+2.2KB

History

💔 Build #84264 failed fba0d44a7f910214f4e368b3c6f92ddd9c027c75
💚 Build #83644 succeeded 31513e7
💔 Build #83031 failed f122bdf
💔 Build #83003 failed 4fcc5f1ae3f6a30d778e9392202df9a9c85112b5

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

lukeelmers

I think I'm on board with this in the current state, and as discussed offline we can come back and do a TS refactor once we see how it works using this in the wild. The casting of the actual migrations isn't ideal, but I also think it's something that would be easy enough to change in a follow up once we have a better strategy for how to approach it.

* master: (71 commits) [Chrome] Extension to append an element to the last breadcrumb (elastic#82015) [Monitoring] Thread pool rejections alert (elastic#79433) [Actions] Fix actionType type on registerType function (elastic#82125) [Security Solution] Modal for saving timeline (elastic#81802) add tests for index pattern switching (elastic#81987) TS project references for share plugin (elastic#82051) [Graph] Fix problem with duplicate ids (elastic#82109) skip 'returns a single bucket if array has 1'. related elastic#81460 Add a link to documentation in the alerts and actions management UI (elastic#81909) [Fleet] fix duplicate ingest pipeline refs (elastic#82078) Context menu trigger for URL Drilldown (elastic#81158) SO management: fix legacy import index pattern selection being reset when switching page (elastic#81621) Fixed dead links (elastic#78696) [Search] Add "restore" to session service (elastic#81924) fix Lens heading structure (elastic#81752) [ML] Data Frame Analytics: Fix feature importance cell value and decision path chart (elastic#82011) Remove legacy app arch items from codeowners. (elastic#82084) [TSVB] Renamed 'positive rate' to 'counter rate' (elastic#80939) Expressions/migrations2 (elastic#81281) [Telemetry] [Schema] remove number type and support all es number types (elastic#81774) ...

adding migrate function to expressions

21e2501

ppisljar added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes Team:AppArch v7.11.0 v8.0.0 labels Oct 21, 2020

ppisljar added 2 commits October 21, 2020 03:05

example migration function (renames kibana -> kibana_input)

d73f31d

updating docs

15d9305

ppisljar force-pushed the expressions/migrations2 branch from 4fcc5f1 to 15d9305 Compare October 21, 2020 10:08

ppisljar commented Oct 21, 2020

View reviewed changes

semver comparison

f122bdf

ppisljar commented Oct 21, 2020

View reviewed changes

ppisljar requested a review from lukeelmers October 21, 2020 10:17

ppisljar marked this pull request as ready for review October 21, 2020 14:02

ppisljar requested a review from a team as a code owner October 21, 2020 14:02

lukeelmers reviewed Oct 21, 2020

View reviewed changes

ppisljar added 3 commits October 26, 2020 06:34

local semvergte implementation

31513e7

Merge branch 'master' into expressions/migrations2

3afcbe8

# Conflicts: # src/plugins/expressions/common/executor/executor.ts # src/plugins/expressions/common/expression_functions/specs/kibana.ts # src/plugins/expressions/common/service/expressions_services.ts

review updates

134e82c

ppisljar force-pushed the expressions/migrations2 branch from fba0d44 to 134e82c Compare October 28, 2020 13:22

lukeelmers approved these changes Oct 30, 2020

View reviewed changes

ppisljar merged commit 076bb73 into elastic:master Oct 30, 2020

ppisljar mentioned this pull request Oct 30, 2020

[7.x] Expressions/migrations2 (#81281) #82101

Merged

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

Expressions/migrations2 (elastic#81281)

b7d3924

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

Expressions/migrations2 (#81281) (#82101)

5e87ce5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expressions/migrations2 #81281

Expressions/migrations2 #81281

ppisljar commented Oct 21, 2020 •

edited

Loading

ppisljar Oct 21, 2020

ppisljar Oct 21, 2020

lukeelmers Oct 21, 2020

ppisljar Oct 21, 2020

ppisljar Oct 21, 2020

lukeelmers Oct 21, 2020

lukeelmers Oct 21, 2020

ppisljar Oct 22, 2020

lukeelmers Oct 22, 2020

elasticmachine commented Oct 21, 2020

lukeelmers left a comment

lukeelmers Oct 21, 2020

lukeelmers Oct 21, 2020

ppisljar Oct 22, 2020

lukeelmers Oct 22, 2020

lukeelmers Oct 30, 2020

lukeelmers Oct 21, 2020

ppisljar Oct 22, 2020

lukeelmers Oct 21, 2020

lukeelmers Oct 21, 2020

ppisljar commented Oct 22, 2020

lukeelmers commented Oct 22, 2020

kibanamachine commented Oct 28, 2020

lukeelmers left a comment

Expressions/migrations2 #81281

Expressions/migrations2 #81281

Conversation

ppisljar commented Oct 21, 2020 • edited Loading

Summary

Checklist

For maintainers

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

Choose a reason for hiding this comment

elasticmachine commented Oct 21, 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

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

ppisljar commented Oct 22, 2020

lukeelmers commented Oct 22, 2020

kibanamachine commented Oct 28, 2020

💚 Build Succeeded

Metrics [docs]

page load bundle size

History

lukeelmers left a comment

Choose a reason for hiding this comment

ppisljar commented Oct 21, 2020 •

edited

Loading