[Canvas] Generic embeddable function by cqliu1 · Pull Request #104499 · elastic/kibana

cqliu1 · 2021-07-06T16:30:33Z

Summary

Related to #81812.
Related to #101633.

This creates a single generic embeddable function to handle all by-reference embeddables in Canvas.

It accepts only two arguments:

config: a base64 encoded string of the embeddable input object
type: the embeddable type

Example expression:

embeddable config="eyJ2aWV3TW9kZSI6ImVkaXQiLCJ0aW1lUmFuZ2UiOnsiZnJvbSI6Im5vdy0xNWQiLCJ0byI6Im5vdyJ9LCJkaXNhYmxlVHJpZ2dlcnMiOnRydWUsInJlbmRlck1vZGUiOiJub0ludGVyYWN0aXZpdHkiLCJpZCI6IjJjOWMxZjYwLTE5MDktMTFlOS05MTliLWZmZTU5NDlhMThkMiIsImZpbHRlcnMiOltdLCJzYXZlZE9iamVjdElkIjoiMmM5YzFmNjAtMTkwOS0xMWU5LTkxOWItZmZlNTk0OWExOGQyIiwibWFwQ2VudGVyIjp7ImxhdCI6NTcuNTgyNDYsImxvbiI6LTExMi43NjM2Nywiem9vbSI6Mi4xMX0sIm1hcEJ1ZmZlciI6eyJtaW5Mb24iOi0xODAsIm1pbkxhdCI6MCwibWF4TG9uIjotNDUsIm1heExhdCI6NzkuMTcxMzN9LCJpc0xheWVyVE9DT3BlbiI6ZmFsc2UsIm9wZW5UT0NEZXRhaWxzIjpbXSwiaGlkZGVuTGF5ZXJzIjpbIjdhbWVxIl19" 
  type="map"
| render

Changes:

adds lab config for by-value embeddables and updates telemetry schema
adds embeddable function that serves as a generic version of savedMap, savedVisualizations, etc
adds support for savedSearch embeddable type via generic embeddable function
updates expression with changes to base embeddable input like time range, title, etc

Known Issues:

This function doesn't support palettes which are supported by Lens. Do we want to support palettes with this generic function if not all embeddables support palettes?
Input controls embeddables don't work in Canvas because filters can't be applied by embeddables currently

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)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
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 · 2021-07-30T19:58:57Z

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

cqliu1 · 2021-08-05T14:37:26Z

@elasticmachine merge upstream

poffdeluxe · 2021-09-28T19:00:36Z

x-pack/plugins/canvas/public/components/embeddable_flyout/flyout.tsx

      };
-      if (allowedEmbeddables[type]) {
+
+      if (isByValueEnabled) {


this by value flag is used even for by reference embeddables? Maybe a little confusing but if I'm understanding this all correctly, all the new embeddable behavior will be controlled by this flag, right?

Yeah, my intention is to favor the new generic embeddable fn, which handles both by-value and by-reference embeddables, as the default expression for an embeddable added from the library, so we can eventually phase out the type specific embeddable functions.

We're using the new embeddable expression introduced in this PR if the by-value lab is enabled. So all embeddables added with this feature enabled should have an expression like

embeddable config="eyJ2aWV3TW9kZSI6ImVkaXQiLCJ0aW1lUmFuZ2UiOnsiZnJvbSI6Im5vdy0xNWQiLCJ0byI6Im5vdyJ9LCJkaXNhYmxlVHJpZ2dlcnMiOnRydWUsInJlbmRlck1vZGUiOiJub0ludGVyYWN0aXZpdHkiLCJpZCI6IjJjOWMxZjYwLTE5MDktMTFlOS05MTliLWZmZTU5NDlhMThkMiIsImZpbHRlcnMiOltdLCJzYXZlZE9iamVjdElkIjoiMmM5YzFmNjAtMTkwOS0xMWU5LTkxOWItZmZlNTk0OWExOGQyIiwibWFwQ2VudGVyIjp7ImxhdCI6NTcuNTgyNDYsImxvbiI6LTExMi43NjM2Nywiem9vbSI6Mi4xMX0sIm1hcEJ1ZmZlciI6eyJtaW5Mb24iOi0xODAsIm1pbkxhdCI6MCwibWF4TG9uIjotNDUsIm1heExhdCI6NzkuMTcxMzN9LCJpc0xheWVyVE9DT3BlbiI6ZmFsc2UsIm9wZW5UT0NEZXRhaWxzIjpbXSwiaGlkZGVuTGF5ZXJzIjpbIjdhbWVxIl19" type="map" | render

If the lab is disabled, we use the original logic which builds an expression based on embeddable type from our mapping of allowed embeddables.

Gotcha. I wonder if it makes sense to rename the lab flag to something that isn't necessarily by-value specific. When I'm reading this code and I see a flag checking for by-value to be enabled, my brain makes the assumption that the code I'm going to be reading next has to do with by-value embeddables which isn't necessarily the case. If it's going to be a pain to re-name the flag, maybe a comment explaining that by-reference embeddables are still handled in the block -- it's just going to use the new function

poffdeluxe

Approving to unblock! I still think we'll want to figure out some clarity around the lab's flag but otherwise this looks good to me.

I also verified that flipping the flag on/off won't break existing workpads (or in the case of flipping the flag off, a workpad using the new generic function doesn't break)

crob611

Looks great 👍

Left a few little nits that should be cleaned up

crob611 · 2021-09-29T15:16:30Z

x-pack/plugins/canvas/canvas_plugin_src/functions/external/embeddable.ts

+        },
+      ];
+
+      return {


If this is a "by value" embeddable, then the state should be passed on to the EmbeddablePersistableStateService for extract.

I'll tackle this in a separate PR to address extracting/injecting references for by-value embeddables.

crob611 · 2021-09-29T15:20:49Z

x-pack/plugins/canvas/canvas_plugin_src/functions/external/embeddable.ts

+
+    inject(state, references) {
+      const reference = references.find((ref) => ref.name === 'embeddable.id');
+      if (reference) {


And here should also be passed on to EmbeddablePersistableStateService for inject

crob611 · 2021-09-29T15:37:06Z

...plugins/canvas/canvas_plugin_src/renderers/embeddable/input_type_to_expression/embeddable.ts

+export function toExpression(input: EmbeddableInput, embeddableType: string): string {
+  const expressionParts = [] as string[];
+
+  expressionParts.push('embeddable');


since there's nothing conditional here, can you just build this as one big string instead of doing the parts+join.

return `embeddable config="${encode(input)}" type="${embeddableType}"`

crob611 · 2021-09-29T15:41:44Z

x-pack/plugins/canvas/public/components/embeddable_flyout/flyout.component.tsx


 export const AddEmbeddableFlyout: FC<Props> = ({ onSelect, availableEmbeddables, onClose }) => {
  const embeddablesService = useEmbeddablesService();
+  const labsService = useLabsService();


I think it makes more sense to pull the labsService up to the flyout.tsx and not in this component. It's only purpose being in this file is to pass to that callback, so why not have that callback know about it instead and then this component has less to deal with.

I still need the lab service in flyout.component to check if by value is enabled when filtering out embeddable factories.

https://github.com/elastic/kibana/pull/104499/files#diff-9aab885adf620bf919f3fad7f54143c060854b29a0a780272e45701aab457821R64

line 63-64

const availableSavedObjects = Array.from(embeddableFactories) .filter((factory) => isByValueEnabled || availableEmbeddables.includes(factory.type))

If by-value is enabled, then this conditional always evaluates to true and doesn't end up filtering out any factories, but if it is disabled, we want to restore the original functionality and filter out for only the allowed embeddable factories.

I can move labsService up to flyout.tsx, but I'd end up using it in both files.

I moved labsService up to flyout.tsx and pass isByValueEnabled as a prop into flyout.component in 5163ffd.

…able-fn

kibanamachine · 2021-10-01T07:46:25Z

💚 Build Succeeded

History

💛 Build #156980 was flaky db4a95a
💔 Build #156859 failed 2b3fe3d
💔 Build #156853 failed 5163ffd
💔 Build #156597 failed fd80ede
💚 Build #156204 succeeded 81a15dc

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

* Created generic embeddable function Fixed telemetry Updates expression on input change Fixed ts errors Store embeddable input to expression Added lib functions Added comments Fixed type errors Fixed ts errors Clean up Removed extraneous import Added context type to embeddable function def Fix import Update encode/decode fns Moved embeddable data url lib file Added embeddable test Updated comment * Fix reference extract/inject in embeddable fn * Simplify embeddable toExpression * Moved labsService to flyout.tsx * Added comment

* [Canvas] Generic embeddable function (#104499) * Created generic embeddable function Fixed telemetry Updates expression on input change Fixed ts errors Store embeddable input to expression Added lib functions Added comments Fixed type errors Fixed ts errors Clean up Removed extraneous import Added context type to embeddable function def Fix import Update encode/decode fns Moved embeddable data url lib file Added embeddable test Updated comment * Fix reference extract/inject in embeddable fn * Simplify embeddable toExpression * Moved labsService to flyout.tsx * Added comment * [Canvas] Adds Save and Return Workflow (#111411) * [Canvas] Adds editor menu to Canvas (#113194) * Merge existing embeddable input with incoming embeddable input (#116026) * [Canvas] Extract and inject references for by-value embeddables (#115124) * Extract/inject references for by-value embeddables in embeddable function Fixed server interpreter setup Register external functions in canvas_plugin_src plugin def * Fixed ref name in embeddable.inject * Fixed ts errors * Fix missing type error Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Created generic embeddable function Fixed telemetry Updates expression on input change Fixed ts errors Store embeddable input to expression Added lib functions Added comments Fixed type errors Fixed ts errors Clean up Removed extraneous import Added context type to embeddable function def Fix import Update encode/decode fns Moved embeddable data url lib file Added embeddable test Updated comment * Fix reference extract/inject in embeddable fn * Simplify embeddable toExpression * Moved labsService to flyout.tsx * Added comment

cqliu1 changed the title ~~[Canvas] Generic embeddable function~~ [Canvas] [POC] Generic embeddable function Jul 6, 2021

cqliu1 force-pushed the canvas/generic-embeddable-fn branch 2 times, most recently from f588bc7 to 2150ef8 Compare July 30, 2021 18:00

cqliu1 changed the title ~~[Canvas] [POC] Generic embeddable function~~ [Canvas] Generic embeddable function Jul 30, 2021

cqliu1 marked this pull request as ready for review July 30, 2021 19:58

cqliu1 requested a review from a team as a code owner July 30, 2021 19:58

cqliu1 added the review label Jul 30, 2021

cqliu1 requested a review from a team as a code owner August 3, 2021 14:33

cqliu1 force-pushed the canvas/generic-embeddable-fn branch from f84c1f3 to 910d8eb Compare August 3, 2021 14:35

cqliu1 requested a review from a team as a code owner August 3, 2021 15:43

cqliu1 changed the base branch from master to canvas/by-value-embeddables August 3, 2021 17:02

cqliu1 removed v7.15.0 v8.0.0 labels Aug 3, 2021

cqliu1 closed this Aug 3, 2021

cqliu1 force-pushed the canvas/generic-embeddable-fn branch from d3d9d1e to 14f66b5 Compare August 3, 2021 17:10

cqliu1 reopened this Aug 3, 2021

cqliu1 force-pushed the canvas/generic-embeddable-fn branch 3 times, most recently from 1093d6d to 199a5f4 Compare August 3, 2021 19:08

mshustov approved these changes Aug 4, 2021

View reviewed changes

poffdeluxe reviewed Sep 28, 2021

View reviewed changes

Fix reference extract/inject in embeddable fn

81a15dc

poffdeluxe approved these changes Sep 28, 2021

View reviewed changes

crob611 approved these changes Sep 29, 2021

View reviewed changes

cqliu1 added 4 commits September 29, 2021 14:20

Simplify embeddable toExpression

fd80ede

Moved labsService to flyout.tsx

5163ffd

Added comment

2b3fe3d

Merge branch 'canvas/by-value-embeddables' into canvas/generic-embedd…

db4a95a

…able-fn

cqliu1 merged commit 3650aea into elastic:canvas/by-value-embeddables Oct 1, 2021

cqliu1 deleted the canvas/generic-embeddable-fn branch October 1, 2021 17:55

cqliu1 mentioned this pull request Oct 4, 2021

[Canvas] By-Value Embeddables #113827

Merged

11 tasks

Conversation

cqliu1 commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jul 30, 2021

Uh oh!

cqliu1 commented Aug 5, 2021

Uh oh!

poffdeluxe Sep 28, 2021 • 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!

cqliu1 Sep 28, 2021 • 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!

poffdeluxe Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

poffdeluxe left a comment

Choose a reason for hiding this comment

Uh oh!

crob611 left a comment

Choose a reason for hiding this comment

Uh oh!

crob611 Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

cqliu1 Sep 29, 2021 • 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!

crob611 Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

crob611 Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

crob611 Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

cqliu1 Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

cqliu1 Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 1, 2021

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cqliu1 commented Jul 6, 2021 •

edited

Loading

poffdeluxe Sep 28, 2021 •

edited

Loading

cqliu1 Sep 28, 2021 •

edited

Loading

cqliu1 Sep 29, 2021 •

edited

Loading