Add topojson support / EMS v3 support by thomasneirynck · Pull Request #15361 · elastic/kibana

thomasneirynck · 2017-12-02T01:04:47Z

This PR adds support for Topojson and the EMS v3 maps backend. It also refactors some of the common code between Coordinate Map and Region Map.

~~This is a rough outline for adding topojson support to Region Maps. Closes #14331.~~

Adding topojson layers raises a few new issues that need to be solved.

I'm not sure the current single format as a string parameter will be sufficient. We need more metadata for topojson , which includes the path where the features are in the topology.
the current joining mechanism is too slow. We potentially need to join tens of thousands of geojson features against thousands of ES term-buckets. I changed it to use a map, but even that should be improved. It's probably wrong to rely on the onFeature callbacks coming out of Leaflet, which have no defined order. Rather, we should just do that join ourselves. (SHOULD EVALUATE IF THIS WAS WORTH IT)
the current outline styling is too thick. Since we'll have more features, they'll tend to be smaller, so we should make the outline configurable (cf. Make line styling of region maps configurable #15360)
we should offer the ability to exclude shapes from the map if they don't match any of the terms. Right now, they stay on as unstyled features. The annoying part is that to do with we have to recreate the leaflet layer from scratch because filtering cannot be applied after layer instantiation.
multi-baselayer support. This requires more changes to the manifest, so we can display something human readable.

nyurik · 2017-12-02T05:57:01Z

In theory we could standardize our topojsons to always have the same object hierarchy. We don't have to use multiple top-level subobjects, but instead standardize on {objects:{data: ...}}. In this case we won't need to store extra topojson file-specific metadata.

nyurik · 2017-12-02T05:58:13Z

P.S. I used this approach in Kartotherian - https://github.com/kartotherian/geoshapes/blob/master/geoshapes.js#L406

thomasneirynck · 2017-12-12T20:50:59Z

I'm going to expand this PR to support other EMS v3 features

include multi-baselayer support
add docs for custom configs for on-prem deployments

thomasneirynck · 2017-12-13T00:36:07Z

I added support for multibase layer (hypothetical, since EMS doesn't publish multiple layers yet). It's a rough outline, not intende to go in as-is. I changed the wms-options now to wrap the entire baselayer configuration.

~~For some reason, this broke the Coordinate Map visualization. It's broken even when excluding the wms-options Not sure why yet..~~ due to zoom-settings not being applied correctly. will fix.

thomasneirynck · 2017-12-13T23:21:40Z

@timroes @ppisljar This is an outline still, but most of the features that should go in, are in. I'm not sure if we need "more" features in order to support the use-case. Tests/docs are missing.

The biggest issue is that we actually don't have an EMS v3 yet. So this requires some manual configurations in the yaml to make topojson work.

Things that can be evaluated already:

do join upfront and don't rely on looping over features in Leaflet. This is a huge performance win when dealing with a lot of shapes. It does introduce more complexity though. For example, we need to rejoin each time the terms-data changes.
ability to only display matching shapes. This has an impact on our ability of reusing choroplethlayer-instances. You can't remove features from leaflet-layers or change filters-on-the-fly, so we might have to recreate the instance if the data changes.
multi-baselayer support (right now only showing single option, see EMSv3 comment above). I redid the existing wms-option directive and moved it there. This addition impacts the Coordinate Map as well, and required changes to the serviceSettings loader
new line-width configuration

I'd like to merge #15507 as well, so I can rebase with the added unit tests. So I'd prioritize that one first for review.

timroes · 2017-12-18T18:01:30Z

<tile-map-vis-params>

timroes · 2017-12-18T18:02:44Z

Get rid of the link function if we don't need it.

Use child scope for the directive instead of writing to vis.params directly in the template.

nyurik · 2017-12-18T20:47:03Z

I wonder if we should integrate https://github.com/briannaAndCo/Leaflet.Antimeridian into this as well, to solve all potential anti-meridian issues. Might need to evaluate performance hit, or make it optional (e.g. the catalog file will mark a certain dataset as requiring a special antimeridian handling)

thomasneirynck · 2018-01-04T03:40:37Z

Attached is an example topojson file that you can use for testing. It works with the makelogs data.

world_countries.topojson.zip

Drop it in src/core_plugins/region_map/public/data and use following configuration in the kibana.yml:

regionmap:
  includeElasticMapsService: true
  layers:
     - name: "World countries (self hosted, topojson)"
       url: "../plugins/region_map/data/world_countries.topojson"
       format:
          type: 'topojson'
       meta:
          feature_collection_path: 'collection'
       fields:
          - name: "iso2"
            description: "Two letter abbreviation"

ppisljar

some minor comments and some additional questions ...
need to do some more testing before approving

ppisljar · 2018-01-04T07:25:37Z

The module doesn't seem to be much used and kept up to date ? (seems last change was half a year ago). Can we rely on it ?

it seems legit to me. Written by the creator of d3 and frequently downloaded. It's a subset of https://www.npmjs.com/package/topojson, with only some conversion functions.

ppisljar · 2018-01-04T07:26:28Z

the only change in this file is the removal of this empty line ....

oh, oops! ;)

ppisljar · 2018-01-04T07:26:57Z

what was the default outlineWeight before ?

ppisljar · 2018-01-04T07:28:30Z

the comment should probably move on its own line to make it easier to notice.

ppisljar · 2018-01-04T07:31:11Z

why are this defaults hardcoded here and not read from vis.params ?

These settings are driven by the base-layers and those are read from the EMS-manifests. These are just placeholders when we create the map, but they get overridden as baselayers are configured.

I did create variables for them.

ppisljar · 2018-01-04T07:33:11Z

why only updating map zoom if maxZoom is bigger than current maxZoomLevel ?

ppisljar · 2018-01-04T07:34:43Z

changes to this file are unnecessary, should remove to make the change cleaner

ppisljar · 2018-01-04T07:35:13Z

ppisljar · 2018-01-04T07:38:58Z

can you quickly explain why all this calls to updateChoroplethLayer ? (4 calls to updateChoroplethLayerForXXX)

After looking into this more in-depth, I can't really trigger this being called 4 times in a single render call.

This may get called 2 times in a single render call. Once when there is a data change and once when there is a change in the params.

These checks arose due to a limitation of leaflet, that doesn't allow us to change the data after we have assigned it to a layer. That's why we need to check if we can reuse the existing layer or create a new instance. Whether we can reuse it depends on a number of factors:

if any parameters change that affect the join-parameters, we cannot reuse the instance

if the data changes, we cannot reuse the instance.

I think it is working as intended. The checks avoid recreating an instance of the leaflet-layer and downloading new data when we shouldn't. This is an important feature. If we remove this and redraw the visualization from scratch for each render call, this will absolutely kill responsiveness of the map.

ppisljar · 2018-01-04T07:41:15Z

this diff got really bad, most of this functions did not really change just their position in the file did ....

thomasneirynck · 2018-01-09T04:51:47Z

@nyurik wrt. antemeridian. I wouldn't make this part of this PR, but we can consider it. Our reliance on leaflet plugins is a long term liability rather than an asset. I'd reevaluate when we get more community feedback on this issue. Feel free to create a bug report for it too.

ppisljar · 2018-01-12T12:37:23Z

i don't have any more comments on code and as far as i could test this seems to work.
waiting for docs and tests to pass.

timroes

Besides some minor code suggestions and the still missing tests and docs this LGTM. Though, also after some readings and watching Thomas' explanation to Peter, I still don't have a very deep understanding of the domain, so not sure if I could validate everything correctly.

timroes · 2018-01-12T15:56:09Z

I guess we shouldn't rely on non breaking whitespaces for styling, but rather fix the accordingly CSS. Also as far as I know since the switch to EUI, these icons now looks okay spacing wise. Maybe rebase this on master and remove that whitespace.

timroes · 2018-01-12T18:21:05Z

Needs to be removed

timroes · 2018-01-12T18:22:43Z

I guess the true isn't needed here?

I preferred the promise to resolve to a success-flag iso. of just undefined, as it's being used as a boolean check by the client.

I am fine with leaving it in, even though I think, the promise should just reject if there are cases where it's not successful, and a resolved promise should always mean successful, which would render the true useless imho. But I won't nitpick on this one :D

i agree 100% that if promise was not succesful it should reject instead of resolve with false

timroes · 2018-01-12T18:31:13Z

I think this could be shortened, by using the .find method?

const fileService = catalogue.services.find(service => service.type === 'file'); if (!fileService) { return []; }

timroes · 2018-01-12T18:31:25Z

.find could be used here too

thomasneirynck · 2018-01-16T07:20:43Z

jenkins, test this

timroes · 2018-01-16T08:27:50Z

Please don't comment out tests, but rather use .skip so it will still be tracked, that these tests exist, and just skipped for the moment. Please also open an issue for all skipped tests, to investigate and get them working again.

ppisljar · 2018-01-16T08:29:22Z

    const northWest = bounds.getNorthWest();
    if (
-      southEast.lng === northWest.lng &&
+      southEast.lng === northWest.lng ||


why this change ?

the and-case didn't trap the case where the visualization would have a width, but not a height (or vice-versa).

ppisljar · 2018-01-16T08:44:08Z

seems old map visualizations (saved before this PR) no longer work on dashboard:

timroes · 2018-01-16T08:57:45Z

Does this PR break previously created visualizations, that has a custom map server configured? They would now require a EMSv3 server, while beforehand an EMSv2. Is EMSv3 backward compatible, so this visualization will still work with any EMSv2 server? If not I think we need a breaking change warning on this, since it would require all users with custom map servers to update Kibana in parallel to upgrade their map server to use EMSv3.

ppisljar · 2018-01-16T09:05:03Z

old visualizations should still work, we can't expect users to recreate all their map visualizations.

same seems to happen for region map

Dismiss Approval until backward compatibility is fixed.

thomasneirynck · 2018-01-16T14:44:36Z

@timroes there doesn't need to be backward compatibility between Kibana 6.2 and up, and EMSv2. Kibana 6.2 will only connect to EMSv3 and never to any of the older EMS-endpoints. Likewise, none of the older versions of Kibana will connect to EMSv3.

From the EMS end, we do need to ensure that every service that is published in v2 remains available in EMSv3. This is to ensure that maps saved in older versions will have the same look&feel when reopened in a later version of Kibana.

thomasneirynck · 2018-01-16T14:46:32Z

thanks @ppisljar . That's a bug due to the service-settings for EMS only being loaded in the UI-editor component. I will fix this.

thomasneirynck · 2018-01-16T14:50:46Z

@timroes there are also no user-hosted EMS-versions, so users will not be able to run v2 themselves.

ppisljar

LGTM, i wonder if we could reenable the other test as well ?

ppisljar · 2018-01-16T17:59:01Z

-        expect(prettyPrint).to.equal('Today');
-      });
-    });
+    // describe('time changes', function () {


what about this test? could we reenable this one as well ?

ppisljar · 2018-01-16T18:00:13Z

also update PR description and open a new issue for the docs and link it here ?

thomasneirynck · 2018-01-16T18:08:11Z

@ppisljar that test is flaky and fails/succeeds locally. it doesn't even have anything to do with maps, or even a populated dashboard. I created a separate ticket #16055, I'd prefer not get hung up on it for this PR.

+1 on separate doc ticket

thomasneirynck · 2018-01-16T19:39:24Z

last test run before merging.

jenkins, test this

This adds support for the v3 endpoint of the Elastic Maps Service. This includes support for Topojson files.

thomasneirynck · 2018-01-16T21:42:20Z

Backport:

6.x: [6.x] Add topojson support / EMS v3 support (#15361) #16076

This adds support for the v3 endpoint of the Elastic Maps Service. This includes support for Topojson files.

bhavyarm · 2018-02-06T09:13:53Z

@thomasneirynck do we have functional tests for this PR? Thanks!

thomasneirynck · 2018-02-06T19:47:40Z

@bhavyarm there's no additional functional test, although there is more unit test.

This is mostly a backend change to accomodate the new EMS-service, less so a functional change in Kibana.

This adds support for the v3 endpoint of the Elastic Maps Service. This includes support for Topojson files.

thomasneirynck added Feature:Region Map release_note:enhancement v6.2.0 v7.0.0 WIP Work in progress release_note:roadmap labels Dec 2, 2017

thomasneirynck mentioned this pull request Dec 7, 2017

Fix invalid method call #15483

Closed

thomasneirynck requested review from ppisljar and timroes December 8, 2017 02:00

thomasneirynck changed the title ~~[WIP] add topojson support~~ [WIP] add topojson support / EMS v3 support Dec 12, 2017

thomasneirynck force-pushed the enh/topojson branch from 617e23d to f324422 Compare December 13, 2017 00:33

thomasneirynck changed the title ~~[WIP] add topojson support / EMS v3 support~~ Add topojson support / EMS v3 support Dec 15, 2017

thomasneirynck force-pushed the enh/topojson branch from b0884d7 to ebcd630 Compare December 15, 2017 23:26

timroes reviewed Dec 18, 2017

View reviewed changes

timroes self-requested a review December 18, 2017 18:07

thomasneirynck force-pushed the enh/topojson branch 2 times, most recently from 2747325 to d195a76 Compare December 29, 2017 19:56

ppisljar reviewed Jan 4, 2018

View reviewed changes

thomasneirynck force-pushed the enh/topojson branch from 94a77e9 to 449441c Compare January 9, 2018 03:10

timroes previously approved these changes Jan 12, 2018

View reviewed changes

thomasneirynck force-pushed the enh/topojson branch from 77d8e8d to 33714c1 Compare January 15, 2018 04:39

thomasneirynck mentioned this pull request Jan 16, 2018

Fix flaky dashboard map test #16055

Closed

see if CI now builds

97eae81

ppisljar reviewed Jan 16, 2018

View reviewed changes

thomasneirynck added 2 commits January 16, 2018 10:04

load default layer

6892efc

reactivate test

a05c638

ppisljar approved these changes Jan 16, 2018

View reviewed changes

timroes approved these changes Jan 16, 2018

View reviewed changes

thomasneirynck merged commit 073f375 into elastic:master Jan 16, 2018

thomasneirynck mentioned this pull request Jan 16, 2018

[6.x] Add topojson support / EMS v3 support (#15361) #16076

Merged

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jan 16, 2018

Add topojson support / EMS v3 support (elastic#15361)

47a283f

This adds support for the v3 endpoint of the Elastic Maps Service. This includes support for Topojson files.

thomasneirynck added a commit that referenced this pull request Jan 16, 2018

Add topojson support / EMS v3 support (#15361) (#16076)

5737f07

This adds support for the v3 endpoint of the Elastic Maps Service. This includes support for Topojson files.

thomasneirynck added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Jan 23, 2018

thomasneirynck mentioned this pull request Mar 15, 2018

Make line styling of region maps configurable #15360

Closed

aphelionz mentioned this pull request Jun 12, 2018

Accessibility: Kibana 6.3 Meta Issue #18866

Closed

34 of 48 tasks

patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026

Add topojson support / EMS v3 support (elastic#15361)

5388a50

This adds support for the v3 endpoint of the Elastic Maps Service. This includes support for Topojson files.

Conversation

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

Uh oh!

Uh oh!

nyurik commented Dec 2, 2017

Uh oh!

nyurik commented Dec 2, 2017

Uh oh!

thomasneirynck commented Dec 12, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

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!

nyurik commented Dec 18, 2017

Uh oh!

thomasneirynck commented Jan 4, 2018

Uh oh!

ppisljar 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!

Choose a reason for hiding this comment

Uh oh!

thomasneirynck Jan 9, 2018 • 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 Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppisljar commented Jan 12, 2018

Uh oh!

timroes left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasneirynck commented Dec 2, 2017 •

edited

Loading

thomasneirynck commented Dec 13, 2017 •

edited

Loading

thomasneirynck commented Dec 13, 2017 •

edited

Loading

thomasneirynck Jan 9, 2018 •

edited

Loading

thomasneirynck commented Jan 9, 2018 •

edited

Loading

timroes left a comment •

edited

Loading

timroes commented Jan 16, 2018 •

edited

Loading

thomasneirynck commented Jan 16, 2018 •

edited

Loading