Upgrade kibana to leaflet 1.x #12367

nreese · 2017-06-15T17:53:04Z

To use PR, uninstall old leaflet modules and install new modules with npm install

Uses leaflet-responsive-popup plugin to fix Tile Map tooltips near edges of map are cut off

Unable to upgrade to leaflet 1.1.0 because 1.1.0 breaks most leaflet plugins, like leaflet.draw - TypeError: can't define property "segmentsIntersect": Object is not extensible

backported to 6.0 #13669
backported to 6.x #13688

thomasneirynck · 2017-08-02T19:19:25Z

Closes #10276.

thomasneirynck · 2017-08-02T20:10:16Z

jenkins, test this

thomasneirynck

This is a real nice update. The map is decidedly more responsive. It will also set us up to work with the latest-greatest Leaflet features.

Two things jump out:

we'll need a new way to regression test the map. Currently, tests would verify that the SVG-content on the map was correct. Now, with Leaflet using html5-canvas, there is no access to such an easily queryable content in the DOM. Testing the correct data on the map has proven to be a good way to catch regressions, and we should retain a mechanism that is sufficiently reliable. I would consider experimenting with screenshot-comparisons for this https://github.com/mapbox/pixelmatch. Basically, test if the leaflet-canvas looks the way it should. But open to all other approaches.
The upgrade heatmap layer is unstable.
- It has this visual artifact where the layer doesn't zoom along smoothly with the map, but lags and then snaps. I can reproduce this on FF in Linux and chrome/ff/IE/edge on Windows (unsure about Mac).
- It also continues to display the following issue Failed to execute 'getImageData' on 'CanvasRenderingContext2D' on tilemap #13293, something we've had since forever. I feel like it's worse now (but that's just a gut-feeling, no specific steps to reproduce).

It may be useful to experiment with other implementations of the heatmap layer, that may have a more reliable implementation (?) (http://leafletjs.com/plugins.html#heatmaps). This may also benefit the simplification of the heatlayer options: #11759.

thomasneirynck

I tested this and I think it is a genuine nice addition. Particularly the heatmap simplification is really nice. The heuristics work well imo. we can always tinker more too.

I've been looking at it with some larger datasets. (300000+ records) and remains responsive. we can try a couple more ('real' ones) in the weeks to come/beta period.

Small thing: I ran into this, but for the love of me don't know how exactly (probably not related to this pr). failing at https://github.com/thomasneirynck/kibana/blob/23d903746383c2fc3d55c0cf1075a686aa6cb0a9/src/core_plugins/tile_map/public/maps_visualization.js#L305-L305

thomasneirynck · 2017-08-08T04:01:22Z

src/core_plugins/tile_map/public/editors/tile_map.html

@@ -13,69 +13,6 @@
 <div ng-if="vis.params.mapType === 'Heatmap'" class="form-group">
  <div>
    <label>
-      Radius


thomasneirynck · 2017-08-08T04:08:21Z

src/core_plugins/tile_map/public/geohash_layer.js

-          maxZoom: parseFloat(this._geohashOptions.heatmap.heatMaxZoom),
+          radius: radius,
+          blur: radius * 0.75,
+          maxZoom: this._kibanaMap.getZoomLevel(),


I think we should not use the zoom-level of the current map (because that may change for a given precision). Instead, the lowest zoom level that correspond to the current ES-precision on the map. I think that would yield more consistent results. It would make the construction for new heatmap-marker layer consistent for every ES-precision-result . By using the current zoom level, depending on where you are in the zoom, this layer could get instantiated with any of the 2-3 zoom levels that correspond to a given ES-precision.

thomasneirynck · 2017-08-08T04:08:25Z

src/core_plugins/tile_map/public/geohash_layer.js

-          blur: parseFloat(this._geohashOptions.heatmap.heatBlur),
-          maxZoom: parseFloat(this._geohashOptions.heatmap.heatMaxZoom),
+          radius: radius,
+          blur: radius * 0.75,


imho, could be a tad blurrier, but that's just me.

thomasneirynck · 2017-08-08T04:11:25Z

src/ui/public/agg_response/geo_json/grid_dimensions.js

+
+export function gridDimensions(precision) {
+  return _.get(gridAtEquator, precision);
+}


nice utility. Would you mind moving kibana/src/ui/public/utils/zoom_to_precision.js to this folder as well? for some reason, that ended up in some rando util folder, but would fit much better alongside this module here.

Is this the best place for zoom_to_precision? Precision is used to create aggregation request. I stuck grid_dimensions into the aggregation response folder since it is used on the response. Maybe there should be a top level geo_utils folder so geo stuff can be gathered into a single place that is outside of aggregation and visualizations.

ok. makes sense

thomasneirynck · 2017-08-08T04:15:59Z

src/core_plugins/tile_map/public/editors/tile_map.html

-    </div>
-  </div>
-  <div>
-    <label>
      Minimum opacity


I'd label this as something more wishy-washy: 'Granularity', 'Intensity'. The reason for that is that it ties us less to the L.heatlayer implementation and would make it easier to backfill this generic property with another heat-implementation.

Range form 0-1 remains good though.

thomasneirynck · 2017-08-08T04:17:59Z

src/core_plugins/tile_map/public/kibana_map.js

+    const latLngY = this._leafletMap.containerPointToLatLng(pointY);
+
+    const distanceX = latLngC.distanceTo(latLngX); // calculate distance between c and x (latitude)
+    const distanceY = latLngC.distanceTo(latLngY); // calculate distance between c and y (longitude)


good idea to measure the diagonal. heatmap-grids change aspect-ratio per zoom level, this is a good approximation.

thomasneirynck · 2017-08-08T04:20:20Z

src/core_plugins/tile_map/public/kibana_map.js

-      zoom: options.zoom ? options.zoom : 2
+      zoom: options.zoom ? options.zoom : 2,
+      renderer: L.canvas(),
+      zoomAnimation: false // Desaturate map tiles causes animation rendering artifacts


ok to switch this off for now.

Client-side desaturation is a problem. we need a base-layer on the server-side that's already desaturated and work towards vector mapping on the client.

thomasneirynck · 2017-08-08T04:30:43Z

src/core_plugins/tile_map/public/kibana_map.js

@@ -100,7 +100,9 @@ export class KibanaMap extends EventEmitter {
      minZoom: options.minZoom,
      maxZoom: options.maxZoom,
      center: options.center ? options.center : [0, 0],
-      zoom: options.zoom ? options.zoom : 2
+      zoom: options.zoom ? options.zoom : 2,
+      renderer: L.canvas(),


by using canvas now, I'd hoped we could have turned off this client-side filtering hack:

https://github.com/thomasneirynck/kibana/blob/72f6b8a37fe76c9d799fc7af9f7dd2358789d987/src/core_plugins/tile_map/public/markers/scaled_circles.js#L30-L35
https://github.com/thomasneirynck/kibana/blob/23d903746383c2fc3d55c0cf1075a686aa6cb0a9/src/core_plugins/tile_map/public/geohash_layer.js#L92-L92

However, when turning this off, it remains pretty jittery. so ill guess we should leave it in. :(

nreese · 2017-08-08T14:34:12Z

@thomasneirynck

Should the heatmap intensity change based on the min/max range? The other geohash layers vary the number of categories based on the the range. Should heatmap mimic this? For example, when the range is below 2, the intensity would be 0.9. When the range is between 2 and 24, the intensity is 0.6. And for everything else, the intensity is 0.1.

The cutoffs of 2 and 24 seem arbitrary. Would there be a better way to create the cutoffs based on statistical values?

nreese · 2017-08-18T21:18:24Z

jenkins, test this

ppisljar

LGTM, thanks a lot @nreese this is amazing!

thomasneirynck · 2017-08-22T19:42:11Z

jenkins, test this

thomasneirynck

LGTM!

the simplified heatmap controls are really nice!

Can you create a new issue for the missing region maps test and put a 6.0 GA label on it?

thomasneirynck · 2017-08-22T20:12:49Z

src/ui/public/agg_response/geo_json/grid_dimensions.js

+
+export function gridDimensions(precision) {
+  return _.get(gridAtEquator, precision);
+}


ok. makes sense

nreese · 2017-08-22T20:33:08Z

@thomasneirynck I created the issue for region map tests, #13652

thomasneirynck · 2017-08-22T21:27:45Z

just occurred to me: let's change the corresponding doc as well.

https://github.com/thomasneirynck/kibana/blob/5a20d44a081b347aabd1cc1813785880debe85a2/docs/visualize/tilemap.asciidoc#L80-L80

nreese · 2017-08-22T21:34:33Z

@thomasneirynck Will do. While in the ascii docs, I will also update https://github.com/elastic/kibana/blob/master/docs/migration/migrate_6_0.asciidoc

…h grids

…al tests

…ectors from DOM

thomasneirynck · 2017-08-23T03:17:04Z

🚀

* upgrade leaflet modules to 1.x latest * fix css for leaflet controls * update draw options so it is easier to see polygon when drawing * fix chrome touch issues with closing polygon in leaflet draw * use canvas renderer * use leaflet-responsive-popup to avoid tooltip cutoff * remove radius configuration from leaflet heatmap * make blur a factor of radius. Set maxZoom to map zoom since new values are calculated per precision * use _.get to avoid error, cannot read property of undefined * add cluster size slider * experiments with image differences * onload not onLoad * use canvas dimensions * compare map canvas to stored PNG of map canvas * remove pixelmatch from project dependecies * fix broken test - rounding error * add expected image tests for geohash layers shaded circles and geohash grids * bump z-index of vis-spy * update functional test expected data * update to leaflet 1.2.0 * revert to leaflet 1.0.3 and update expected data set for map functional tests * test geohash_layer heatmap in unit test * update region_map functional test since it can not longer pluck map vectors from DOM * update documentation

epixa · 2017-08-23T19:42:10Z

@nreese Does this need to go into 6.x as well? It looks like it's only in master/6.0

nreese · 2017-08-23T20:05:45Z

@epixa yes, this needs to be merged with 6.x. I am having a problem with another PR, #13667, on 6.x and CI. My plan was to migrate this PR to 6.x once the other PR passes CI and gets merged

* upgrade leaflet modules to 1.x latest * fix css for leaflet controls * update draw options so it is easier to see polygon when drawing * fix chrome touch issues with closing polygon in leaflet draw * use canvas renderer * use leaflet-responsive-popup to avoid tooltip cutoff * remove radius configuration from leaflet heatmap * make blur a factor of radius. Set maxZoom to map zoom since new values are calculated per precision * use _.get to avoid error, cannot read property of undefined * add cluster size slider * experiments with image differences * onload not onLoad * use canvas dimensions * compare map canvas to stored PNG of map canvas * remove pixelmatch from project dependecies * fix broken test - rounding error * add expected image tests for geohash layers shaded circles and geohash grids * bump z-index of vis-spy * update functional test expected data * update to leaflet 1.2.0 * revert to leaflet 1.0.3 and update expected data set for map functional tests * test geohash_layer heatmap in unit test * update region_map functional test since it can not longer pluck map vectors from DOM * update documentation

thomasneirynck self-requested a review June 28, 2017 23:00

thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:enhancement labels Jun 28, 2017

nreese force-pushed the leaflet branch from fc26f27 to f48b293 Compare July 27, 2017 15:06

thomasneirynck reviewed Aug 2, 2017

View reviewed changes

nreese force-pushed the leaflet branch from f84ce24 to 948fd6a Compare August 3, 2017 14:10

thomasneirynck self-requested a review August 7, 2017 21:00

thomasneirynck reviewed Aug 8, 2017

View reviewed changes

thomasneirynck self-requested a review August 9, 2017 19:14

nreese force-pushed the leaflet branch from 57bac83 to e68b243 Compare August 18, 2017 17:45

thomasneirynck requested a review from ppisljar August 22, 2017 13:09

ppisljar approved these changes Aug 22, 2017

View reviewed changes

thomasneirynck approved these changes Aug 22, 2017

View reviewed changes

thomasneirynck added v6.0.0-beta2 v6.1.0 v7.0.0 labels Aug 22, 2017

nreese added 6 commits August 22, 2017 16:30

upgrade leaflet modules to 1.x latest

62a7f0e

fix css for leaflet controls

727d8a0

update draw options so it is easier to see polygon when drawing

99596b4

fix chrome touch issues with closing polygon in leaflet draw

6f65dff

use canvas renderer

43a489c

use leaflet-responsive-popup to avoid tooltip cutoff

a0da09a

nreese added 15 commits August 22, 2017 16:30

add cluster size slider

ee28bcc

experiments with image differences

cb05aaa

onload not onLoad

ce14885

use canvas dimensions

17d8c93

compare map canvas to stored PNG of map canvas

831ea09

remove pixelmatch from project dependecies

41bb04e

fix broken test - rounding error

7a3952b

add expected image tests for geohash layers shaded circles and geohas…

3309f70

…h grids

bump z-index of vis-spy

89683db

update functional test expected data

5057647

update to leaflet 1.2.0

b6cb492

revert to leaflet 1.0.3 and update expected data set for map function…

42ee912

…al tests

test geohash_layer heatmap in unit test

5d702c8

update region_map functional test since it can not longer pluck map v…

5a56b7b

…ectors from DOM

update documentation

bbe3fd0

nreese force-pushed the leaflet branch from 9046d29 to bbe3fd0 Compare August 22, 2017 22:31

nreese merged commit 599c8c4 into elastic:master Aug 22, 2017

ppisljar mentioned this pull request Aug 23, 2017

Simplify heatmap controls #11759

Closed

nreese mentioned this pull request Aug 23, 2017

[6.0] Upgrade kibana to leaflet 1.x #13669

Merged

nreese mentioned this pull request Aug 24, 2017

[6.x] Upgrade kibana to leaflet 1.x #13688

Merged

timroes mentioned this pull request Apr 18, 2018

Upgrade to Leaflet 1.0.3 #10276

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade kibana to leaflet 1.x #12367

Upgrade kibana to leaflet 1.x #12367

nreese commented Jun 15, 2017 •

edited

Loading

thomasneirynck commented Aug 2, 2017

thomasneirynck commented Aug 2, 2017

thomasneirynck left a comment •

edited

Loading

thomasneirynck left a comment

thomasneirynck Aug 8, 2017

thomasneirynck Aug 8, 2017

thomasneirynck Aug 8, 2017

thomasneirynck Aug 8, 2017

nreese Aug 8, 2017

thomasneirynck Aug 22, 2017

thomasneirynck Aug 8, 2017

thomasneirynck Aug 8, 2017

thomasneirynck Aug 8, 2017

thomasneirynck Aug 8, 2017

nreese commented Aug 8, 2017

nreese commented Aug 18, 2017

ppisljar left a comment

thomasneirynck commented Aug 22, 2017

thomasneirynck left a comment

thomasneirynck Aug 22, 2017

nreese commented Aug 22, 2017

thomasneirynck commented Aug 22, 2017

nreese commented Aug 22, 2017

thomasneirynck commented Aug 23, 2017

epixa commented Aug 23, 2017

nreese commented Aug 23, 2017

Upgrade kibana to leaflet 1.x #12367

Upgrade kibana to leaflet 1.x #12367

Conversation

nreese commented Jun 15, 2017 • edited Loading

thomasneirynck commented Aug 2, 2017

thomasneirynck commented Aug 2, 2017

thomasneirynck left a comment • edited Loading

Choose a reason for hiding this comment

thomasneirynck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese commented Aug 8, 2017

nreese commented Aug 18, 2017

ppisljar left a comment

Choose a reason for hiding this comment

thomasneirynck commented Aug 22, 2017

thomasneirynck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese commented Aug 22, 2017

thomasneirynck commented Aug 22, 2017

nreese commented Aug 22, 2017

thomasneirynck commented Aug 23, 2017

epixa commented Aug 23, 2017

nreese commented Aug 23, 2017

nreese commented Jun 15, 2017 •

edited

Loading

thomasneirynck left a comment •

edited

Loading