Geocentroid / tilemap bug fixes by thomasneirynck · Pull Request #10871 · elastic/kibana

thomasneirynck · 2017-03-23T19:22:44Z

Summary:

Kibana now uses the geo-centroid of the results to place results on a map. The result is a more natural looking map with fewer visual artifacts. It avoids the "gridded"-look from earlier versions. The map now also supports panning&zooming with touchscreens, as well as bug fixes that improve stability.

New PR, without the vector map. This is so we can resolve the blocker for X-pack quicker.

avoid unnecessary call to manifest/do not hang up Kibana while manifest is requested: Tilemap manifest loading in 5.2 blocks loading of Kibana for up to 60s when behind a firewall #10205
avoid map flicker when zooming: Map is recreated too often #8823
enable scroll, pinch, double click zooming: [Map] Enable Zoom with scroll wheel #7778, Tile Map double-click to zoom only works on Heatmap #6344
avoid Heatmap errors: Uncaught IndexSizeError: Failed to execute 'getImageData' on 'CanvasRenderingContext2D': The source width is 0. #6343 / Tilemap Heatmap error - possible cleanup issue #6226
map doesnt fill screen in dashboard: Tilemap - Dashboard visualization size #10477
fit should work consistently Tilemap "Fit data bounds" should have the same result regardless of the starting zoom #10654
remove constraints on map positioning Tilemap constraints on map positioning #9653 / Map always seems to centre on Africa #4944
use geocentroid as default position for Use geo-centroid aggregator for geo-hash grids #5464

todos:

update test code
shouldn't use zoom level 0 for WMS

ppisljar · 2017-03-24T10:12:10Z

functionality wise it looks great! i also gave a quick look at the code and can't find anything. im gonna give it another run later today.

kobelb

I copied over the relevant comments from the previous PRs, so a lot of this is the same, but a few things are new.

kobelb · 2017-03-24T11:26:15Z

+import KibanaMapLayer from './kibana_map_layer';
+import _ from 'lodash';
+import { EventEmitter } from 'events';
+import Heatmap from './heatmap';


Thank you for splitting this file up. It might be worthwhile to put all of the Layers into a folder.

kobelb · 2017-03-24T12:17:13Z

-    return _.sortBy(this, function (agg) {
+
+
+    const aggregations = _.sortBy(this, function (agg) {


There's currently an extension point in the AppConfig to allow each individual item to customize the responseAggs, I added one for the requestAggs as well so that we can do the geohash logic there and it seems to work: kobelb@fd6b344

That way we don't have to hard-code the logic for specific aggregations here.

We should add unit-tests for this, regardless of which direction we go.

oki, changed it using this approach, thx!

kobelb · 2017-03-24T12:29:50Z

+        break;
+      case 'Heatmap':
+        this._geohashMarkers = new Heatmap(this._geohashGeoJson, {
+          radius: +this._geohashOptions.heatmap.heatRadius,


The + before this is to ensure that they're positive numbers, right? When were we seeing this? It seems like something that should be caught by our validation.

it coerces it to the number type. It was like that before. Agreed it looks weird and is probably not necessary, I will change it.

Ah, forgot you could do it that way in Javascript...

kobelb · 2017-03-24T12:31:44Z

+ * @param geoJson {geoJson Object}
+ * @param params {Object}
+ */
+export default class Heatmap extends EventEmitter {


The heatmap still has this empty legend highlighted by the red rectangle below:

The empty legend is still showing up, when it isn't in 5.2.2. Obviously, not a blocker, but just FYI.

kobelb · 2017-03-24T12:37:08Z

  status: Joi.object({
    allowAnonymous: Joi.boolean().default(false)
  }).default(),
+  vectormap: Joi.object({


This config section snuck in here, even though we won't be adding vectormap support with this PR.

good catch, thanks

kobelb · 2017-03-24T12:41:22Z

@@ -0,0 +1,507 @@
+import { EventEmitter } from 'events';


On tilemaps you're currently able to zoom out all the way, and then there are multiple earths but only one of them has the data on it:

The same zoom issue on save is still there as well. If you zoom in a bunch, and then hit save, it zooms you out all the way.

the data on just a single earth is normal. it is a limitation of leaflet.

zoom on save is fixed now

The snapping back to the map is better, but the data can be empty when it happens:

kobelb · 2017-03-24T12:47:47Z

+
 }

+  .vis-option-item {


This applies to a number of visualizations and IMO looks rather awkward:

thomasneirynck · 2017-03-26T00:56:31Z

jenkins, test this

ppisljar · 2017-03-27T07:16:06Z

not sure if this is even related but if i zoom out completely using wms maps it shows white screen (Seems that zoom level is not available ? )

ppisljar · 2017-03-27T07:20:17Z

is CI broken or are the unit test failures related to this PR ?

thomasneirynck · 2017-03-27T14:11:27Z

@ppisljar tests are broken due to this PR.

add tooltip boilerplate

move legend with option add tms settings add min/max zoom settings add valueformatters

…g map

ppisljar

LGTM

- adds the geo-centroid metric as the new default for visualizations - various bug fixes and improvements - avoid unnecessary calls to manifest - avoid map flicker when zooming - enable scroll/pinch/touch zooming - avoid heatmap errors - ensure map fills screen in dashboard - ensure fit works consistently - relax tilemap constraints - remove support for multi-maps - this refactor sets the stage for new vector map visualization which will reuse the same map components

due to merge conflicts, this also required backporting the resize_checker components * Geocentroid / tilemap bug fixes (#10871) - adds the geo-centroid metric as the new default for visualizations - various bug fixes and improvements - avoid unnecessary calls to manifest - avoid map flicker when zooming - enable scroll/pinch/touch zooming - avoid heatmap errors - ensure map fills screen in dashboard - ensure fit works consistently - relax tilemap constraints - remove support for multi-maps - this refactor sets the stage for new vector map visualization which will reuse the same map components

thomasneirynck requested review from kobelb and ppisljar March 23, 2017 19:22

thomasneirynck mentioned this pull request Mar 23, 2017

Vector map, geocentroid, tilemap bug fixes #10824

Closed

20 tasks

kobelb requested changes Mar 24, 2017

View reviewed changes

tbragin added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Mar 27, 2017

thomasneirynck added 20 commits March 27, 2017 17:58

bootstrap

1ec9508

enable WMS

8596985

extract choropleth in separate class

014d97b

isolate geohash layer in separate module

86beb01

add desaturation flag / more efficient refreshing of baselayer

2de4bcf

add remaining map types

27602f8

add fit control

1d612df

add legend control

378b34d

add tooltip boilerplate

add tooltip flag

33ad073

move legend with option add tms settings add min/max zoom settings add valueformatters

some simplification

b96fa15

fix choropleth

70b5fc4

do not request manifest on route-change, instead call it when creatin…

074b1a6

…g map

fix typo

2b75b02

add rendercomplete

4fe0c09

initial review

ad8c46e

fix resizing

88ebb24

add tooltips

584728a

add legend

915ca87

fix linting errors

5ed5ada

add rendercomplete

db74d44

thomasneirynck added 11 commits March 27, 2017 17:59

move logic of adding geo_centroid to geo_hash agg

5341bd9

move files

2a9e39d

fix zoom on save

9423210

update data on refresh

da418d9

removed unused files

d028ffa

fix tooltips for heatmaps

1ae6dda

make heatmap tooltips not flicker

979802b

remove obsolete files

4aeeffc

resolve linter errors

bc5570d

fix test after removing multiple layouts

c8e79b9

fix failing test after introducing useGeocentroid agg

9ba3894

thomasneirynck force-pushed the feature/tilemap_improvements branch from ad1e852 to 9ba3894 Compare March 27, 2017 22:00

ppisljar approved these changes Mar 28, 2017

View reviewed changes

thomasneirynck added 9 commits March 28, 2017 14:18

checking test stub

c4fb9ab

add resizing tests

a86b22e

checkin forgotten files again

70e9e41

add tests for baselayer loading

ccb5bfb

add geohash test

410f69d

remove log

5d5c86f

add marker tests

3895120

add heatmap tests

815812c

remove faulty test

a66617f

thomasneirynck merged commit 556bfab into elastic:master Mar 29, 2017

thomasneirynck mentioned this pull request Mar 29, 2017

Backport: [5.x] Geocentroid / tilemap bug fixes #10927

Merged

alexfrancoeur mentioned this pull request Mar 30, 2017

[DOCS] Update documentation for geocentroid support #10959

Closed

epixa added the v5.4.0 label Apr 10, 2017

thomasneirynck mentioned this pull request Apr 11, 2017

Tilemap - Dashboard visualization size #10477

Closed

epixa added the v6.0.0 label May 17, 2017

		return _.sortBy(this, function (agg) {


		const aggregations = _.sortBy(this, function (agg) {

Conversation

thomasneirynck commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppisljar commented Mar 24, 2017

Uh oh!

kobelb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobelb Mar 24, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

thomasneirynck commented Mar 26, 2017

Uh oh!

ppisljar commented Mar 27, 2017

Uh oh!

ppisljar commented Mar 27, 2017

Uh oh!

thomasneirynck commented Mar 27, 2017

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thomasneirynck commented Mar 23, 2017 •

edited

Loading

kobelb Mar 24, 2017 •

edited

Loading