[Vega] user should be able to set a specific tilemap service using the mapStyle property #88440

alexwizp · 2021-01-15T09:51:25Z

Summary

This is a PR continued discussion of the next comment

to discuss: in accordance with the following documentation https://www.elastic.co/guide/en/kibana/6.8/vega-with-a-map.html mapStyle param accepts just a 2 values: false and default.

It looks a little odd to me because in some cases it might be useful to let the user set a specific tms layer value. For example, if I set map.tilemap.url param and want to create some vis with that layer and some with the default one (road_map)
{
  $schema: https://vega.github.io/schema/vega/v5.json
  config: {
    kibana: {
      type: map
      mapStyle: road_map
    }
  }
}
To add support of that we just need to remove the following if statement from vis_type_vega/public/data_model/vega_parser.ts
if (res.mapStyle !== `default` && res.mapStyle !== false) {
    this._onWarning(
      i18n.translate('visTypeVega.vegaParser.mapStyleValueTypeWarningMessage', {
        defaultMessage:
          '{mapStyleConfigName} may either be {mapStyleConfigFirstAllowedValue} or {mapStyleConfigSecondAllowedValue}',
        values: {
          mapStyleConfigName: 'config.kibana.mapStyle',
          mapStyleConfigFirstAllowedValue: 'false',
          mapStyleConfigSecondAllowedValue: '"default"',
        },
      })
    );
    res.mapStyle = `default`;
  }
@nreese @thomasneirynck from your point of view should we support that case or not?

Originally posted by @alexwizp in #88316 (comment)

For people who will check these changes:

To test that changes please follow next steps:

Start your Kibana instance with the following configuration property:
map.tilemap.url: http://c.tile.stamen.com/watercolor/{z}/{x}/{y}.jpg
Create or open Vega visualizations with configured Map layer.
You can use the following spec for testing:

{
  $schema: https://vega.github.io/schema/vega/v5.json
  config: {
    kibana: {
      type: map
    }
  }
}

Apply changes and see which tile map layer was applied.
Expected screen:
Set mapStyle option to false
Expected screen:
Set emsTileServiceId option to dark_map. It's one of our default layers. Be sure that map.includeElasticMapsService configuration property is true
Expected screen:

src/plugins/vis_type_vega/public/data_model/vega_parser.ts

docs/user/dashboard/vega-reference.asciidoc

elasticmachine · 2021-01-18T16:19:39Z

Pinging @elastic/kibana-app (Team:KibanaApp)

alexwizp · 2021-01-18T16:19:49Z

@elasticmachine merge upstream

alexwizp · 2021-01-18T19:38:54Z

@elasticmachine merge upstream

nreese · 2021-01-19T15:09:59Z

docs/user/dashboard/vega-reference.asciidoc

+      // Can be set to one of the following:
+      // - set to false to disable base layer;
+      // - set to "default" to use Kibana's default base map layer;
+      // - set "mapStyle" to any Elastic Maps Service (EMS) tile layer id.  


How about listing out valid tile layer ids here?

we cannot guarantee that this doc will be updated on updating available EMS layers =)

I think that is OK, and we can remove once elastic/ems-landing-page#304 is fixed and there is an easy way for users to get the ids from EMS. I will add a comment in elastic/ems-landing-page#304 to update these docs to link to available EMS baselayer ids once they are available.

Comment elastic/ems-landing-page#304 (comment)

I'm still not a big fan of hard coding that id's. I see that currently for some reasons maps_legacy/ServiceSettings returns only road_map and in case if user set the value of map.tilemap.url I also see TMS in config/kibana.yml in that array:

I prefer to keep this message as it for now. But in parallel we are working on migration leaflet -> mapbox #88605 where we use @elastic/ems-client directly. So very soon "road_map_desaturated" and "dark_map" will be also available.

The goal of that PR is to remove wrong if statement from vega_parser.ts.

nreese · 2021-01-19T15:10:15Z

src/plugins/vis_type_vega/public/vega_view/vega_map_view.js

-          defaultMessage: '{mapStyleParam} was not found',
-          values: { mapStyleParam: `"mapStyle":${mapStyle}` },
+          defaultMessage:
+            '{mapStyleParam} was not found. Set to "default" to use Kibana\'s default base map layer or set to "false" to disable base layer',


How about listing out EMS layer ids when EMS is enabled?

I can do it, but from the other hand I don't like how our current vega warning messages looks like. It's just a text without any formatting. I would not like to overload these messages with this information. But if you insist I'll do it.

far enough, maybe just putting the EMS ids in the docs then

alexwizp · 2021-01-20T08:16:40Z

@elasticmachine merge upstream

thomasneirynck

hi @alexwizp

I agree that it would be useful for Vega-users to set custom baselayers (either with map.tilemap.url or by explicitly setting the EMS-layer).

I'm not sure if I'm in favor of overloading the mapStyle parameter to this extent.

`it overloads the mapStyle parameter to now expect three types

boolean false
string default
layer-id (which is an enumeration published by EMS)

The end-user behavior in the context of Kibana would end up being quite complicated:

mapStyle	no map.tilemap.url	map.tilemap.url: foobar/{x}/{y}/{z}
false	.no basemap	no basemap
undefined	auto-switch road_map/dark_theme from EMS	use foobar/{x}/{y}/{z}
`default`	auto-switch road_map/dark_theme from EMS	use foobar/{x}/{y}/{z}
ems layerId enumeration	use the EMS layerId	use the EMS layerId

Overloading mapStyle puts implicit constraints on EMS

This change puts a constraint on EMS. EMS can not introduce a layer called default. Minor, but it is contextual knowledge we then need to maintain going forward.

Rather, my 2c would be to keep this change more minimal.

Is it possible to start with improving the behavior of default?

This would just take into account whether maps.tilemap.url is set. This would align the behavior of Vega with how it is in the Maps-app or in the region and tile-map visualizations.

To introduce the ability to set the EMS layerId, I would do this by adding a new parameter, instead of overloading the mapStyle value.

For example something like:

mapStyle: default

mapStyle: false

mapStyle: ems
emsTileLayerId: dark_theme

For (2), we should start taking into account whether connection to EMS has is been turned off explicitly. Once the mapbox-migration is complete, we can also will need to check whether the on-=rem setup is correct and the license is valid. This then can be surfaced into the warning message.

I think (1) would be a good first step, but for (2), I would wait until the mapbox-migration is complete. Given all the little edge-cases that (2) might introduce, it's probably something Maps-team should take on as well.

thanks, let me know what you think.

alexwizp · 2021-01-21T07:50:58Z

@thomasneirynck from my perspective mapStyle option should accept only false to disable base layer and EMS layerId's. I don't see a lot of sense in default value. It works absolutely the same if we don't pass a value at all.

If we remove default then it doesn't seem to me that this property is overloaded

And I agree with you, let's get back to this PR after we finish with #88605.

…e mapStyle property

alexwizp · 2021-02-02T13:35:14Z

@elasticmachine merge upstream

alexwizp · 2021-02-04T09:29:44Z

@elasticmachine merge upstream

alexwizp · 2021-02-04T11:50:29Z

PR updated as recommended by @thomasneirynck

alexwizp · 2021-02-05T07:40:37Z

@elasticmachine merge upstream

stratoula

It works fine and the code is LGTM, my only concern is that is it a breaking change. Not a huge one but the users that have the mapStyle on default should change it to true. Otherwise they will see an error on the vega maps. So maybe a migration script could be a good solution here? Of course, the vega maps are on experimental phase so we could omit this. I would love your feedback on that @thomasneirynck @nreese

nreese · 2021-02-05T14:41:58Z

It works fine and the code is LGTM, my only concern is that is it a breaking change. Not a huge one but the users that have the mapStyle on default should change it to true. Otherwise they will see an error on the vega maps. So maybe a migration script could be a good solution here? Of course, the vega maps are on experimental phase so we could omit this. I would love your feedback on that @thomasneirynck @nreese

If its possible to write a migration script, then this PR should add one. We should strive to make upgrading between minors as seamless as possible.

stratoula · 2021-02-05T14:56:03Z

I think we can, @alexwizp do you want to also take care of this?

thomasneirynck · 2021-02-05T19:43:52Z

+1 on migration script, but imho I think the breaking change is fairly minor, and given that this is an experimental feature, it's imho something we could live with.

We can also remove default from the documentation (like in this PR), and log a deprecation warning when it is being used (defaulting to true then).

I don't feel strongly either way.

alexwizp · 2021-02-08T08:23:11Z

@elasticmachine merge upstream

alexwizp · 2021-02-08T09:33:54Z

@stratoula @thomasneirynck Let me clarify a little. Currently if user set value for mapStype to 'default' all works as before + we show the warning message.

I'm not a big fan of creating a migration script for that. I see some difficulties on JSON cause on parse/stringify specs with the 'default' value we lose user comment and formatting. Probably we can replace value using the regular string functions. But not sure if it's worth it....

stratoula · 2021-02-08T09:37:40Z

This means, that the map will load, right? And the user will see a warning. Ok I am fine with it if the map team agrees 🙂

thomasneirynck

thx @alexwizp. This is a nice addition, and thanks a lot for the API cleanup.

stratoula

LGTM 👏

# Conflicts: # src/plugins/vis_type_vega/public/vega_view/vega_map_view/view.ts

kibanamachine · 2021-02-09T09:04:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1ecb4ed

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`visTypeVega`	2.7MB	2.7MB	-309.0B

History

💚 Build #104594 succeeded 9e9d59a
💚 Build #104160 succeeded 0ae0b01
💚 Build #103779 succeeded ff32e71
💚 Build #103743 succeeded b338175
💚 Build #102980 succeeded c1ab291

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

…e mapStyle property (elastic#88440) * [Vega] user should be able to set a specific tilemap service using the mapStyle property * Update vega-reference.asciidoc * fix PR comments * rename mapStyle -> emsTileServiceId Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…timeline-and-rollover-info * 'master' of github.com:elastic/kibana: (47 commits) [Fleet] Use TS project references (elastic#87574) before/beforeEach clean up (elastic#90663) [Vega] user should be able to set a specific tilemap service using the mapStyle property (elastic#88440) [Security Solution][Case] ServiceNow SIR Connector (elastic#88655) [Search Sessions] Enable extend from management (elastic#90558) [ILM] Delete phase redesign (rework) (elastic#90291) [APM-UI][E2E] use withGithubStatus step (elastic#90651) Add folding in kb-monaco and update some viewers (elastic#90152) [Grok Debugger] Changed test to wait for grok debugger container to exist to fix test flakiness (elastic#90543) Strongly typed EUI theme for styled-components (elastic#90106) Fix vega renovate label (elastic#90591) [Uptime] Migrate to TypeScript project references (elastic#90510) [Monitoring] Migrate data source for legacy alerts to monitoring data directly (elastic#87377) [Upgrade Assistant] Add A11y Tests (elastic#90265) [Time to Visualize] Adds functional tests for linking/unlinking panel from embeddable library (elastic#89612) [dev-utils/ship-ci-stats] fail when CI stats is down (elastic#90678) chore(NA): remove write permissions on Bazel remote cache for PRs (elastic#90652) chore(NA): move bazel workspace status from bash script into nodejs executable (elastic#90560) Use default ES distribution for functional tests (elastic#88737) [Alerts] Jira: Disallow labels with spaces (elastic#90548) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/timeline/timeline.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/lib/absolute_timing_to_relative_timing.test.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/lib/absolute_timing_to_relative_timing.ts

…e mapStyle property (#88440) (#90745) * [Vega] user should be able to set a specific tilemap service using the mapStyle property * Update vega-reference.asciidoc * fix PR comments * rename mapStyle -> emsTileServiceId Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

alexwizp requested review from KOTungseth, stratoula, thomasneirynck and nreese January 15, 2021 10:08

alexwizp force-pushed the vega-map-style branch from 784db1f to 60b5565 Compare January 15, 2021 10:12

alexwizp self-assigned this Jan 15, 2021

alexwizp added v7.12.0 v8.0.0 Feature:Vega Vega visualizations Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:enhancement labels Jan 15, 2021

nreese reviewed Jan 15, 2021

View reviewed changes

src/plugins/vis_type_vega/public/data_model/vega_parser.ts Show resolved Hide resolved

docs/user/dashboard/vega-reference.asciidoc Outdated Show resolved Hide resolved

alexwizp force-pushed the vega-map-style branch from 4bcb7c1 to 43bfdf7 Compare January 16, 2021 15:21

alexwizp marked this pull request as ready for review January 18, 2021 16:19

alexwizp requested a review from a team January 18, 2021 16:19

nreese reviewed Jan 19, 2021

View reviewed changes

nreese mentioned this pull request Jan 19, 2021

Add the layer_id to the layer description elastic/ems-landing-page#304

Closed

thomasneirynck reviewed Jan 20, 2021

View reviewed changes

alexwizp added the discussion label Jan 21, 2021

[Vega] user should be able to set a specific tilemap service using th…

253e661

…e mapStyle property

alexwizp force-pushed the vega-map-style branch from 9ef4209 to 253e661 Compare February 1, 2021 08:28

Update vega-reference.asciidoc

cf32490

thomasneirynck self-requested a review February 1, 2021 19:10

Merge branch 'master' into vega-map-style

c1ab291

kibanamachine and others added 2 commits February 4, 2021 04:29

Merge branch 'master' into vega-map-style

b338175

fix PR comments

baa268c

rename mapStyle -> emsTileServiceId

ff32e71

Merge branch 'master' into vega-map-style

0ae0b01

stratoula reviewed Feb 5, 2021

View reviewed changes

Merge branch 'master' into vega-map-style

9e9d59a

thomasneirynck approved these changes Feb 8, 2021

View reviewed changes

stratoula approved these changes Feb 9, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into vega-map-style

1ecb4ed

# Conflicts: # src/plugins/vis_type_vega/public/vega_view/vega_map_view/view.ts

alexwizp merged commit 19543d8 into elastic:master Feb 9, 2021

alexwizp mentioned this pull request Feb 9, 2021

[7.x] [Vega] user should be able to set a specific tilemap service using the mapStyle property (#88440) #90745

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vega] user should be able to set a specific tilemap service using the mapStyle property #88440

[Vega] user should be able to set a specific tilemap service using the mapStyle property #88440

alexwizp commented Jan 15, 2021 •

edited

Loading

elasticmachine commented Jan 18, 2021

alexwizp commented Jan 18, 2021

alexwizp commented Jan 18, 2021

nreese Jan 19, 2021

alexwizp Jan 19, 2021

nreese Jan 19, 2021 •

edited

Loading

alexwizp Jan 20, 2021 •

edited

Loading

nreese Jan 19, 2021

alexwizp Jan 19, 2021

nreese Jan 19, 2021

alexwizp commented Jan 20, 2021

thomasneirynck left a comment

alexwizp commented Jan 21, 2021

alexwizp commented Feb 2, 2021

alexwizp commented Feb 4, 2021

alexwizp commented Feb 4, 2021

alexwizp commented Feb 5, 2021

stratoula left a comment

nreese commented Feb 5, 2021

stratoula commented Feb 5, 2021

thomasneirynck commented Feb 5, 2021

alexwizp commented Feb 8, 2021

alexwizp commented Feb 8, 2021

stratoula commented Feb 8, 2021

thomasneirynck left a comment

stratoula left a comment

kibanamachine commented Feb 9, 2021

[Vega] user should be able to set a specific tilemap service using the mapStyle property #88440

[Vega] user should be able to set a specific tilemap service using the mapStyle property #88440

Conversation

alexwizp commented Jan 15, 2021 • edited Loading

Summary

For people who will check these changes:

elasticmachine commented Jan 18, 2021

alexwizp commented Jan 18, 2021

alexwizp commented Jan 18, 2021

nreese Jan 19, 2021

Choose a reason for hiding this comment

alexwizp Jan 19, 2021

Choose a reason for hiding this comment

nreese Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

alexwizp Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

nreese Jan 19, 2021

Choose a reason for hiding this comment

alexwizp Jan 19, 2021

Choose a reason for hiding this comment

nreese Jan 19, 2021

Choose a reason for hiding this comment

alexwizp commented Jan 20, 2021

thomasneirynck left a comment

Choose a reason for hiding this comment

alexwizp commented Jan 21, 2021

alexwizp commented Feb 2, 2021

alexwizp commented Feb 4, 2021

alexwizp commented Feb 4, 2021

alexwizp commented Feb 5, 2021

stratoula left a comment

Choose a reason for hiding this comment

nreese commented Feb 5, 2021

stratoula commented Feb 5, 2021

thomasneirynck commented Feb 5, 2021

alexwizp commented Feb 8, 2021

alexwizp commented Feb 8, 2021

stratoula commented Feb 8, 2021

thomasneirynck left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 9, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

alexwizp commented Jan 15, 2021 •

edited

Loading

nreese Jan 19, 2021 •

edited

Loading

alexwizp Jan 20, 2021 •

edited

Loading