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

Migrating vega_vis from plugin #15014

Merged
merged 69 commits into from Jan 13, 2018
Merged

Migrating vega_vis from plugin #15014

merged 69 commits into from Jan 13, 2018

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Nov 17, 2017

(ready for review & merge)

Integration of the Vega plugin into core as an experimental feature.

Testing:

  • test/functional/apps/visualize/_vega_chart.js - creates a new Vega vis
    • Test that there is no spy toggle button (this should change once spy panel is created)
    • Test that the new Vega viz has some initial graph
    • Test that the new Vega viz has initialized HTML container elements (vega and controls)
  • src/core_plugins/vega/public/tests/vega_visualization.js - creates just the viz portion
    • create a 512x512 VegaLite vis with hardcoded values, and validate against stored image
    • resize to 256x256, and compare again
    • create a 512x512 Vega vis with hardcoded values, and validate against stored image

Blockers:

  • filters are not dynamic - adding "status:200" in dashboard or searchbar are not refreshed automatically

Nice-to-haves:

  • Unit tests!
  • vertical editor/view splitter not moving well
  • Ctrl+F not working in editor
  • Keep the search field (at the top), but do not show index picker for new visualizations
  • (upstream) In some cases with extra control inputs, shows scrollbars (fixed upstream in jQuery, expected in v3.3)

@timroes TODOs:

  • Create a flag for disabling access to external data (inside kibana.yml)
  • Change the default spec to include an ES example, with nice documentation on the parameters
  • We should use our visualization default color palette if not overwritten: Replace qualitative color palette in visualizations #13327
  • disable external URLs by default

@thomasneirynck TODOs:

  • disable maps for 6.2, or migrate to ElasticMapsService (if there is enough time)
  • remove as much of the angular dependencies where possible
    • use base-visualization instead of Angular
    • reuse sidebar-editor
  • use the request-handler as intended (data injection instead of load-on-request)
  • (?) hjson

Ticket: #14911

@nyurik nyurik added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 WIP Work in progress labels Nov 17, 2017
@nyurik
Copy link
Contributor Author

nyurik commented Nov 17, 2017

While technically we don't use canvas module, Vega still requires it. Vega uses travis config to add several native libs. Can we add those to Jenkins?
P.S. I have asked Vega team to remove canvas dependency from the core vega lib.

The validation error is due to missing license vega/vega-datasets#22

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());
Copy link
Contributor

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>
Copy link
Contributor

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 */
Copy link
Contributor

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.

@timroes
Copy link
Contributor

timroes commented Nov 29, 2017

TODOs:

  • Create a flag for disabling access to external data (inside kibana.yml)
  • Change the default spec to include an ES example, with nice documentation on the parameters
  • We should use our visualization default color palette if not overwritten: Replace qualitative color palette in visualizations #13327

@nyurik
Copy link
Contributor Author

nyurik commented Dec 3, 2017

image
image
image
image
image

@nyurik
Copy link
Contributor Author

nyurik commented Dec 3, 2017

Above are the new color schemes. I have created the new elastic scheme for ranges (e.g. if graph shows 5 categories of items, it would use first 5 colors of the "elastic" palette by default, unless a different color scheme is specified). The question though is - should we change the regular default for lines/bars/...? Vega uses #4682b4 or steelblue by default.

** Update ** Per @thomasneirynck , using #00A69B

Copy link
Contributor

@thomasneirynck thomasneirynck left a 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({
Copy link
Contributor

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';
Copy link
Contributor

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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) {
Copy link
Contributor

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 the KibanaMap. 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.

Copy link
Contributor Author

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'] }

Copy link
Contributor

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,
Copy link
Contributor

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) {
Copy link
Contributor

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.

Copy link
Contributor Author

@nyurik nyurik Dec 5, 2017

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.

Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

@nyurik nyurik Dec 5, 2017

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...

Copy link
Contributor

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>
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be OK with stripping the comment capabilities.

@timroes @ppisljar what do you think?

const createGraph = async () => {
this.messages = [];

// FIXME!! need to debounce editor changes
Copy link
Contributor

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';
Copy link
Contributor

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 :)

@@ -102,3 +102,10 @@
# The default locale. This locale can be used in certain circumstances to substitute any missing
# translations.
#i18n.defaultLocale: "en"


optimize:
Copy link
Contributor

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,
Copy link
Contributor

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 timroes self-requested a review December 18, 2017 14:09
@nyurik
Copy link
Contributor Author

nyurik commented Dec 18, 2017

@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.
nyurik@31a17a5#diff-de38685f34de190d8ecb9e5c3bb06c89

@nyurik nyurik force-pushed the master branch 4 times, most recently from 2bfd0f3 to 2db93fa Compare December 19, 2017 20:48
@nyurik
Copy link
Contributor Author

nyurik commented Jan 13, 2018

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!

@timroes
Copy link
Contributor

timroes commented Jan 13, 2018

I'm fine with the default spec as is. I've just one worry about it: Querying _all can be very very expensive on a large cluster where people store lots of data. I think you might create frustration with those users, when they create a new vega visualization (especially once you would be able to enable auto apply by default). I think there should be a frontend setting (in the advanced config, not the yml) visualization:vega:useDefaultSpec, that is by default on true, but could be disabled to not include any default spec. That way users with large clusters could easily turn of using the default spec. I don't mind if this would be implemented in a separate pull request after 6.2, since this will not really break anything, maybe just cause users with large clusters not to use vega vis.

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) {
Copy link
Contributor

@timroes timroes Jan 13, 2018

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.

@nyurik
Copy link
Contributor Author

nyurik commented Jan 13, 2018

@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 push). And in any case, not waiting for the new dispose requests is the same as if the request was added after the object was fully disposed, so that should be ok.
I will merge it and backport it to 6.x tonight. Thanks for the thorough review!

@nyurik nyurik merged commit 1b70e74 into elastic:master Jan 13, 2018
nyurik added a commit to nyurik/kibana that referenced this pull request Jan 13, 2018
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
nyurik added a commit that referenced this pull request Jan 13, 2018
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);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.2.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants