-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Migrating vega_vis from plugin #15014
Conversation
|
name: 'vega', | ||
handler: (/*vis, appState, uiState, queryFilter*/) => { | ||
// return different data when handler is called so vega visualization can watch visData | ||
return Promise.resolve((new Date()).getTime()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Date.now()
instead of (new Date()).getTime()
since it's significantly faster.
<span>JSON</span> | ||
</button> | ||
<div class="vegaEditorHelp"> | ||
<a target="_blank" href="https://github.com/nyurik/kibana-vega-vis#vega-visualization-plugin-for-kibana">help</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we pull in the plugin into the core, we should also provide a link here to some documentation, that is either published on the docs or in the repository.
static setDebugValues(view, spec, vlspec) { | ||
if (window) { | ||
if (window.VEGA_DEBUG === undefined && console) { | ||
/*eslint-disable */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't disable eslint completely, but only the rule you need to have disabled.
TODOs:
|
Above are the new color schemes. I have created the new ** Update ** Per @thomasneirynck , using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice intro!
Overall, looking really good. The code is tidy too.
All my comments are from a fairly high level at this point:
- structure of the plugin:
- remove as much of the angular dependencies where possible (use base-visualization, reuse sidebar-editor)
- use the request-handler as intended
- removing feature support:
- I think the mapping support is not quite right for core Kibana (see comment for deetz)
- remove the HJSON
- remove the auto-apply (somewhat follows from reusing the side-bar editor)
|
||
// return the visType object, which kibana will use to display and configure new | ||
// Vis object of this type. | ||
return VisFactory.createAngularVisualization({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to see this rewritten as a base-visualization. So basically, .createBaseVisualization
. The requirements for angular are very minimal, basically plopping in some content in a node. Also, we are actively removing much of angular outside of Kibana.
So I'd remove the vis-directive and vis-controller, and just wrap it in a class VegaVisVizualization which implements the interface https://www.elastic.co/guide/en/kibana/master/development-visualization-factory.html#development-base-visualization-type.
@@ -0,0 +1 @@ | |||
export { VegaView } from './vega_view'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider removing this and just importing vega_view directly.
if (!this._$container) return; | ||
|
||
const view = new vega.View(vega.parse(this._specParams.spec), this._viewConfig); | ||
VegaView.setDebugValues(view, this._specParams.spec, this._specParams.vlspec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to keep while developing, but let's not forget to remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) this is not for debugging the plugin, this is for debugging the vega graph -- https://github.com/nyurik/kibana-vega-vis#debugging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per discussion with @thomasneirynck make VEGA_DEBUG only available while in the vis editor.
}); | ||
|
||
try { | ||
if (this._specParams.useMap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to see the support for mapping using Leaflet removed, at least in this first version.
For a couple of reasons:
- This is beyond core-vega and introduces a lot of custom code, taking this plugin much beyond a simple mashup of ES-data with Vega-markup. imho, that's a big scope, for a first version.
- Leaflet is one of the more complex 3rd party dependencies in Kibana. Upgrading from 0.7 to 1.0 wasn't a simple switch. We also don't know if we want to retain Leaflet in the long run. Kibana now hides all leaflet dependencies behind
KibanaMap
. This is somewhat perfunctory, but really helps us to prevent Leaflet's API bleeding throughout Kibana. If we want to support mapping in the Vega-plugin, it should at least be written to reuse theKibanaMap
. Vega-plugin could then introduce something like a custom KibanaLayer implementation, similar to how we have a ChoroplethLayer and a GeohashLayer. - end-user mapping support is very much in flux still for the stack. Given the uncertain roadmap, I'd prefer not too introduce new mapping visualizations at this time.
- there is no integration with the ElasticMapsService. I think all mapping support in Kibana needs to hook into the EMS service, and discover available data and services from the catalogue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are very good reasons. The problem is that maps captured the most attention at the presentations, and seems to be the most desired portion of the system. Of course we can say that for maps the user has to install the unofficial plugin until we port the map tech too.
Vega plugin integrates with Leaflet via the leaflet-vega. I do plan to also implement the openlayers-vega fairly soon, with the similar interface, making it easily swapable. Vega plugin itself uses bare minimum of Leaflet - simply instantiates it and attaches leaflet-vega plugin. BTW, that plugin supports 0.7 - 1.1
I will have to look how KibanaMap
is set up - if its possible to attach to the map object.
The Vega plugin uses tmsService.getUrl() and getTMSOptions(), but in some cases it is possible to use a blank leaflet - useful when you want to show a custom image like factory floor or a conference hall, without the base map. It should be possible for the users to specify which layer(s) they want via the vega spec - e.g. config: { mapLayer: ['baseA', 'extraB'] }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ +1 on looking into this for using KibanaMap and EMS.
I would still hold off on map support for a first version. Users will already have a nice new feature-set, we don't need to throw in everything at once.
But for later versions;
If we can integrate this tightly with EMS, that's certainly a great hook there! (baselayer switching, referencing vector layers). The config example you give makes sense to me. For that to work, we also need that EMS-landing page, that centrally hosted page where people can explore all the map service/data offerings.
As for KibanaMap, apart from hiding Leaflet, it has some sugar for setting a baselayer. setBaseLayer
. It's also set on on initialization time (https://github.com/thomasneirynck/kibana/blob/c56360b9089c974f32b1c917f72011ef1cc60ba0/src/core_plugins/tile_map/public/kibana_map.js#L543-L543). KibanaMap also provides the hooks to keep Kibana's uiState in sync (https://github.com/thomasneirynck/kibana/blob/c56360b9089c974f32b1c917f72011ef1cc60ba0/src/core_plugins/tile_map/public/kibana_map.js#L543-L543). This means if people pan/zoom the map, the state will be preserved in the URL and reloaded at the correct time.
options: { | ||
showIndexSelection: false | ||
}, | ||
isExperimental: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be changed to stage: 'lab'
|
||
import vegaVisEditorTemplate from './vega_vis_editor.template.html'; | ||
|
||
export function VegaEditorProvider($rootScope, $compile, $timeout, getAppState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho, I'd reuse the default sidebar editor. All we need is a big input box to hold our configuration.
The iring of the editor would be then similarly structured as the markdown-plugin: https://github.com/thomasneirynck/kibana/blob/7bcc7435e22dc2ce68929b1b418c037eaa9a4ec7/src/core_plugins/markdown_vis/public/markdown_vis.js#L28-L33
We will have the play
button. fwiw, I don't think it's a good idea to do an auto-apply here. Each time a user types it fires a change, spamming the server. As long as we don't have client-side validation if the JSON in the vega is any sensible, we shouldn't be handling a request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does spam the server, and I think we should optimize it, possibly by introducing some "cache response" parameter, or perhaps "disable auto-apply" button. Developing a new visualization without auto-apply is extremely painful. Vega is complex, and most coding in it is done by trial and error - you type a character, and see what changes. In fact, autoapply was so needed that I had to write a Wikipedia graph sandbox, and Vega editor itself implemented this as the default behavior. The editor has a "parse: auto/manual" button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I do agree that that is user friendly.
Alternatively, can we make the VegaEditor NOT an angular component, but just a plain javascript component that wires up the internals? It'd be nice not to introduce new angular-code and the editor's structure is really simple, imho not requiring the introduction of a UI-framework.
return { | ||
name: 'vega', | ||
handler: (/*vis, appState, uiState, queryFilter*/) => { | ||
// return different data when handler is called so vega visualization can watch visData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) clever!
This is probably an artifact of bringin in vega_view
and reusing that as-is. But this request-handler should actually run the actual query, basically what the vega_loader
is doing right now.
I know this is going to somewhat mess up how you implemented the loader right now, it's basically something you inject into vega.View
. So the vega_loader
should just return whatever came out of the request_handler
.
I'm not 100% familiar with Vega, so let me know how feasible this is.
In any case, keeping the data-fetch OUTSIDE of the request_handler is going to hurt is in the long run. The request_handler
is our funnel for visualizations to request data from the server (implied of course that 99% of requests end up hitting an Elasticsearch cluster, this is obviously not the case for mashing up data from 3rd party sources). It's important that ES-searchs are handled by requestHandlers because it is consistent with all our other visualizations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... this might be tricky, but TBH i'm not very happy with the current implementation either.
Vega spec has a data
section, which is a list of data sources, and each could be an ES request or a simple URL. Vega calls the loader once for each data source, and expects a promise of the data. I would like to use Vega observable pattern, possibly implementing it myself - which would remove the need to regenerate the graph on auto refresh.
One option might be to dynamically change the Vega spec with a pre-parser - inspect data section, and replace "url" with the hardcoded values right before passing it to Vega parser. We can even use "query": {...}
instead of "url": {...}
. On auto-refresh, the data could be auto-re-injected into the Vega view. Not sure how stable this approach would be...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not familiar enough with Vega to speak to the pre-parser.
Could we inject a callback in the vega_loader that returns the result from the requesthandler (or even the actual data-object)? We know where the ES-search query is on the Vega-object. The data-fetch that is now in vega_loader could be transfered to requestHandler. The instantiation of the vega.View happens after the requestHandler-call is made I believe, so the vega_loader can use that data from the requestHandler as-is.
tooltip="Format graph code as HJSON" | ||
class="kuiButton kuiButton--basic" | ||
> | ||
<span>HJSON</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove the HJSON support. I think we should keep this as small and functional as possible. The problem with bringing in yet another flavor of language, is similar to what we're experiencing with our flavors of markdown-support. Just more 3rd party dependencies and all the slight mismatches and long-term issues that arise with that.
I think the benefits over the long run (a leaner Kibana), will outweigh the benefits in the short run (slightly better user experience). This is a first introduction of this plugin, I'd start with the smallest thing that works and build up from there. It's better to be able to dial in features, than have to prune features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove HJSON support, it would be impossible to have comments in JSON - which we heavily use in all of the examples including the first one. JSON is painful for humans to write in... and YAML has nasty automagical things like references and yes|no
is actually a true|false
. Do we want to strip all the comment capabilities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const createGraph = async () => { | ||
this.messages = []; | ||
|
||
// FIXME!! need to debounce editor changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
see earlier comment on removing the custom vega_editor. It'd skirt the issue altogether, by just reusing the sidebar editor. We'd end up with a leaner plugin (code footprint wise), with relatively minor impact.
@@ -0,0 +1,41 @@ | |||
import { VisTypesRegistryProvider } from 'ui/registry/vis_types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to vega_vis_type.js
, eurgh, without the "." at least :)
be56511
to
42277f5
Compare
config/kibana.yml
Outdated
@@ -102,3 +102,10 @@ | |||
# The default locale. This locale can be used in certain circumstances to substitute any missing | |||
# translations. | |||
#i18n.defaultLocale: "en" | |||
|
|||
|
|||
optimize: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you put this into kibana.dev.yml
, it will load for your but not show up here, which is what we want. start kibana with node scripts/kibana --dev
@@ -17,6 +18,7 @@ export function injectVars(server) { | |||
kbnDefaultAppId: serverConfig.get('kibana.defaultAppId'), | |||
regionmapsConfig: regionmapsConfig, | |||
mapConfig: mapConfig, | |||
vegaConfig: vegaConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be inside the injectVars
configuration for your plugin and not the kibana
general one.
@timroes I updated it to use plugin's injectVars, but it seems they are not being called by Kibana. I can only see timelion using this feature, and they are doing some weird init path I think, because it functions as a standalone app as well. |
2bfd0f3
to
2db93fa
Compare
* imprescise libs in package.json * removed href=# on buttons * added rel="noopener noreferrer" for target=_blank * default editor size = medium * reverted babel vega exception
Thanks @timroes, fixed both issues. Please review https://github.com/nyurik/kibana/blob/b7bcf4a72ffb549c31e7f0cb3804eed970a5aee6/src/core_plugins/vega/public/default.spec.hjson -- see if it makes sense for the new user, and/or grammar issues. Thx! |
I'm fine with the default spec as is. I've just one worry about it: Querying I've also left one minor code comment/question, but otherwise I'm fine to go with this for 6.2. |
// properly handle multiple destroy() calls by converting this._destroyHandlers | ||
// from an array into a promise, while handlers are being disposed | ||
if (this._destroyHandlers) { | ||
if (this._destroyHandlers.then) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I wouldn't use the same variable, to store the ongoing process of destroying and the previously registered destroy listeners. I think it makes this less readable, than something like follows:
destroy() {
if (this._destroyHandlers) {
if (!this._ongoingDestroy) {
// If no destroy is yet running, execute all handlers and wait for all of them to resolve.
this._ongoingDestroy = Promise.all(this._destroyHandlers.map(v => v())
.then(() => {
this._destroyHandlers = null;
this._ongoingDestroy = null;
});
}
return this._ongoingDestroy;
}
}
_addDestroyHandler(handler) {
if (this._ongoingDestroy) {
handler();
} else {
this._destroyHandlers.push(handler);
}
}
I btw still think that code has an issue. In both ways, when the destroy is already ongoing and we call _addDestroyHandler
, we execute the handler immediately, but we don't actual wait for it to finish, since the Promise.all
will always resolve once the initial destroy handlers are finished, ignoring those later once.
If this can be an issue, we should think about a solution to properly resolve the destroy()
promise once everything (also the newly added once) are executed.
@timroes thx, I changed disposing a bit to use two different vars. (btw, I think you had a bug there - once dispose is done, additions will crash because there is no more array with |
Large PR to migrate Vega plugin into the core. https://github.com/nyurik/kibana-vega-vis The code underwent a large number of changes and reviews discussed in the PR thread: elastic#15014
Large PR to migrate Vega plugin into the core. https://github.com/nyurik/kibana-vega-vis The code underwent a large number of changes and reviews discussed in the PR thread: #15014
} | ||
// If no destroy is yet running, execute all handlers and wait for all of them to resolve. | ||
// Once done, keep the resolved promise, and get rid of any values returned from handlers. | ||
this._ongoingDestroy = Promise.all(this._destroyHandlers.map(v => v())).then(() => 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the .then(() => 0)
intended to do? It will just reset the resolve value to 0
. Is that really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timroes I think I was over-thinking it :) Basically if the handler returns an object, all these objects are stored in an array inside the promise that is part of the View. Setting it to 0 expedites garbage collection, but in reality i don't think it matters. I will remove it in the followup patch.
(ready for review & merge)
Integration of the Vega plugin into core as an experimental feature.
Testing:
Blockers:
Nice-to-haves:
@timroes TODOs:
kibana.yml
)@thomasneirynck TODOs:
Ticket: #14911