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

Filter geohash_grid aggregation to map view box with collar #8087

Closed
nreese opened this issue Aug 25, 2016 · 8 comments
Closed

Filter geohash_grid aggregation to map view box with collar #8087

nreese opened this issue Aug 25, 2016 · 8 comments
Labels
bug Fixes for quality problems that affect the customer experience Feature:Coordinate Map Feature:Visualizations Generic visualization features (in case no more specific feature label is available) PR sent

Comments

@nreese
Copy link
Contributor

nreese commented Aug 25, 2016

Currently the geohash_grid aggregation is requested at a global level. The client uses Leaflet to filter the response and only include buckets inside the map view box. On each moveend event, the vector layer is recreated from the aggregation response.

The current solution has two problems.

  • Trimmed buckets resulting in inaccurate visualizations and user confusion. The geohash_grid aggregation caps the number of buckets at 10,000 (This is a very reasonable number and I am not recommending that this be increased). Results are trimmed based on the volume of documents each bucket contains. This has resulted in dropped buckets that would have been visible within the map view box. Users get very confused (and loss faith in their tool-set) when they know that data exists yet that data is not visible.
  • Performance. Each moveend event triggers the recreation of the vector layer. This makes map scrolling a jolting experience. Performance is also affected by the client having to process unnecessary buckets. Proper profiling metrics need to be provided to determine if this causes a perceivable user experience degradation.

I would like to propose changing the current implementation to filter the geohash_grid aggregation call to the map view area plus a configurable collar size. movend events would only trigger an AJAX call when the current view box is outside the collar. Properly configuring the collar for the use-case should minimize the need to continuously request new aggregation results. One important aspect of the solution will be to allow the tile map visualization to request new data without refreshing all other visualizations in the dashboard.

@thomasneirynck thomasneirynck added the Feature:Vislib Vislib chart implementation label Aug 26, 2016
@thomasneirynck
Copy link
Contributor

I agree with your analysis of the current approach, and the impact it has on performance, and on causing non-intuitive results in certain cases due to trimming of the results.

One additional remark wrt.:

One important aspect of the solution will be to allow the tile map visualization to request new data without refreshing all other visualizations in the dashboard.

It is a core aspect of the visualizations that they represent the result of an aggregation query to ES, as defined by the Data form in the sidebar and any global filters. Many UI-patterns depend on it (e.g. opening a tabular view of the data, linking visualizations to saved searches, ...), as does the current implementation. We'll have to ensure that aspect continues to work, however this would be addressed.

@nreese
Copy link
Contributor Author

nreese commented Sep 2, 2016

I have been hacking together a solution and have hit a snag. How should the collar bounds be determined for the very first AJAX call?

When the first AJAX request is generated, the visualization has not been rendered yet so there is no way of knowing the map view bounds. Map state, consisting of center point and zoom level, are stored in the visualization under visState.aggs[1].params.mapZoom and visState.aggs[1].params.mapCenter. I was hoping that the collar bounds could be stored in a similar fashion. This does not work since the collar bounds are determined by the visualization container height and width - which is stored in each dashboard. Multiple dashboards could include the visualization resulting in multiple bounds.

Any ideas on how to generate the collar bounds before the leaflet map has been created and the map view bounds are unknown?

@thomasneirynck
Copy link
Contributor

Hi @nreese, my apologies for the late follow up.

Thanks for your efforts to look into this already. I would suggest you create a PR for this to show where you are. That would be easiest way to share thoughts and refer to code.

@thomasneirynck thomasneirynck added bug Fixes for quality problems that affect the customer experience P3 and removed Feature:Vislib Vislib chart implementation labels Oct 3, 2016
@thomasneirynck
Copy link
Contributor

see also #8522 for additional description and steps to reproduce

@thomasneirynck thomasneirynck self-assigned this Oct 4, 2016
@nreese
Copy link
Contributor Author

nreese commented Oct 5, 2016

Hi @thomasneirynck, not a problem on the delay - I have been busy doing other things.

I have implemented a solution in the enhanced tilemap plugin.

The solution was rather straight forward. I just overwrote the visState.aggs.toDsl function to nest the geohash_grid aggregation under a filter aggregation with the geo_bounding_box of the map collar. I was able to generate the map collar for the first request by decoupling the creation of the map with the drawing of the aggregation results.

The results in performance are dramatic. As you zoom further in, the number of buckets drop from the thousands to the hundreds and no results get trimmed.

@thomasneirynck
Copy link
Contributor

@nreese thanks for sharing, I'll check it out more. Perhaps this is an improvement we can bring back to the default map as well ...

@nreese
Copy link
Contributor Author

nreese commented Oct 6, 2016

I had some bugs in the original commit. Reinstall the plugin with the
latest before you checking it out.

On Wed, Oct 5, 2016 at 4:41 PM, Thomas Neirynck notifications@github.com
wrote:

@nreese https://github.com/nreese thanks for sharing, I'll check it out
more. Perhaps this is an improvement we can bring back to the default map
as well ...


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8087 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWzu6n0nBxeLDoAnHhxMfYUyxiYiIPKks5qxCehgaJpZM4JtNjM
.

@nreese
Copy link
Contributor Author

nreese commented Jul 21, 2017

merged into kibana 6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Coordinate Map Feature:Visualizations Generic visualization features (in case no more specific feature label is available) PR sent
Projects
None yet
Development

No branches or pull requests

4 participants