Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lens] Implement deep linking and embedding #84416

Merged
merged 16 commits into from
Jan 12, 2021

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Nov 26, 2020

This PR explores ways to implement deep linking and embedding in Lens.

It includes a small example application which embedds a config inlined in the consuming app with two buttons to randomly change the color of the chart and another one to take the current configuration and make it editable in Lens.

  • Every state can easily made editable by the user
  • Everything possible with Lens can be accessed this way
  • Works with saved objects and inline configs

Lens embedding

To test:

  • run yarn start --run-examples
  • Install logs sample data and make sure the logs index pattern is the default one
  • Go to /app/embedded_lens_example

Consumer side

This PR adds two things to the lens start contract:

  • EmbeddableComponent: React.ComponentType<LensProps> A component usable in your application which takes either a persisted state of a Lens visualization (technically it's the embeddable input, but there's very little difference) or a saved object id of a Lens visualization. The component loads the Lens bundle and uses the Lens embeddable implementation to render the visualization very similar to how it's working in a dashboard. If the input changes, the visualization is rendered again.
  • navigateToPrefilledEditor: (input: LensEmbeddableInput) => void Takes a Lens embeddable input (again persisted state or saved object id) and navigates to Lens with this state preloaded

LensProps['attributes'] is properly typed for the individual state shapes, so it's possible to statically check consumers of this API.

x-pack/examples/embedded_lens_example/public/app.tsx contains the example plugin using these APIs

Internals

x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable_component.tsx and x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable_component_loader.tsx provide a very thin wrapper around the embeddable acting as a lazy-loading mini embeddable container, passing through the input via props.

navigateToPrefilledEditor simply uses EmbeddableStateTransfer to kick off the navigation like the edit action on dashboard does.

Normally the attributes of the Lens embeddable input are typed very loose, this PR makes them specific for all existing visualizations:

  attributes:
    | LensAttributes<'lnsXY', XYState>
    | LensAttributes<'lnsPie', PieVisualizationState>
    | LensAttributes<'lnsDatatable', DatatableVisualizationState>
    | LensAttributes<'lnsMetric', MetricState>;

Once we make it possible to register visualizations from outside of the Lens plugin, we can use an approach similar to UI actions to extend a central interface containing all existing pairs to keep this level of type-safety.

Problems

  • Exposes the internal types - if we treat changes here as breaking change, we would be severly limited in adding/changing stuff in the state. For the beginning pne option would be to explicitly mention this in the docs and mark this API as experimental so consumers are aware the integration can break easily. As it's a single CI build I don't think we will have problems with Kibana-internal integrations this way. Another option would be to introduce a compatibility layer for this which maintains BWC for a major version if that's necessary, but I would wait with that until it actually becomes a problem
  • Completely re-initalizes the state on each change - the way the code to turn a document into an expression is set up right now, it does the init routine of the datasource on every change. This can be improved by keeping the initialized state around in the embeddable so we don't have to initialize from scratch on every small change.
  • Right now the state transfer moving the state into the Lens app requires an "originatingAppId" - in this PR it's set to empty string which doesn't work correctly everywhere. This can be solved cleanly by making originatingAppId optional

Extensions

  • It would be cool to have a bunch of helper functions to construct the Lens state to make it simpler to use

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

"requiredPlugins": [
"lens",
"data",
"developerExamples"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @flash1293! For this example plugin, can we add a dependency on features to illustrate that you'll need to grant access to the lens saved object type in order to properly embed when security is enabled?

I'm happy to help draft this change if you'd like help with it

Copy link
Contributor Author

@flash1293 flash1293 Dec 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legrego Happy to include this - but I'm not sure how it should look like.

This example is not working with saved objects directly, but it does link into the Lens editor. So I guess the right integration would be to check whether there there are at least visualize read permissions via ui capabilities and otherwise disable the button, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is not working with saved objects directly, but it does link into the Lens editor. So I guess the right integration would be to check whether there there are at least visualize read permissions via ui capabilities and otherwise disable the button, right?

Ah, so this doesn't allow you to view existing visualizations, or save new ones? If that's the case, then you shouldn't need any capabilities checks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, I will introduce a third method on the contract which can be used by consumers to determine whether it's possible to link to Lens or not.

@flash1293
Copy link
Contributor Author

Jenkins, test this

@flash1293 flash1293 marked this pull request as ready for review December 23, 2020 13:57
@flash1293 flash1293 requested a review from a team December 23, 2020 13:57
@flash1293 flash1293 added Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0 labels Dec 23, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@flash1293 flash1293 added the release_note:skip Skip the PR/issue when compiling release notes label Dec 23, 2020
@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to do a second pass on this, but left some questions for you.

to: 'now',
},
attributes: getLensAttributes(
props.core.uiSettings.get('defaultIndex'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I ran into this by coincidence, but the @timestamp field isn't available on the ecommerce data, which causes the example to crash. Maybe the example should be tied to a specific index like logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I fixed this by reading the index pattern and using the default time field

<EuiButton
isDisabled={!props.plugins.lens.canUseEditor()}
onClick={() => {
props.plugins.lens.navigateToPrefilledEditor({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was testing the behavior here and I think we do need to specify the originatingApp to get reasonable behavior here. I tested Lens by value vs what happens in the Lens pre-filled editor, and the breadcrumb navigation seem wrong here:

Lens by value:

Screen Shot 2021-01-05 at 7 07 34 PM

This PR:

Screen Shot 2021-01-05 at 7 09 32 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't set a title in the example app, fixed this

valueInput: input,
},
});
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this pre-filled editor logic is not quite right. First of all the breadcrumbs are not set. Second of all: are we expecting that users will return to the app they came from, or that the saved Lens visualization will get added to a dashboard? Right now it suggests adding to a dashboard.

Copy link
Contributor Author

@flash1293 flash1293 Jan 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was intended. It's possible to have a channel back to the app (using the originating app parameter), but that's much more complex to implement correctly because the app has to somehow persist the current state, then restore it.

IMHO this is a separate feature we can work on separately once it becomes relevant - I think the flow "solution app -> Lens -> dashboard" is what makes sense here (because solution apps don't want to provide a way to persist/load custom Lens visualizations).

This is similar to how the metrics explorer integration with TSVB works right now.

EuiTitle,
} from '@elastic/eui';
import { CoreStart } from 'kibana/public';
import { TypedLensByValueInput } from '../../../plugins/lens/public';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we expecting that other plugins will start depending on Lens in the plugin system?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly - as they are creating Lens-specific configuration, this seems like a good way to represent it on the type level

name: 'indexpattern-datasource-layer-layer1',
type: 'index-pattern',
},
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried about the references parameter of our saved object. Seems easy for an app to provide the wrong information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about a builder function of some sort abstracting away these things, but didn't want to overdo it for the first experimental release.

Do you have something simple in mind we could add here? IMHO we can wait for consumers to see in which direction we should drive this API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also expecting some kind of builder, but I agree that we should iterate on it. For now I'm assuming that users will copy+paste the JSON from a saved object.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 463 464 +1

Async chunks

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

id before after diff
lens 1.0MB 1.0MB +1.9KB

Page load bundle

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

id before after diff
lens 54.3KB 54.0KB -257.0B

History

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

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I think you're right that we can iterate on this, but this seems like a useful first step. Tested and LGTM.

name: 'indexpattern-datasource-layer-layer1',
type: 'index-pattern',
},
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also expecting some kind of builder, but I agree that we should iterate on it. For now I'm assuming that users will copy+paste the JSON from a saved object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants