[Embeddable] Clientside migration system #162986

ThomThomson · 2023-08-02T14:58:56Z

Summary

This PR changes the versioning scheme used by Dashboard Panels and by value Embeddables, and introduces a new clientside system that can migrate Embeddable Inputs to their latest versions.

Embeddable Migration System

Note
This PR does not affect the existing serverside Embeddable migrations in any way - it only adds a secondary system on top that can be used without adding an additional saved object migration to Dashboard. Older Embeddable migrations will continue to run against older inputs.

How are versions stored?

After this PR Embeddable versions and the migrations between are no longer tied to the Kibana version. Instead, each Embeddable type maintains its own versioning information. This is stored as follows:

the Latest version is stored in the Embeddable Factory. If a latest version is not present, no migrations will be run against this embeddable type.
the Current version is stored in the Embeddable's Input.

When do migrations get run?

The migrations get run as part of the embeddableFactory.create process. This means that every Embeddable that gets rendered by any application will receive migrations. Persisting an Embeddable in a saved object no longer means worrying about keeping them up to date or writing any migrations. Just save the input as-is, and pass it to an Embeddable Renderer, which will do the rest.

On Dashboards... there are some extra steps.

Dashboard Embeddable Migrations

Usually the process of just rendering an Embeddable and having the migrations happen naturally is enough. For Dashboard this is not the case because:

Dashboards need to know if a migration has happened in order to prompt the user to save.
Dashboards remember their last saved state to allow for resetting and diff checking. Resetting a Dashboard should not undo migrations!

For these reasons, Dashboards run a function clientside on saved object load. This function migrates all panels and Controls on the Dashboard before the Embeddables are even created. If this function detects any changes a save prompt will be shown

Save Prompt

Saving back-propagates any changes from the migrations back to the saved object, and can make loading faster next time because migrations can be skipped. To encourage users to do this if changes have been detected and the Dashboard is in edit mode a prompt will be shown next to the unsaved changes badge. Here is the prompt in context:

Old Version Key

There is a version key in the Dashboard panel type which has historically stored the Kibana version the Dashboard was last saved on.

In this PR that version key was removed and version information was moved inside the Input key so that all Embeddables can be versioned, not just the ones stored in Dashboards. As part of that change, the two usages of the old version key needed to be updated to reflect that the older version key can be missing.

The two usages were:

To check if the panel was saved in a version before 7.3.0 in order to use a legacy version of the extract references function. This PR removes that legacy version of the extract method, because the migration that required it would never be run on a version after 8.0 because upgrading to 8.x requires a stop at 7.17 first.
To check if a panel injected via the URL is older than 7.3.0 in order to abort loading it. This has been updated to also check directly for the hallmarks of an older style panel, like the lack of a gridData key.

How to test

src/plugins/image_embeddable/public/image_embeddable/image_embeddable_factory.tsx

public latestVersion = '1.0.1';
public migrations = {
  '1.0.1': (input: ImageEmbeddableInput) => {
    input.imageConfig.altText = `Neat migration! Previous alt text was "${input.imageConfig.altText}"`;
    return input;
  },
};

The new save prompt should show, but the unsaved changes badge should not show. Resetting the input, switching back and forth between view & edit mode and any action done on the Dashboard should not undo the migration, or run the migration again on already migrated input.

If the migration runs against the input more than once, you'll see the alt text become: Neat migration! Previous alt text was "Neat migration! Previous alt text was "Whatever your alt text was originally"".

What is not covered?

This PR is a very basic and simple version of the clientside migration system. More functionality and features can be added to it as they are needed. Because of this, some pieces are missing:

Right now, the Dashboard does not run clientside Control Group Migrations. This is because in order to do so, the Dashboard Saved Object would need to optionally store a Control Group version, which would require changes in the saved object schema, and the content management. Before we can implement any migrations for the Control Group itself we will need to make these changes. Fortunately, migrations are quite rare for the Control Group itself.
Drilldown Migrations are not covered at the moment because in order for them to be implemented we would need to extend the Embeddable Enhancements system to be able to store version information independent from the actual Embeddable version. There are currently no migrations registered for drilldowns, but before we add one, we would need to ensure that a system for properly versioning them is in place.
Propagation from child Embeddables back to Container
The function that runs on Dashboard saved object load means that most of the time, when the embeddable create function is called on Dashboards there are no more migrations to be run. If for any reason there are migrations to be run at this point, the embeddable inheritance structure means they get immediately undone because the Container's child input is considered the source of truth. This could happen when:
- The Dashboard has state in the session storage. If a user has unsaved panels in their session storage, they refresh the Dashboard, and Kibana has updated, the Embeddable create method will detect if any new migrations have been registered and will run them, but the container will pick up the changes and immediately undo them.
- There are panels from an older version injected via the URL from a snapshot link. Again the migrations will get run, but will immediately get undone.
Implementing this back-propagation on the existing Embeddables framework would require ugly workarounds and changes that would affect the stability of the Embeddable platform. Once we move to a less prescriptive inheritance structure, this will not be a problem.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

cla-checker-service · 2023-08-02T14:59:02Z

💚 CLA has been signed

…te --fix'

…nSystem

…nSystem' into embeddable/migrationSystem

…until Dashboard save, fix jest tests.

…in place. Throw error if Embeddable factory registered with migrations but no latest version

…nSystem

ThomThomson · 2023-08-11T18:27:54Z

...rd/common/dashboard_saved_object/persistable_state/dashboard_saved_object_references.test.ts

@@ -51,190 +51,6 @@ const commonAttributes: DashboardAttributes = {
  title: '',
 };

-describe('legacy extract references', () => {


All of these tests & the legacy extract references function have been removed because they are no longer used. It is not possible to upgrade to 8.10 without first upgrading to 7.17, and URLs from before 7.3 are unsupported as of 8.7.0 with this PR

What if a user imports 7.3 saved objects from saved object import UI into 8.11 instance? Will these migrations be needed in that case? Or will saved object import fail?

7.3 used JSON exporting which is incompatible with versions over 8.0 which require NDJSON. 7.17 was the latest version that supported JSON, so the user would have to import them to 7.17 first which would run all the required migrations.

…mbeddableconfig and version

…nSystem

ThomThomson · 2023-08-11T22:08:03Z

@gchaps @amyjtechwriter here is the current copy for the save prompt:

I'm not sure what to say here exactly, but I'd like to get across that we've run some extra code this time because of a Kibana version upgrade, and if you save your dashboard we won't have to run that extra code anymore!

…nSystem

ThomThomson · 2023-08-14T19:33:42Z

@elasticmachine merge upstream

ThomThomson · 2023-08-16T14:07:55Z

@elasticmachine merge upstream

ThomThomson · 2023-08-16T16:11:30Z

@gchaps thank you for the suggestion! I've updated the copy to:

One or more panels on this dashboard have been updated to a new version. Save the dashboard so it loads faster next time.

@andreadelrio - do you think having a second badge as per the current design is the right way to notify a Dashboard author that an upgrade has happened and saving their Dashboard is a good idea?

elasticmachine · 2023-08-16T18:08:48Z

Pinging @elastic/kibana-presentation (Team:Presentation)

andreadelrio · 2023-08-16T18:33:04Z

@andreadelrio - do you think having a second badge as per the current design is the right way to notify a Dashboard author that an upgrade has happened and saving their Dashboard is a good idea?

@ThomThomson I think the second badge would work however I'm not sure users will know that Unsaved changes and Save recommended are trying to communicate something different. Will they be curious to go and hover and see the tooltip? Maybe it's just a matter of a different label.

If we wanted to bring even more attention to the recommendation we could add either a EuiBeacon + EuiTourStep on the Save button or use a EuiToast.

ThomThomson · 2023-08-16T19:41:08Z

Oh! I really like the EuiBeacon / EuiTourStep combination. Do you think this should replace the badge entirely? Or do you think we should keep both?

andreadelrio · 2023-08-16T20:30:19Z

Oh! I really like the EuiBeacon / EuiTourStep combination. Do you think this should replace the badge entirely? Or do you think we should keep both?

I would keep both.

nreese · 2023-08-17T16:55:54Z

src/plugins/dashboard/common/content_management/v1/types.ts

+  /**
+   * This version key was used to store Kibana version information from versions 7.3.0 -> 8.11.0.
+   * As of version 8.11.0, the versioning information is now per-embeddable-type and is stored on the
+   * embeddable's input. (embeddableConfig in this type).


Just to clarify, this means that version will no longer be populated for SavedDashboardPanel and will be undefined for newly created dashboards?

Yes exactly. The version key will no longer exist in the Dashboard panels themselves. It will exist in the input and only if the Embeddable factory has a latestVersion defined.

nreese · 2023-08-17T16:57:45Z

...rd/common/dashboard_saved_object/persistable_state/dashboard_saved_object_references.test.ts

@@ -51,190 +51,6 @@ const commonAttributes: DashboardAttributes = {
  title: '',
 };

-describe('legacy extract references', () => {


What if a user imports 7.3 saved objects from saved object import UI into 8.11 instance? Will these migrations be needed in that case? Or will saved object import fail?

nreese

LGTM
code review, tested in chrome

ppisljar

persistable state changes LGTM

TinaHeiligers · 2023-08-21T17:30:59Z

I've had 👀 on behalf of the Core team. The things this PR doesn't cover make sense in terms of priorities and will need to be done at some point. If there are issues, please link back to the initial work here. If not, they need to be created a.s.a.p

…nSystem

kibana-ci · 2023-08-22T18:53:12Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dashboard`	386	388	+2
`embeddable`	108	116	+8
total			+10

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`embeddable`	439	442	+3
`kibanaUtils`	416	417	+1
total			+4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`dashboard`	358.9KB	368.6KB	+9.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`dashboard`	41.8KB	33.5KB	-8.3KB
`embeddable`	69.9KB	78.6KB	+8.7KB
`kibanaUtils`	73.6KB	73.6KB	+4.0B
total			+403.0B

Unknown metric groups

API count

id	before	after	diff
`dashboard`	100	101	+1
`embeddable`	537	542	+5
`kibanaUtils`	609	610	+1
total			+7

History

💚 Build #151109 succeeded b8eace5
💚 Build #151011 succeeded 066b33e
💔 Build #150696 failed 0179c02
💔 Build #150291 failed 2d24d7c
💔 Build #150123 failed ea0ac68
💚 Build #149782 succeeded c1231c6

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

ThomThomson · 2023-08-22T19:17:32Z

@TinaHeiligers I've created issues for the followups identified in this PR:

#164499, #164500 & #164501

ThomThomson · 2023-08-22T19:19:44Z

@andreadelrio I looked into the Toast, and the EuiTour options for drawing more attention to the save recommended badge when clientside migrations have run, and both of those options would require more effort than I anticipated. I'm considering delaying that until we are certain that folks aren't saving their dashboards in these cases, and / or the lack of back-propagation starts to cause problems.

rudolf · 2023-08-23T11:04:29Z

...lugins/dashboard/public/services/dashboard_content_management/lib/migrate_dashboard_input.ts

+      if (panelMigrationRun) anyMigrationRun = true;
+      panel.explicitInput = newInput as DashboardPanelState['explicitInput'];
+    } else if (factory.latestVersion) {
+      // by reference panels are always considered to be of the latest version


So a by reference panel that needs a migration would not trigger a "Save recommended"? If a user does edit and save the dashboard would we save the migrated by reference panels too?

For the time being, by reference Embeddables should never need a migration via this system. Any embeddables that can be both by reference or by value will need to implement the same migration function in both the SO migrations, and the clientside Embeddable migrations.

So currently, no it won't trigger a "Save recommended", but if the user saves, we will save the latestVersion key into the Embeddable's input.

A by reference embeddable may need migration from this system when we implement drilldown migrations, but won't until then.

ThomThomson added 3 commits August 1, 2023 18:25

Clientside Migration System initial commit

4a0c759

migrate last saved input alongside embeddables

c913e6b

Move migrations to happen on Dashboard load.

2502a61

Remove legacy version on save, fix tests & types

6936bee

ThomThomson force-pushed the embeddable/migrationSystem branch from dcc92cd to 6936bee Compare August 2, 2023 15:14

kibanamachine and others added 11 commits August 2, 2023 15:33

[CI] Auto-commit changed files from 'node scripts/check_mappings_upda…

6a67ad5

…te --fix'

Merge remote-tracking branch 'upstream/main' into embeddable/migratio…

276eaa9

…nSystem

fix types, fix tests

07f15e1

Merge remote-tracking branch 'refs/remotes/origin/embeddable/migratio…

21eb86c

…nSystem' into embeddable/migrationSystem

Revert changes to Dashboard initialization, keep version information …

e0f6282

…until Dashboard save, fix jest tests.

Make latest version optional, return early when no latest version is …

0472b08

…in place. Throw error if Embeddable factory registered with migrations but no latest version

fix jest test, remove unused pre 730 extract references, fix types

5bdf39b

Merge remote-tracking branch 'upstream/main' into embeddable/migratio…

6eca80a

…nSystem

remove control group version info

ab8d0cc

revert versioning info for control group

628f454

remove legacy extract references tests, fix functional tests.

0366ed8

ThomThomson commented Aug 11, 2023

View reviewed changes

ThomThomson added 2 commits August 11, 2023 14:30

Update migration count, determine if panel is too old via gridData, e…

1db7411

…mbeddableconfig and version

Merge remote-tracking branch 'upstream/main' into embeddable/migratio…

c1231c6

…nSystem

ThomThomson added 2 commits August 14, 2023 10:30

Merge remote-tracking branch 'upstream/main' into embeddable/migratio…

5c879e0

…nSystem

Remove comment

ea0ac68

Merge branch 'main' into embeddable/migrationSystem

2d24d7c

Merge branch 'main' into embeddable/migrationSystem

0179c02

Merge branch 'main' into embeddable/migrationSystem

066b33e

update copy

b8eace5

ThomThomson force-pushed the embeddable/migrationSystem branch from 0a94fe9 to b8eace5 Compare August 16, 2023 16:45

ThomThomson marked this pull request as ready for review August 16, 2023 18:08

ThomThomson requested review from a team as code owners August 16, 2023 18:08

nreese self-requested a review August 17, 2023 15:55

nreese reviewed Aug 17, 2023

View reviewed changes

botelastic bot added the Feature:Embedding Embedding content via iFrame label Aug 17, 2023

nreese approved these changes Aug 17, 2023

View reviewed changes

ppisljar approved these changes Aug 21, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into embeddable/migratio…

e24f33c

…nSystem

ThomThomson merged commit 26389e5 into elastic:main Aug 22, 2023

kibanamachine added v8.11.0 backport:skip This commit does not require backporting labels Aug 22, 2023

This was referenced Aug 22, 2023

[Embeddables] Migration System for Control Group #164499

Closed

[Embeddables] Migration System for Drilldowns #164500

Closed

[Embeddables] Migration System for Unsaved State #164501

Closed

rudolf reviewed Aug 23, 2023

View reviewed changes

[Embeddable] Clientside migration system #162986

[Embeddable] Clientside migration system #162986

Uh oh!

Conversation

ThomThomson commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Embeddable Migration System

How are versions stored?

When do migrations get run?

Dashboard Embeddable Migrations

Save Prompt

Old Version Key

How to test

What is not covered?

Checklist

Uh oh!

cla-checker-service bot commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomThomson commented Aug 11, 2023

Uh oh!

ThomThomson commented Aug 14, 2023

Uh oh!

ThomThomson commented Aug 16, 2023

Uh oh!

ThomThomson commented Aug 16, 2023

Uh oh!

elasticmachine commented Aug 16, 2023

Uh oh!

andreadelrio commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThomThomson commented Aug 16, 2023

Uh oh!

andreadelrio commented Aug 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers commented Aug 21, 2023

Uh oh!

kibana-ci commented Aug 22, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Uh oh!

ThomThomson commented Aug 22, 2023

Uh oh!

ThomThomson commented Aug 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ThomThomson commented Aug 2, 2023 •

edited

Loading

cla-checker-service bot commented Aug 2, 2023 •

edited

Loading

andreadelrio commented Aug 16, 2023 •

edited

Loading