Geo centroid metric aggregation type added #9676
Conversation
|
Can one of the admins verify this patch? |
epixa
added
the
Feature:Visualizations
|
jenkins, test this |
|
Is the mock data correct for geohash/_geo_json.js? I'm looking at it and wondering why the geometry point of the feature is an exact match of the center property. According to the comment here https://github.com/elastic/kibana/pull/9676/files#diff-c7c9f237e673ff486654f6cc6caa89f6R44 the geoJson coordinates should be reversed since it uses Lng Lat. Am I correct and can I update the mock data to reflect this? Nevermind, I'm actually going to update the test for _dataToHeatArray and leave the mock data alone. The test was incorrect in that it assumed the return array from the _dataToHeatArray method is returning data in LngLat. |
|
@epixa How do I initiate a Jenkins build again? |
|
@JacobBrandt Unfortunately, only Elastic org members can kick off jenkins builds since it results in remotely executed code on our servers. You can always run the tests locally though. Here are is the unit test command that jenkins uses: https://github.com/elastic/kibana/blob/master/tasks/jenkins.js#L24-L34 |
|
jenkins, test this |
|
/cc @thomasneirynck |
|
In case anyone was wondering how it affects the visualization. Here is the difference it makes in the heatmap.
|
|
Hi @JacobBrandt, thanks for the PR. I'll take a look as soon as I can, sometime later this week. |
There was a problem hiding this comment.
This is a valuable improvement and we should move forward adding this to Kibana.
Due to centroids being a metric in ES, it makes the UI a little odd (e.g. see also #5464 (comment)). I agree that is not entirely intuitive, especially for users not familiar with ES.
That said, I think your approach, having this as an additional metric aggregation, is an appropriate way forward. It is consistent with the existing way Kibana&ES are wired in the Visualize-app. Another advantage is too that this doesn't impact backwards compatibility.
But let's rope in a 2nd opinion on this.
As for the rest, this will need testing. I'd suggest expanding on the existing agg_response/__tests__/geo_json tests and adding a new one in agg_types/__tests__/ for the new geo_centroid aggregation.
|
@ppisljar Can you weigh in on this? thx! |
|
@thomasneirynck I expanded the geo_json agg_response test as suggested. What do you expect a geo_centroid agg_type test to actually test? It doesn't make a custom label nor does it have default params it passes to Elasticsearch. |
|
Disregard below... I had ported the changes into kibana 4.6 and changed the property.center from centerLatLng to Point. When property.center remains centerLatLng then everything works great. shaded circles are no longer the same diameter when using geo centroid metric. Looks like the logic in ShadedCircleMarker _geohashMinDistance function uses the center to determine the radius. This function should just take the smaller of the east and north sides of the grid and not bother looking at the center. |
|
@nreese Yes you need to be mindful of what center means if trying to put this in another Kibana version. Center is the center of the geohash. The heatmap visualization assumed the same thing that a Point feature's location was the same as the center. This is why I changed it in this pull request to be the geometry because a Point features location is always in the geometry given to the feature. Not sure why center was used in heatmap in the first place. Also of note is that "Center" is still used for the tooltip displayed for the feature. This is another reason why I suggested in this PR #9819 to change the tooltip to not say Center with LatLng being displayed as an object and instead specifically call out what the Latitude and Longitude of the feature is. Hopefully you won't need to have a modified version of Kibana to make use of this feature. This is something that should really be added for any Kibana version that supported Elasticsearch 2.1 since geo centroid was an aggregation going back that far. |
|
@JacobBrandt thanks for the test. As for skipping |
|
lets say i have two geo coordinate fields customerLocation and officeLocation for example.
In opposite case we should not allow that. And if that is the case then Geo Centroid as a metric doesn't make much sense, seems like a checkbox on the geo coordinate bucket would do? another thing, can we even use geo centroid without geohash ? if not my feeling it should be a setting on geo coordinate bucket is even stronger. |
|
@ppisljar Yes you can use geo centroid without geo hash. |
|
Hi @JacobBrandt, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile? |
|
@karmi I no longer have that e-mail. I'll just re-sign the CLA. |
|
@ppisljar https://www.elastic.co/guide/en/elasticsearch/reference/2.1/search-aggregations-metrics-geocentroid-aggregation.html Some good examples of using geo centroid without geo hash. |
JacobBrandt
force-pushed
the
geo_centroid_metric_master
branch
from
66a2b32 to
f060147
Compare
|
thx @ppisljar! @JacobBrandt fyi, we'll be demoing this PR this Friday to some more UX-minded people in Kibana. They might have some more feedback on e.g. @ppisljar suggestion to use checkbox iso metric-UI. Apart from that, functionality-wise, I think it's right on track, thanks. Please stay tuned! |
|
@thomasneirynck OK. I originally made it as a checkbox on the GeoHash bucket but realized that limits the usefulness of this kind of aggregation against other bucket types. Keep me posted. |
|
Why not simply always use the geo_centroid aggregation when using the map. At worst bury the option to turn it off somewhere, and even then I don't see the use of turning it off. The only reason it wasn't designed like this originally is that geo_centroid didn't exist. It was one of the first things we asked for after using the map for the first time. |
|
@rashidkpc The reason to not always use geo_centroid is aesthetics. It's why this issue #8001 is also not solved. People pushed back on the fix because the circles overlapped. With geo_centroid always on you will have overlapping circles which again others are against. As for allowing this new feature to be on/off this already does that. |
|
Meh, overlapping circles seems correct. Geohash grids aren't square. |
|
@rashidkpc Yea I agree, overlapping circles aren't a concern of mine. But others voiced their concern so much that it stopped development of that other fix. I don't want that to happen to my changes. |
|
@JacobBrandt we just finished talking about this internally (meeting got pushed back from Friday). The consensus seems to be more in line with what's @rashidkpc's saying. We'd make positioning on the centroid the new default from new releases onward, but won't use the Metric UI. Upgrades from earlier Kibana-versions will have to explicitly opt-in if they want the centroid positioning. This will be somewhat of a departure of how in Kibana users select Metrics aggregations (instead of using the metrics-UI, it will basically be an options-flag). This will give new users the better behavior out of the box (without requiring extra configuration), but also preserve backwards compatibility. We'd also fold this feature into some of the internal map work we're doing. I'd propose to close this PR, and I'll merge in your commits into our internal work. That way, we get your contribution here as part of Kibana's commit history all the same. Thanks a lot! |
|
@thomasneirynck I think you missed the point of this feature. It was NOT to improve the map. I just showed how it COULD be used to improve the existing map. Without this metric aggregation you are still limiting people of the full capabilities of Elasticsearch. I don't understand why you're doing that. |
|
@JacobBrandt I think we're in agreement. We certainly want this feature part of Kibana. We'd just prefer not using the Metrics UI for people to enable it. When it is added to Kibana, geocentroid positioning will be the default standard behavior. |
|
@thomasneirynck That's fine, let's take out the addition of this metric type to your visualization map. But why remove the metric type all together? Or am I misunderstanding and you are keeping it? |
|
Basically what I'm getting at is how am I going to allow the user to pick the geo coordinate (I have several geo coordinates in my index) to use for geo centroid if you remove this new aggregation type? My visualization I'm working on has only one way to pick a geo coordinate and that's through the use of this new metric type I added here. There is no way for the user to select Geo Hash as a bucket because I don't want my user doing Geo Hash bucket aggregations in this visualization. So ease my concerns by telling me that you're at least keeping the new metric type and I don't have to carry this feature along with my version of Kibana. |
|
@JacobBrandt I apologize for my miscommunication It's not our intention to remove the metric-type for the geocentroid. We will add it to Kibana. But instead of using the Metric-UI in the Data-panel (similar to how e.g. you'd add an average/sum/count... aggregation), we want it to be the default new Kibana behavior. The reason for this is that people may not be familiar with Elasticsearch Metrics. The idea now is to move this as an option to the Options-panel instead. It would be enabled by default, so new users get the benefit without extra configuration. For people upgrading (e.g. a saved dashboard from older versions), they would have to manually enable it. This is mainly to preserve backwards compatibility of the look of the map. As for moving this feature to the larger map PR, instead of keeping it in a separate PR: this is mainly because the behavior outlined above will require some more involved refactor work, since it will be the first time that we would allow users to modify the ES-query without using the UI in the "Data" panel. This work then makes sense to do as part of that larger refactor effort. |
|
@thomasneirynck Yea I understand your concerns for your map visualization and wanting to keep it more user friendly and backwards compatible. So I completely agree with not not wanting your map to use this metric type like I demonstrated in this PR. I just wanted to make sure that I can still use the aggregation type through the Metric-UI in my visualization that has no concept of default Kibana behavior for an enabled/disabled geo centroid. It's the only point that they can pick. It, sounds like I'll be able to do that still. |
|
@JacobBrandt yes, you'll still be able to do that. Sorry for all the confusion and thanks again for all your efforts, we really appreciate them very much. |
|
@thomasneirynck Cool, no worries. Could we update #5464 with some blurb about the proposed changes you guys are making for that feature? Thanks. |
|
@JacobBrandt just to confirm i am getting this correct .... you want this to be a metric which you can also use on other visualizations (not just map). how would you use this on non-map vis ? can you give us an example ? regarding multiple fields .... could you explain one example where calculating geo_hash on field1 and geo_centroid on field2 makes sense ? |
|
@ppisljar Yes, you are almost correct. I still will use it on a map visualization though. I want to be able to use this metric for other visualizations and not just one's provided by default through Kibana. Having it still as a metric type would allow externally developed Kibana plugins to reuse this aggregation for their visualizations. I prototyped this with my own map based visualization (although mine is not using Leaflet). I think we both agree that calculating a geo_centroid and not displaying it on a map doesn't make a whole lot of sense. In my case I don't need to calculate both a geo_hash in field 1 and then a geo_centroid on field 2. I need to be able to calculate only the geo_centroid point without a geo_hash bucket. So that means the user needs a way to choose the geo coordinate from the index some how without using the geo_hash bucket (enter the geo_centroid metric agg type). As far as an example of this, the one provided by elastic is a good start to doing geo_centroid aggregations on different kind of buckets other than geo_hash. Here's the link for that: https://www.elastic.co/guide/en/elasticsearch/reference/2.1/search-aggregations-metrics-geocentroid-aggregation.html. Hopefully I'm explaining this better. I realize I may not have from the start. |
fixes #5464