Embeddable telemetry and reference extraction/injection by ppisljar · Pull Request #74352 · elastic/kibana

ppisljar · 2020-08-05T11:26:14Z

Summary

adds .telemetry .extract and .inject functions to embeddable service, which can take in any embeddable input and perform associated action on it.

embeddable service exposes .telemetry .inject and .extract function which then:

runs those functions for base embeddable input
checks embeddable factory registry for specified embeddable type and runs those functions on it
checks enhancements registry for present enhancements (new registry) and runs those functions on them

same set of function was also added to dynamic actions registry:

dynamic action enhancement uses uiActionsEnhanced plugin (calls those methods on it_)
uiActionsEnhanced plugin checks dynamic action registry and runs those functions on the found item

As we need this functionality on the server as well and embeddable plugin and uiActionEnhanced plugins didn't have server contract until now, that was added as well, together with all the mentioned registried and functions.

note that telemetry/extract/inject functions were not added for any embeddable and should be done as a followup. most of them also have nested state dependency tree (for example visualize embeddable will need to check visualizations registry (which will need to provide all above functions, as well as every visualization registered), and some visualizations will need to go further like getting search source functions, which in turn shall get filter, query and timerange functions.

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
This was checked for keyboard-only and screenreader accessibility
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

elasticmachine · 2020-08-05T11:26:17Z

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

Dosant

Just picked around during our meeting ( when you presented it )
Will try to play and understand a bit later.

We have this one place where we do migration for dashboard drilldown:
https://github.com/elastic/kibana/blob/master/x-pack/plugins/embeddable_enhanced/public/embeddables/embeddable_action_storage.ts#L122

Should we try to refactor it to use new approach in this pr?

src/plugins/embeddable/public/plugin.tsx

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

x-pack/plugins/ui_actions_enhanced/public/drilldowns/drilldown_definition.ts

src/plugins/embeddable/public/types.ts

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

stacey-gammon · 2020-08-12T21:17:39Z

Can you add tests for migrating base, extended and enhanced state?

stacey-gammon · 2020-08-13T13:24:27Z

src/plugins/embeddable/public/plugin.tsx

input.id != type - this won't work.

I think the migrate function will have to ask for type, separate from input.

stacey-gammon · 2020-08-13T13:37:31Z

This is generally inline with my thinking. Few things though:

tests to make sure it works as expected. I have been playing around with an isolated example on CodeSandbox with tests and I had to loop through multiple possible versions for it to work correctly. I think you also need version passed in to migrate, and I also had to pass in type.
Add type guards to avoid casting and any. Throw (or return) errors if the input doesn't match expectations.

Example:

  migrate(state: unknown, version: number) {
    if (version === 1 && isValid1State(state)) {
      return state;
    } else if (version === 0 && isValid0State(state)) {
      return migrateBarChart0(state);
    } else {
      throw new Error("Invalid state");
    }
  }
}

Somewhere in here we should warn that we are only temporarily passing the full input state to the factory.migrate functions and the plan is to extract extended state out and store it separately to ensure the migrations only have access to the state they own.

ppisljar · 2020-09-16T09:45:48Z

@elasticmachine merge upstream

Dosant

lgtm,
thanks for addressing latest feedback and merging clean up pr

P.S.: not sure why CI is going crazy. I assume small ts export / reexport mistake

Dosant · 2020-09-16T10:57:52Z

x-pack/plugins/ui_actions_enhanced/public/drilldowns/url_drilldown/types.ts

// disable can be also remove?

Dosant · 2020-09-16T10:58:56Z

x-pack/examples/ui_actions_enhanced_examples/public/dashboard_to_discover_drilldown/types.ts

I think also worth disabling this rule for ui_actions_enhanced_examples

flash1293

Left a few more comments. Especially the ones about ts-ignore and the way telemetry works should be addressed before merging this

flash1293 · 2020-09-16T13:35:00Z

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

  >,
  TSavedObjectAttributes extends SavedObjectAttributes = SavedObjectAttributes
-> {
+  // @ts-ignore


This is worrying in such a basic type - can we avoid it to make sure we don't introduce actually wrong types here in the future?

flash1293 · 2020-09-16T13:37:18Z

src/plugins/embeddable/server/plugin.ts

+    return {
+      telemetry: this.telemetry,
+      extract: this.extract,
+      inject: this.inject,


We can probably live without it for now, but as Tim mentioned once we have migrations on that level we also need to add them on the server as well and migrate everything before fetching telemetry

adding migrations is the next step for this which will be done in a followup

flash1293 · 2020-09-16T13:38:46Z

src/plugins/embeddable/server/plugin.ts

+
+    telemetryBaseEmbeddableInput(state, telemetryData);
+    if (factory) {
+      factory.telemetry(state, telemetryData);


This implementation seems strange to me - telemetry returns an updated telemetryData object, but we are simply discarding and probably assume the passed in object will be mutated.

I really don't like relying on object reference here and mutation the object passed into the function. This is not how reduce is typically implemented - can we make telemetry return a new object with added information instead of mutating the existing one? This can lead to hard to catch bugs.

flash1293 · 2020-09-16T13:42:51Z

x-pack/plugins/ui_actions_enhanced/server/dynamic_action_enhancement.ts

+    id: 'dynamicActions',
+    telemetry: (state: SerializableState, telemetry: Record<string, any>) => {
+      (state as DynamicActionsState).events.forEach((event: SerializedEvent) => {
+        if (uiActionsEnhanced.getActionFactory(event.action.factoryId)) {


nit: Can be shortened to

uiActionsEnhanced.getActionFactory(event.action.factoryId)?.telemetry(event, telemetry);

that might assign void to telemetryData

flash1293 · 2020-09-16T13:43:39Z

x-pack/plugins/ui_actions_enhanced/public/dynamic_actions/dynamic_action_enhancement.ts

+): EnhancementRegistryDefinition => {
+  return {
+    id: 'dynamicActions',
+    telemetry: (state: SerializableState, telemetryData: Record<string, any>) => {


No strong opinion, just interested - why did you keep the telemetry API on the client even if it's not used?

so we can keep the same persistable state definition type on both sides

ppisljar · 2020-09-16T14:58:03Z

@elasticmachine merge upstream

…dableMigrations

ppisljar · 2020-09-17T04:15:55Z

@elasticmachine merge upstream

ppisljar · 2020-09-18T05:07:29Z

@elasticmachine merge upstream

kibanamachine · 2020-09-18T06:44:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 425f521

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
embeddable	95	+3	92
kibanaUtils	190	+1	189
uiActionsEnhanced	278	+2	276
total		+6

page load bundle size

id	value	diff	baseline
embeddable	435.1KB	+4.6KB	430.6KB
kibanaUtils	471.7KB	+1.3KB	470.4KB
uiActionsEnhanced	378.9KB	+561.0B	378.3KB
total		+6.4KB

distributable file count

id	value	diff	baseline
default	45924	+11	45913
oss	27700	+6	27694
total		+17

History

💔 Build #75066 failed c9737eb
💔 Build #74889 failed bed619a
💔 Build #74823 failed 6f2aee7
💔 Build #74816 failed 4817d12
💔 Build #74746 failed a1317a5

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

)

ppisljar added WIP Work in progress Feature:Embedding Embedding content via iFrame Team:AppArch Feature:Drilldowns Embeddable panel Drilldowns labels Aug 5, 2020

ppisljar requested a review from stacey-gammon August 5, 2020 11:26

ppisljar requested a review from a team as a code owner August 5, 2020 11:26

ppisljar force-pushed the embeddableMigrations branch 2 times, most recently from 412dfc9 to 28b78c7 Compare August 5, 2020 13:31

ppisljar requested a review from a team August 5, 2020 13:31

ppisljar added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 labels Aug 5, 2020

ppisljar force-pushed the embeddableMigrations branch from 28b78c7 to 9192a0a Compare August 5, 2020 13:33

Dosant reviewed Aug 5, 2020

View reviewed changes

lukeelmers reviewed Aug 5, 2020

View reviewed changes

src/plugins/embeddable/public/lib/embeddables/embeddable_factory.ts Outdated Show resolved Hide resolved

stacey-gammon reviewed Aug 13, 2020

View reviewed changes

ppisljar force-pushed the embeddableMigrations branch from 9192a0a to 3408f1d Compare August 20, 2020 12:02

ppisljar changed the title ~~Embeddable migrations and reference extraction/injection~~ Embeddable telemetry and reference extraction/injection Aug 25, 2020

ppisljar force-pushed the embeddableMigrations branch 2 times, most recently from 7f06f38 to 60cf0b1 Compare August 26, 2020 11:43

ppisljar added 7 commits September 7, 2020 03:20

adding telemetry, extraction, injection to embeddables

16ce3fb

dynamic actions

c363dd6

drilldowns

7609131

fixing after rebase

b4afe35

reverting drilldown fn implementations

0fec27d

ui actions enhanced

17db743

first

5a808db

Merge branch 'master' into embeddableMigrations

c2b75e3

Dosant approved these changes Sep 16, 2020

View reviewed changes

review feedback

43438c3

ppisljar force-pushed the embeddableMigrations branch from be0a8dd to 43438c3 Compare September 16, 2020 11:13

review feedback

a1317a5

flash1293 reviewed Sep 16, 2020

View reviewed changes

review feedback

4817d12

elasticmachine and others added 3 commits September 16, 2020 08:58

Merge branch 'master' into embeddableMigrations

6f2aee7

fixing eslint

c161491

Merge remote-tracking branch 'origin/embeddableMigrations' into embed…

bed619a

…dableMigrations

Merge branch 'master' into embeddableMigrations

c9737eb

Merge branch 'master' into embeddableMigrations

425f521

ppisljar merged commit e361035 into elastic:master Sep 18, 2020

ppisljar mentioned this pull request Sep 18, 2020

[7.x] Embeddable telemetry and reference extraction/injection (#74352) #77911

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Sep 18, 2020

Embeddable telemetry and reference extraction/injection (elastic#74352)

c5fec50

ppisljar added a commit that referenced this pull request Sep 19, 2020

Embeddable telemetry and reference extraction/injection (#74352) (#77911

84c7886

)

This was referenced Oct 19, 2020

Embeddable persistable state migration #80973

Closed

Dashboard drilldowns are not using saved object references #71409

Closed

This was referenced Nov 6, 2020

Dashboard panel references to use embeddable persistable state service #82830

Closed

Use saved object references for dashboard drilldowns #82602

Merged

rudolf mentioned this pull request Dec 3, 2020

saved object migrations should support embeddable migrations #84907

Closed

Conversation

ppisljar commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Aug 5, 2020

Uh oh!

Dosant left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stacey-gammon commented Aug 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stacey-gammon commented Aug 13, 2020

Uh oh!

ppisljar commented Sep 16, 2020

Uh oh!

Dosant left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ppisljar commented Sep 16, 2020

Uh oh!

ppisljar commented Sep 17, 2020

Uh oh!

ppisljar commented Sep 18, 2020

Uh oh!

kibanamachine commented Sep 18, 2020 • edited by spalger Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

page load bundle size

distributable file count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ppisljar commented Aug 5, 2020 •

edited

Loading

Dosant left a comment •

edited

Loading

Dosant left a comment •

edited

Loading

flash1293 Sep 16, 2020 •

edited

Loading

kibanamachine commented Sep 18, 2020 •

edited by spalger

Loading