[Dashboard][Embeddable] Create Explicit Diffing System #121241

ThomThomson · 2021-12-14T21:04:05Z

Summary

This PR creates a new system within the embeddable plugin to provide robust input-diffing capabilities. Instead of the dashboard app diffing all of its state the same way, using Lodash's isEqual or equivalent, each child embeddable is in charge of determining whether or not its own current explicit input is equal to another input object given by the dashboard (the last saved state). To implement this, two functions have been added to the generic embeddable:

getExplicitInput - is an explicit function / clarification to the embeddable system which allows embeddables to more easily differentiate between what is their own explicit input, and what is input which was passed to them by a parent. If the embeddable does not have a parent, the results from getExplicitInput and getInput should be the same.
getExplicitInputIsEqual - is a diffing function which takes one input object and diffs it against its current explicitInput. The generic embeddable has a generic and fairly comprehensive implementation of this function which should work in most cases. Any embeddable type which has input keys that should be ignored when diffing can override this function.

Additionally, as an example of why these new API features are useful, the Maps Embeddable was given an implementation of getExplicitInputIsEqual which ignores mapBuffer, thus solving #98180.

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

…and wrote jest tests

elasticmachine · 2021-12-15T15:37:22Z

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

…EqualityChecks

Dosant

I am in progress of processing the change :D

src/plugins/embeddable/public/lib/containers/container.ts

src/plugins/embeddable/public/lib/embeddables/diff_embeddable_input.ts

src/plugins/embeddable/public/lib/embeddables/embeddable.tsx

Co-authored-by: Anton Dosov <[email protected]>

drewdaemon · 2021-12-16T23:06:35Z

It looks like this will work nicely with our in-progress custom Lens equality function. 👍

ThomThomson · 2021-12-20T14:21:33Z

@elasticmachine merge upstream

fix map embeddable equality check

nreese

maps changes LGTM
code review, tested in chrome.

Dosant · 2021-12-21T13:36:37Z

@elasticmachine merge upstream

Dosant · 2021-12-21T14:34:17Z

src/plugins/dashboard/public/application/hooks/use_dashboard_app_state.ts

@@ -262,9 +262,13 @@ export const useDashboardAppState = ({
        dashboardBuildContext.$checkForUnsavedChanges,
      ])
        .pipe(debounceTime(DashboardConstants.CHANGE_CHECK_DEBOUNCE))
-        .subscribe((states) => {
+        .subscribe(async (states) => {


Because of async piece, I think we should put this logic into switchMap to avoid race conditions

Good call @Dosant !
I didn't think of it, because the function in its current form is almost synchronous due to the only async piece being the untilEmbeddableLoaded method which is synchronous unless the dashboard is loading.

That said, this could have easily caused race conditions if a downstream implementation used a long-running task in their diffing method. I added a switchmap operator to the pipe and return when the inner observable is closed. I tested by adding a one second delay to the diff_dashboard_state function, firing off many changes in quick succession, and ensuring that only the latest changes are diffed.

nit: maybe it can be simpler with wrapping promise into an observable using from:

stream$.pipe(switchMap((v) => from(op(v)))).subscribe((v) => { // logic });

… isContainer type guard

ThomThomson · 2021-12-28T15:18:31Z

@elasticmachine merge upstream

vadimkibana

Can the embeddable code be refactored such that we don't need a deep comparison? Instead we would only do a quick equality check? Say

const isInputEqual = embeddable.getInput() === newInput

If it is not possible as-is, because on each embeddable state change we need to merge persisted state with the embeddable local UI state, maybe we could separate those two concepts? And then use a quick equality check for those?

const isPersistentInputEqual = embeddable.getPersistentInput() === newPersistentInput
const isStateEqual = embeddable.getState() === newState

vadimkibana

AppServices code changes LGTM. We discussed the comments in this PR offline and some of the ways of improving embeddable state management.

Not related to this PR, some ideas for future:

Remove deep-clone usage in embeddable input.
Separate embeddable (1) persistable state from (2) UI state.
Start embeddable working group, to discuss embeddable plugin future improvements.

ThomThomson · 2021-12-30T19:41:42Z

@elasticmachine merge upstream

ThomThomson · 2022-01-04T15:27:10Z

@elasticmachine merge upstream

poffdeluxe

LGTM

ThomThomson · 2022-01-06T16:21:02Z

@elasticmachine merge upstream

…EqualityChecks

ThomThomson · 2022-01-10T23:00:30Z

@elasticmachine merge upstream

kibana-ci · 2022-01-11T00:20:43Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`embeddable`	73	74	+1
`maps`	812	813	+1
total			+2

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`	374	384	+10
`maps`	206	208	+2
total			+12

Async chunks

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

id	before	after	diff
`dashboard`	272.7KB	273.1KB	+421.0B
`lens`	1022.7KB	1022.6KB	-118.0B
`maps`	2.6MB	2.6MB	+965.0B
total			+1.2KB

Page load bundle

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

id	before	after	diff
`embeddable`	63.3KB	64.4KB	+1.1KB

Unknown metric groups

API count

id	before	after	diff
`embeddable`	452	465	+13
`maps`	207	209	+2
total			+15

References to deprecated APIs

id	before	after	diff
`dashboard`	254	241	-13

History

💚 Build #16200 succeeded 7103225
💚 Build #16153 succeeded aa8eedf
💚 Build #15710 succeeded 7524d79
💚 Build #15465 succeeded 31af726
💚 Build #15246 succeeded aec43a0
💚 Build #14878 succeeded 2908043

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

stratoula

Vis editors changes LGTM. I tested the embeddable functionality in Chrome and it seems to work fine :)

ThomThomson · 2022-01-11T15:47:49Z

💚 All backports created successfully

Status	Branch	Result
✅	8.0

Note: Successful backport PRs will be merged automatically after passing CI.

Co-authored-by: Anton Dosov <[email protected]> Co-authored-by: nreese <[email protected]> (cherry picked from commit 944ccf1)

…2668) Co-authored-by: Anton Dosov <[email protected]> Co-authored-by: nreese <[email protected]> (cherry picked from commit 944ccf1)

Co-authored-by: Anton Dosov <[email protected]> Co-authored-by: nreese <[email protected]> (cherry picked from commit 944ccf1) # Conflicts: # src/plugins/dashboard/public/application/lib/diff_dashboard_state.ts # x-pack/plugins/maps/server/plugin.ts

… (#124293) * [Dashboard][Embeddable] Create Explicit Diffing System (#121241) Co-authored-by: Anton Dosov <[email protected]> Co-authored-by: nreese <[email protected]> (cherry picked from commit 944ccf1) # Conflicts: # src/plugins/dashboard/public/application/lib/diff_dashboard_state.ts # x-pack/plugins/maps/server/plugin.ts * type fix * replace ecommerce sample map id

ThomThomson added 2 commits December 14, 2021 15:48

initial implementation of explicitInputIsEqual method

504eea6

implemented getExplicitInputIsEqual for map embeddable, fixed issues …

d2b1457

…and wrote jest tests

ThomThomson mentioned this pull request Dec 14, 2021

[Dashboard] [Meta] Tech Debt Roadmap #120182

Closed

15 tasks

add missing await

520ad37

ThomThomson marked this pull request as ready for review December 15, 2021 15:37

ThomThomson requested review from a team as code owners December 15, 2021 15:37

Merge branch 'main' of github.com:elastic/kibana into embeddable/move…

91650a4

…EqualityChecks

Dosant reviewed Dec 16, 2021

View reviewed changes

Update src/plugins/embeddable/public/lib/containers/container.ts

ebb8f62

Co-authored-by: Anton Dosov <[email protected]>

fix map embeddable equality check

8c66f42

kibanamachine and others added 2 commits December 20, 2021 09:21

Merge branch 'main' into embeddable/moveEqualityChecks

088a182

Merge pull request #14 from nreese/unsaved_changes

e7c572e

fix map embeddable equality check

nreese approved these changes Dec 21, 2021

View reviewed changes

Merge branch 'main' into embeddable/moveEqualityChecks

a556667

Dosant reviewed Dec 21, 2021

View reviewed changes

ThomThomson added 2 commits December 21, 2021 17:20

change variable names to last and current explicit input. Implemented…

cd99627

… isContainer type guard

Use switchmap for previous task cancellation

2908043

Merge branch 'main' into embeddable/moveEqualityChecks

aec43a0

vadimkibana reviewed Dec 29, 2021

View reviewed changes

vadimkibana approved these changes Dec 30, 2021

View reviewed changes

Merge branch 'main' into embeddable/moveEqualityChecks

31af726

Merge branch 'main' into embeddable/moveEqualityChecks

7524d79

poffdeluxe approved these changes Jan 5, 2022

View reviewed changes

kibanamachine and others added 3 commits January 6, 2022 11:21

Merge branch 'main' into embeddable/moveEqualityChecks

aa8eedf

Merge branch 'main' of github.com:elastic/kibana into embeddable/move…

81aab35

…EqualityChecks

Remove unused title from LensUnwrapMetaInfo

7103225

Merge branch 'main' into embeddable/moveEqualityChecks

825e366

stratoula approved these changes Jan 11, 2022

View reviewed changes

ThomThomson merged commit 944ccf1 into elastic:main Jan 11, 2022

ThomThomson mentioned this pull request Jan 11, 2022

[8.0] [Dashboard][Embeddable] Create Explicit Diffing System (#121241) #122668

Merged

ThomThomson added a commit that referenced this pull request Jan 11, 2022

[Dashboard][Embeddable] Create Explicit Diffing System (#121241) (#12…

1ab9364

…2668) Co-authored-by: Anton Dosov <[email protected]> Co-authored-by: nreese <[email protected]> (cherry picked from commit 944ccf1)

seang-es mentioned this pull request Jan 26, 2022

7.x backport for [Dashboard][Embeddable] Create Explicit Diffing System #123857

Closed

ThomThomson mentioned this pull request Feb 1, 2022

[7.17] [Dashboard][Embeddable] Create Explicit Diffing System (#121241) #124293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard][Embeddable] Create Explicit Diffing System #121241

[Dashboard][Embeddable] Create Explicit Diffing System #121241

ThomThomson commented Dec 14, 2021

elasticmachine commented Dec 15, 2021

Dosant left a comment •

edited

Loading

drewdaemon commented Dec 16, 2021 •

edited

Loading

ThomThomson commented Dec 20, 2021

nreese left a comment

Dosant commented Dec 21, 2021

Dosant Dec 21, 2021

ThomThomson Dec 22, 2021 •

edited

Loading

Dosant Dec 29, 2021 •

edited

Loading

ThomThomson commented Dec 28, 2021

vadimkibana left a comment •

edited

Loading

vadimkibana left a comment •

edited

Loading

ThomThomson commented Dec 30, 2021

ThomThomson commented Jan 4, 2022

poffdeluxe left a comment

ThomThomson commented Jan 6, 2022

ThomThomson commented Jan 10, 2022

kibana-ci commented Jan 11, 2022

API count

References to deprecated APIs

stratoula left a comment

ThomThomson commented Jan 11, 2022

[Dashboard][Embeddable] Create Explicit Diffing System #121241

[Dashboard][Embeddable] Create Explicit Diffing System #121241

Conversation

ThomThomson commented Dec 14, 2021

Summary

Checklist

elasticmachine commented Dec 15, 2021

Dosant left a comment • edited Loading

Choose a reason for hiding this comment

drewdaemon commented Dec 16, 2021 • edited Loading

ThomThomson commented Dec 20, 2021

nreese left a comment

Choose a reason for hiding this comment

Dosant commented Dec 21, 2021

Dosant Dec 21, 2021

Choose a reason for hiding this comment

ThomThomson Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Dosant Dec 29, 2021 • edited Loading

Choose a reason for hiding this comment

ThomThomson commented Dec 28, 2021

vadimkibana left a comment • edited Loading

Choose a reason for hiding this comment

vadimkibana left a comment • edited Loading

Choose a reason for hiding this comment

ThomThomson commented Dec 30, 2021

ThomThomson commented Jan 4, 2022

poffdeluxe left a comment

Choose a reason for hiding this comment

ThomThomson commented Jan 6, 2022

ThomThomson commented Jan 10, 2022

kibana-ci commented Jan 11, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

References to deprecated APIs

History

stratoula left a comment

Choose a reason for hiding this comment

ThomThomson commented Jan 11, 2022

💚 All backports created successfully

Dosant left a comment •

edited

Loading

drewdaemon commented Dec 16, 2021 •

edited

Loading

ThomThomson Dec 22, 2021 •

edited

Loading

Dosant Dec 29, 2021 •

edited

Loading

vadimkibana left a comment •

edited

Loading

vadimkibana left a comment •

edited

Loading