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

[Maps] Add 3rd party vector tile support #62084

Merged
merged 58 commits into from
Apr 16, 2020

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Mar 31, 2020

This adds the core boilerplate to Maps to display vector-tiles as first-class layer.

Introduces:

  • a new layer-type (TiledVectorLayer): the layer that is added to the layer-list
  • a new source-type (ITiledSingleLayerVectorSource): a source which models a url-template and layer-name. It is restricted to a single source-layer.
  • an implementation and wizard for 3rd party services.

3rdpartyvector_ink

The scope is limited to keep this PR small. Below features should be added in other PRs.

  • add dynamic styling
  • tooltip support (this requires some additional refactors in the existing code (see [Maps] [Meta] Add .mvt vector tile support #58519)
  • limited to a single source-layer per source (fwiw I think that's an ok limitation, especially if we fix the below)
  • identical tile-sources are not shared across mb-layers. This is performance micro-optimization that would significantly impact the existing mb layer-management code.
  • require hardcoding of the url-template and layer-name. TileJson support is a larger (separate) effort. (ie. and some 3rd party services do not implement TileJson anyway).
    • TileJson support will also give us the field metadata that can be used for dynamic styling
  • add attribution: this is blocked by [Maps] Cannot put in attribution for all source types #32821 (comment)

Note that the endgoal is primarly to support tiling from Elasticsearch data, which also informs some of the limitations discussed above (ie. single source-layer, no TileJson support)


Reviewer notes:

  • Note: this PR is stuck in the TS-swamp for a moment, please disregard compilation errors. Will aim to address this some of the larger TS-issues in other PRs. (cf. [Maps] Cleanup sources #63175)
  • vector tile services have native zoom levels. Users will need to configure this to avoid 404s when connecting to the service. It also helps with providing feedback about visibility in the TOC

To test:

@thomasneirynck thomasneirynck added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.8.0 v8.0.0 WIP Work in progress labels Apr 6, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@jsanz
Copy link
Member

jsanz commented Apr 7, 2020

This is a lot of fun!! Tested locally, I couldn't use EMS tiles on my set up, but worked smoothly with another tile provider. You have already captured all small UI things I also noticed (reusing the source, tooltips, style-driven rendering).

For what is worth, I used this snippet to get the names of the layers using vt2geojson:

$ vt2geojson https://url.tile.provider/0/0/0.mvt | jq -c ".features[].properties.vt_layer" | uniq  
"water"
"landcover"
"boundary"
"water_name"
"place"

Note that layers change depending on the zoom level.

@thomasneirynck
Copy link
Contributor Author

thanks @jsanz for checking out! This PR is a little too rough around the edges for detailed reviewed, but we should certainly discuss scope on this. I'm looking to dial in additional functionality in small steps, but we probably want to avoid getting in a situation where we merged something in master/7.x that isn't feature complete enough to be useful.

An interim option, in absence of clearly defined boundaries, might to hide the vector-tile layer-type behind a feature-flag in the 7.x branch (could keep this enabled by default in master) ?

@jsanz
Copy link
Member

jsanz commented Apr 7, 2020

+1 on keeping this behind a feature flag, maybe some folks will benefit from it if they really know their trade, but for most of our users, this would be confusing at this moment.

@thomasneirynck thomasneirynck requested a review from nreese April 14, 2020 18:56
@thomasneirynck thomasneirynck requested a review from nreese April 15, 2020 20:50
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

App Arch code changes LGTM

return new TiledVectorLayer(vectorLayerArguments);
}

getGeoJsonWithMeta(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be deleted? This no longer implements VectorSource so no need for the method

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm with green CI
code review, tested in chrome

@thomasneirynck thomasneirynck removed WIP Work in progress discuss labels Apr 16, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@thomasneirynck thomasneirynck merged commit ad41eea into elastic:master Apr 16, 2020
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Apr 16, 2020
Adds support for adding an external vector tile service to Maps. This is experimental functionality. To enable, add `xpack.maps.enableVectorTiles: true` to the `kibana.yml`configuration file.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 17, 2020
* master: (40 commits)
  [APM]Upgrade apm-rum agent to latest version to fix full page reload (elastic#63723)
  add deprecation warning for legacy 3rd party plugins (elastic#62401)
  Migrate timelion vis (elastic#62819)
  Replacebad scope link with actual values (elastic#63444)
  Index pattern management UI -> TypeScript and New Platform Ready (create_index_pattern_wizard) (elastic#63111)
  [SIEM] Threat hunting enhancements: Filter for/out value, Show top field, Copy to Clipboard, Draggable chart legends (elastic#61207)
  [Maps] fix term join agg key collision (elastic#63324)
  [Ingest] Fix agent config key sorting (elastic#63488)
  [Monitoring] Fixed server response errors (elastic#63181)
  update elastic charts to 18.3.0 (elastic#63732)
  Start services (elastic#63720)
  [APM] Encode spaces when creating ML job (elastic#63683)
  Uptime 7.7 docs (elastic#62228)
  [DOCS] Updates remote cluster and ccr docs (elastic#63517)
  [Maps] Add 3rd party vector tile support (elastic#62084)
  [Endpoint][EPM] Retrieve Index Pattern from Ingest Manager (elastic#63016)
  [Endpoint] Host Details Policy Response Panel (elastic#63518)
  [Uptime] Certificate expiration threshold settings (elastic#63682)
  Refactor saved object types to use `namespaceType` (elastic#63217)
  [SIEM][CASE] Create comments sequentially (elastic#63692)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 20, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

thomasneirynck added a commit that referenced this pull request Apr 21, 2020
Adds support for adding an external vector tile service to Maps. This is experimental functionality. To enable, add `xpack.maps.enableVectorTiles: true` to the `kibana.yml`configuration file.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 21, 2020
@kmartastic
Copy link
Contributor

Can we update this text before 7.9 releases?
image

@thomasneirynck
Copy link
Contributor Author

Can we update this text before 7.9 releases?

@kmartastic yes, do you have a preference?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants