-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Region map visualization #10937
Region map visualization #10937
Conversation
f60d556
to
f597c30
Compare
src/ui/settings/defaults.js
Outdated
'visualization:vectormap:showWarnings': { | ||
value: true, | ||
description: 'Should the vector map show a warning when terms cannot be joined to a shape on the map.' | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not 100% sure of this.
Since we're mashing up 2 datasets, the join might not succeed (partially or completely). The odds of having one or more results not being present on the map can be high, especially when the data is not very clean. That's why I introduced an advanced setting to turn these warnings on/off.
import _ from 'lodash'; | ||
import AggConfigResult from 'ui/vis/agg_config_result'; | ||
import KibanaMap from 'ui/vis_maps/kibana_map'; | ||
import FilterBarFilterBarClickHandlerProvider from 'ui/filter_bar/filter_bar_click_handler'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you might not actually need to import this, it should be available on vis.listeners.click
|
||
|
||
|
||
function getValue(metricsAgg, bucket) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move on top ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved it up
}); | ||
|
||
this._loaded = false; | ||
$.ajax({//todo: replace with es6 fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup :)
this looks great! some small comments below:
|
thanks @ppisljar , I'll get started on your feedback.. |
@thomasneirynck finally had some time this morning to play around with vector map. Please see my comments below. I believe I mentioned some of these in the previous PR so they might already be on your radar.
Having some issues generating reports locally but will try vector maps there as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! Just a few comments, and one thing that can hopefully be removed.
I'm also unsure how I feel about calling it "choropleth" in code, but "vector map" to the end-user. I could see someone new to the project struggling to find the relevant code because of the mis-match in naming.
require('./_heatmap_chart'); | ||
require('./_point_series_options'); | ||
require('./_shared_item'); | ||
// require('./_chart_types'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to revert this before committing.
test/functional/index.js
Outdated
'intern/dojo/node!./apps/xpack', | ||
'intern/dojo/node!./apps/discover', | ||
'intern/dojo/node!./apps/management', | ||
// 'intern/dojo/node!./apps/xpack', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, we don't want to commit this to master
@@ -25,6 +25,7 @@ bdd.describe('visualize app', function describeIndexTests() { | |||
'Data Table', | |||
'Metric', | |||
'Tile Map', | |||
'Vector Map', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are quite a few merge conflicts for the tests because of #10910
function getValue(metricsAgg, bucket) { | ||
let size = metricsAgg.getValue(bucket); | ||
if (typeof size !== 'number' || isNaN(size)) { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been unable to find this code-path being hit, or other visualizations (besides tagcloud) doing something similar.
2353864
to
8e3e227
Compare
rebasing against master, incorporating test-suite changes |
After more discussion, we want to change how we load the example json files. Instead of using Kibana to serve those, we should host them from a central repo. Apart from that change, users will continue to be able to bring in custom data layers with the same mechanism we have now. |
67dd87f
to
fe1f906
Compare
removing default data config. all custom data should now be configured in the .yml. e.g.
Default layers will now be loaded from remote catalogue service (forthcoming) |
with 6b41e66, we're testing against a new metadata-service. This is just a POC now, the structure of these documents, and the endpoints are not final yet. |
b19925d
to
1bf88c1
Compare
24cbb2a
to
a45b6b7
Compare
…ating vector-layers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally, this seems to work perfectly! Just a few comments.
Also, the optimize/.empty
file looks like it was somehow deleted with this PR.
q
Outdated
@@ -0,0 +1,94 @@ | |||
[1mdiff --git a/src/core_plugins/region_map/public/region_map_controller.js b/src/core_plugins/region_map/public/region_map_controller.js[m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this file was accidentally checked in.
@@ -6,13 +8,20 @@ export function injectVars(server) { | |||
//keeping this logic for backward compatibilty. | |||
const configuredUrl = server.config().get('tilemap.url'); | |||
const isOverridden = typeof configuredUrl === 'string' && configuredUrl !== ''; | |||
const tilemapConfig = serverConfig.get('tilemap'); | |||
const tilemapConfig = _.cloneDeep(serverConfig.get('tilemap')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your reasoning for cloning these settings? It's my understanding that these injected_vars
are serialized into an HTMLElement in the initial response and then deserialized client-side, so it seems that this might be unnecessary.
@@ -6,13 +8,20 @@ export function injectVars(server) { | |||
//keeping this logic for backward compatibilty. | |||
const configuredUrl = server.config().get('tilemap.url'); | |||
const isOverridden = typeof configuredUrl === 'string' && configuredUrl !== ''; | |||
const tilemapConfig = serverConfig.get('tilemap'); | |||
const tilemapConfig = _.cloneDeep(serverConfig.get('tilemap')); | |||
const vectormapsConfig = _.cloneDeep(serverConfig.get('regionmap')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A remnant of the vectormaps naming, should be regionmapsConfig.
const mapConfig = _.cloneDeep(serverConfig.get('map')); | ||
|
||
|
||
vectormapsConfig.layers = (vectormapsConfig.layers) ? vectormapsConfig.layers : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, I've seen us using the .default()
method of Joi to set defaults for the config schema, so we could do something like the following in the 'src/server/config/schema.js' to do the equivalent:
regionmap: Joi.object({
layers: Joi.array().items(Joi.object({
url: Joi.string(),
type: Joi.string(),
name: Joi.string(),
fields: Joi.array().items(Joi.object({
name: Joi.string(),
description: Joi.string()
}))
})).default([])
}).default()
The .default([])
portion of above should always default the layers to an empty array if they aren't specified.
} | ||
|
||
const { min, max } = getMinMax(data); | ||
data = data.slice(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing data.slice()
here and then doing data.splice(lastIndex, 1)
below inside of the getLeafletStyleFunction
is rather confusing. We're using getLeafletStyleFunction
to mutate data
that is also used by getMismatches
, so the order that these functions are called becomes significant. Was this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that order is significant. the getMisMatches is just for the client to be able to read out the errors of the inner-join (ie. the style function).
const TemplateVisType = Private(TemplateVisTypeProvider); | ||
const Schemas = Private(VisSchemasProvider); | ||
|
||
const vectorLayers = vectormapsConfig.layers.slice(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only be being called once for the "client-side app" when it's registering all the visualization types, what are we trying to prevent with the slice()
? I know before that we were having layers duplicated when creating multiple maps in a row, was this meant to address this?
link: function ($scope) { | ||
|
||
|
||
console.log('calling the linker function...'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log
should be removed before merging.
src/server/config/schema.js
Outdated
@@ -161,6 +161,13 @@ module.exports = () => Joi.object({ | |||
status: Joi.object({ | |||
allowAnonymous: Joi.boolean().default(false) | |||
}).default(), | |||
map: Joi.object({ | |||
manifestServiceUrl: Joi.when('$dev', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the new map.manifesetServiceUrl
is used for both the tiles and the geo layers, does that mean we should be deprecating the tilemap
config section?
src/server/config/schema.js
Outdated
map: Joi.object({ | ||
manifestServiceUrl: Joi.when('$dev', { | ||
is: true, | ||
then: Joi.string().default('https://geo.elastic.co/v1/manifest'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dev and Non-Dev appear to be the same, was this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, for now this needs to be the same. Infra has only setup a single production server. this will likely change in the weeks to come.
@@ -0,0 +1,835 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file's been moved, I think you should just be able to delete it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rebase snafu, thanks.
sibling pipeline aggs should be allowed (no reason why they should be disabled i think) sample use case:
i would get representation of windows usage around the world. |
@ppisljar thanks, I added them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
wrt. tilemap-config. still required, since people can override the url still to plug in a custom url. perhaps we can deprecate this configuration in 6.0, but keeping it for now. |
src/server/config/schema.js
Outdated
then: Joi.string().default('https://geo.elastic.co/v1/manifest'), | ||
otherwise: Joi.string().default('https://geo.elastic.co/v1/manifest') | ||
}) | ||
}).default(), | ||
tilemap: Joi.object({ | ||
manifestServiceUrl: Joi.when('$dev', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, sorry, mistake on my part. gone!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
elastic#10937 seems to have brought this file back, likely as part of a bad merge resolution
#10937 seems to have brought this file back, likely as part of a bad merge resolution
#10937 seems to have brought this file back, likely as part of a bad merge resolution
Summary: Kibana now has the Region Map Visualization. These are thematic maps in which boundary vector shapes are colored using a gradient, with higher intensity colors indicating larger values and lower intensity colors indicating smaller values. These are also known as choropleth maps. In order to color these layers, users specify a terms aggregation that matches a field in the vector layer. Kibana offers two vector layers by default; one for countries of the world and one for US Shapes. Users can also bring in their own vector layers by configuring the Kibana-configuration file to point to any GeoJson file that is hosted on a CORS-enabled server.
[EDIT]: see last comment for update wrt. data loading: #10937 (comment)
Feedback (round 2):
Feedback:
only one metric is possible, it should be expandedagreed to do this in separate PR since same issue occurs in other viz as wellshape field + aggregation type (term) should be pre selected as its the only possibilitysee abovedoes geocentroid metric make sense ?no, since we are not querying for geo-fields in ESadd labeling on map(decided to skip this until long term outlook of map-component is worked out).Todos: