Filter geohash_grid aggregation to map view box with collar #12806

nreese · 2017-07-12T17:41:00Z

Apply geo_bounding_box filter aggregation to narrow the subject area to the map view box with collar as requested in issue #8087

Feature can be toggled off with new controls placed on the buckets aggregation page

Remaining issues

courier.fetch getting called anytime the map fires dragend or events. Kibana map updates the uiState when these event handlers are fired. This PR adds mapCollar to the uiState. As a result, courier shouldQuery unintentionally fires just because the aggs are different.
Fit Control does not really work since the entire dataset is no longer loaded in the browser

thomasneirynck

oops, pressed submit review to quick.

thomasneirynck · 2017-07-12T22:17:28Z

src/core_plugins/tile_map/public/maps_visualization.js

@@ -87,7 +91,15 @@ export function MapsVisualizationProvider(serviceSettings, Notifier, getAppState
      this._kibanaMap.on('zoomchange', () => {
        precisionChange = (previousPrecision !== this._kibanaMap.getAutoPrecision());
        previousPrecision = this._kibanaMap.getAutoPrecision();
-        this.vis.aggs[1].params.precision = previousPrecision;
+        const agg = this._getGeoHashAgg();


this new guard is breaking these tests: https://github.com/thomasneirynck/kibana/blob/bf63cac183e54f64a054eb781ffba5708270710e/src/ui/public/agg_types/__tests__/buckets/_geo_hash.js#L52-L52

In that mock vis object in the test, will need to add mock aggregation.

thomasneirynck · 2017-07-12T22:41:43Z

src/ui/public/agg_types/controls/precision.html

+    <label>
+      Collar Scale <kbn-info info="Set the size of the geo_bounding_box filter applied to the map bounds. Small values could result in excessive fetches during map panning. Large values could result in trimmed results."></kbn-info>
+    </label>
+    <input type="number" class="form-control"


I'm not sure if we even need to make this a user-facing setting. I would go for a good default (1.5 is fine). If it proves to be a problem in the wild, let's add it in. So I would just remove this option altogether.

If you think this is important, I'd rework it a little:

ensure the UI control does not allow to select a value outside the the valid 1-5 range (the kui-framework has a number picker). use something like https://github.com/thomasneirynck/kibana/blob/b86f97ce0f83c50b8737c3cef75773f54d4df64a/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.html#L229-L229

read the default-value consitently from this._getGeohashAgg().type.params[2].default instead of duplicating its declaration in multiple locations.

thomasneirynck · 2017-07-12T22:43:47Z

src/core_plugins/tile_map/public/maps_visualization.js

@@ -77,6 +79,8 @@ export function MapsVisualizationProvider(serviceSettings, Notifier, getAppState
      options.center = centerFromUIState ? centerFromUIState : this.vis.type.visConfig.defaults.mapCenter;

      this._kibanaMap = new KibanaMap(containerElement, options);
+      this._lastFetchedMapCollar = scaleBounds(this._kibanaMap.getBounds(), this._getCollarScale());
+      uiState.set('mapCollar', this._lastFetchedMapCollar);
      this._kibanaMap.addDrawControl();
      this._kibanaMap.addFitControl();


hide the fit-control when _isFilteredByCollar

thomasneirynck · 2017-07-12T22:51:06Z

src/core_plugins/tile_map/public/kibana_map.js

@@ -300,7 +301,7 @@ export class KibanaMap extends EventEmitter {
    return zoomToPrecision(this._leafletMap.getZoom(), 12, this._leafletMap.getMaxZoom());
  }

-  getBounds() {
+  getBounds(isTrimmed = true) {


consider not having the default param and create different method called getTrimmedBounds

thomasneirynck

I looked at this only briefly, but this is pretty pretty cool!!

Some minor preliminary comments throughout.

We need to look into the following as well.
https://github.com/thomasneirynck/kibana/blob/b86f97ce0f83c50b8737c3cef75773f54d4df64a/src/core_plugins/tile_map/public/geohash_layer.js#L71-L71

The above hook is a carry-over from the depths of time in Kibana. It applies a client-side filter on the layer in leaflet on the collar, so not all svg elements are added to the screen. The downside is that we need to recreate these layer on each pan. This is kind of a hack. It may also be unncessary in later versions of Leaflet.

Now, by introducing a mode to do server-side filtering, this check is unncessary. So we need a way to propagate this to the geohash layer so it doesn't do this client-side filtering when server-side filtering is turned on.

thomasneirynck

Hey, pretty nice!

Most of the comments relate to the wiring of the bounds and collar determination. I think that will simplify some things, and get the Region Map working again.

Good find on #12919. Let's solve this separately.

thomasneirynck · 2017-07-17T18:19:54Z

src/ui/public/utils/geo_utils.js

+  return true;
+}
+
+export function scaleBounds(bounds, scale) {


remove scale parameter and define 1.5 constant here. you can remove the safeScale justification then.

thomasneirynck · 2017-07-17T18:29:38Z

src/ui/public/agg_types/controls/precision.html

+    <input type="checkbox"
+           name="useFilter"
+           ng-model="agg.params.useFilter">
+    Enable collar (geo_bounding_box filter)


I'd rename to something like:
'Only request data within the extent of the map'. Maybe with a little kbn-info button that refers to ES's geo_bounding_box filter.

I had to shorten the description to get it to fit in the options width constraints. I set it to Only request data around extent of map <kbn-info info="Apply geo_bounding_box filter aggregation to narrow the subject area to the map view box with collar."></kbn-info>

thomasneirynck · 2017-07-17T18:31:10Z

src/ui/public/agg_types/buckets/geo_hash.js

@@ -58,6 +63,11 @@ export function AggTypesBucketsGeoHashProvider(Private, config) {
        write: _.noop
      },
      {
+        name: 'useFilter',


I'd rename to isFilteredByCollar, to match all the other usage.

thomasneirynck · 2017-07-17T18:32:17Z

src/core_plugins/tile_map/public/kibana_map.js

@@ -517,7 +506,9 @@ export class KibanaMap extends EventEmitter {
  }

  _updateExtent() {
-    this._layers.forEach(layer => layer.updateExtent());
+    if (!this._isFilteredByCollar()) {


I'd remove this check from kibana_map, and check in geohash_layer instead. IMO, each layer should make the determination what to do when the extent is updated. It would also avoid the need to pass functions around to make this call.

thomasneirynck · 2017-07-17T18:57:57Z

src/core_plugins/tile_map/public/kibana_map.js

@@ -309,15 +308,8 @@ export class KibanaMap extends EventEmitter {

    const southEast = bounds.getSouthEast();
    const northWest = bounds.getNorthWest();
-    let southEastLng = southEast.lng;
-    if (southEastLng > 180) {
-      southEastLng -= 360;


why is this no longer necessary? this was to prevent date-line wrapping, ES chokes on it. I may be missing something though.

This trimming functionality does not return the correct bounding box when at a high zoom level. The returned value has a bottom_right that is to the left of top_left. In the original commit, this function was enhanced with a isTrimmed parameter so both cases could be satisfied and original consumers did not have to change their usage. Should I add the isTrimmed parameter back?

Personally not a fan of default params, since they hurt searchability in the code, especially in a language like JS. Would add a parallel method, getBoundsTrimmed, or getBoundsWithoutTrimming, whatever would require the least changes.

Anyways, it's your call ;) as long as we avoid regressing on #8946.

thomasneirynck · 2017-07-17T19:05:01Z

src/core_plugins/tile_map/public/maps_visualization.js

@@ -261,10 +272,47 @@ export function MapsVisualizationProvider(serviceSettings, Notifier, getAppState
      this.vis.updateState();
    }

-  }
+    async getGeohashBounds() {


Not sure about this.

Each layer should determine its bounds. We're throwing that away when creating this function that needs to be supplied to the FitControl.

I'd move this method to geohash_bounds, and modify fitToData so it can work with async results.

The function is now passed to the geohash_layer. I did this instead of migrating the code to geohash_layer because several dependencies are not available in geohash_layer, like Private and vis index pattern.

thomasneirynck · 2017-07-17T19:12:44Z

src/core_plugins/tile_map/public/maps_visualization.js

@@ -75,10 +79,13 @@ export function MapsVisualizationProvider(serviceSettings, Notifier, getAppState
      const centerFromUIState = uiState.get('mapCenter');
      options.zoom = !isNaN(zoomFromUiState) ? zoomFromUiState : this.vis.type.visConfig.defaults.mapZoom;
      options.center = centerFromUIState ? centerFromUIState : this.vis.type.visConfig.defaults.mapCenter;
+      options.isFilteredByCollar = this._isFilteredByCollar.bind(this);


I don't think we need this functional approach. Also, this breaks the Region Map, because this option isn't applied See other comments on moving this to geohash_layer for different approach.

thomasneirynck · 2017-07-17T19:13:26Z

src/core_plugins/tile_map/public/kibana_map.js

@@ -95,6 +95,7 @@ export class KibanaMap extends EventEmitter {
    this._layers = [];
    this._listeners = [];
    this._showTooltip = false;
+    this._isFilteredByCollar = options.isFilteredByCollar;


see other related comments.

thomasneirynck · 2017-07-19T16:26:17Z

jenkins, test this

thomasneirynck · 2017-07-19T16:34:42Z

CI failing with:

Running "licenses" task
Fatal error: Non-conforming licenses: 
  ua-parser-js
    version: 0.7.14
    all licenses: (GPL-2.0 OR MIT)
    invalid licenses: (GPL-2.0 OR MIT)
    path: node_modules/ua-parser-js

https://kibana-ci.elastic.co/job/elastic+kibana+pull-request+multijob-intake/9165/console

?!?!?

thomasneirynck · 2017-07-19T16:37:16Z

Rebasing with master should fix the CI error: cf. c3713a8

thomasneirynck

small thing still outstanding I think.

Collapsing the side bar increases the collar of the map. This may require a fetch of new data (similar with expanding the browser window). We can use the status.resize flag to detect this.

This also will need rebase for CI to work.

It would be nice to get in #12931 at the same time.

apart from that, lgtm! :)

ppisljar

really nice addition @nreese LGTM

thomasneirynck · 2017-07-20T20:32:17Z

looks good. CI is failing all PRs now, so let's just wait until done.

…zoom changed or map bounds are outside of collar

…server

…param from scaleBounds function

…added filter aggregation

nreese · 2017-07-21T03:10:45Z

jenkins, test this

tylersmalley · 2017-07-21T15:04:20Z

test/functional/apps/visualize/_tile_map.js

@@ -182,26 +202,28 @@ export default function ({ getService, getPageObjects }) {
       ** changed, then open the saved viz and check that it's back to the original data.
       */
      it('should save with zoom level and load, take screenshot', function () {


This test failed on master

nreese requested review from thomasneirynck and ppisljar July 12, 2017 17:41

thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:enhancement labels Jul 12, 2017

thomasneirynck approved these changes Jul 12, 2017

View reviewed changes

thomasneirynck self-requested a review July 12, 2017 22:58

thomasneirynck requested changes Jul 12, 2017

View reviewed changes

thomasneirynck reviewed Jul 17, 2017

View reviewed changes

thomasneirynck added the v6.0.0-beta1 label Jul 19, 2017

thomasneirynck reviewed Jul 19, 2017

View reviewed changes

nreese force-pushed the filter_geo_hash2 branch from cd3b0de to c54989e Compare July 19, 2017 16:59

ppisljar approved these changes Jul 20, 2017

View reviewed changes

nreese force-pushed the filter_geo_hash2 branch from c54989e to 9171cab Compare July 20, 2017 16:34

thomasneirynck approved these changes Jul 20, 2017

View reviewed changes

nreese added 12 commits July 20, 2017 18:23

filter geo hash agg - merged with vis refactor

5416bd2

get collar size from geohash aggregation

4f8c8d5

fix getBounds

b077117

add test for _buckets to handle single bucket case

073272d

use _getGeoHashAgg to update precision

15bc2cc

call reload when map bounds outside of map collar

88407f0

update fit control to use geo_bounds aggregation metric

07fe8da

remove geo_hash param collarScale

93cb54a

fix precision parameter test

99180a5

refactor _geo_hash.js test for readability

5ab61d0

do not trim map bounds

8742f23

only change aggregation state when data needs to be refected because …

75de435

…zoom changed or map bounds are outside of collar

nreese added 8 commits July 20, 2017 18:23

remove extra geo_utils.js file from tile_map plugin

17a76b3

only filter geoJson layer on client when results are not filtered on …

4059aac

…server

set up layer options before creating geoJson layer

c04367f

add updateExtent back

6bfe87f

add getUntrimmedBounds, clean up filter option verbage, remove scale …

a4009e2

…param from scaleBounds function

move check for server-side filtering for updateExtent into geohash_layer

cb27ed0

move fetchBounds to geohash_layer

58cc57f

fix _tile_map functional test, response data is different because of …

98cc386

…added filter aggregation

nreese force-pushed the filter_geo_hash2 branch from b0e49a0 to 98cc386 Compare July 21, 2017 00:24

nreese merged commit 72f6b8a into elastic:master Jul 21, 2017

tylersmalley reviewed Jul 21, 2017

View reviewed changes

thomasneirynck mentioned this pull request Jul 21, 2017

Fix flaky map test #13032

Closed

thomasneirynck mentioned this pull request Mar 15, 2018

Fit Data Bounds doesn't reveal all the data on the map when geocentroid is active #13054

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter geohash_grid aggregation to map view box with collar #12806

Filter geohash_grid aggregation to map view box with collar #12806

nreese commented Jul 12, 2017 •

edited

Loading

thomasneirynck left a comment •

edited

Loading

thomasneirynck Jul 12, 2017

thomasneirynck Jul 12, 2017

thomasneirynck Jul 12, 2017

thomasneirynck Jul 12, 2017

thomasneirynck left a comment

thomasneirynck left a comment

thomasneirynck Jul 17, 2017

thomasneirynck Jul 17, 2017

nreese Jul 19, 2017

thomasneirynck Jul 17, 2017

thomasneirynck Jul 17, 2017

thomasneirynck Jul 17, 2017

nreese Jul 17, 2017

thomasneirynck Jul 18, 2017

thomasneirynck Jul 17, 2017

nreese Jul 19, 2017

thomasneirynck Jul 17, 2017

thomasneirynck Jul 17, 2017

thomasneirynck commented Jul 19, 2017

thomasneirynck commented Jul 19, 2017

thomasneirynck commented Jul 19, 2017

thomasneirynck left a comment

ppisljar left a comment

thomasneirynck commented Jul 20, 2017

nreese commented Jul 21, 2017

tylersmalley Jul 21, 2017

Filter geohash_grid aggregation to map view box with collar #12806

Filter geohash_grid aggregation to map view box with collar #12806

Conversation

nreese commented Jul 12, 2017 • edited Loading

Remaining issues

thomasneirynck left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck left a comment

Choose a reason for hiding this comment

thomasneirynck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck commented Jul 19, 2017

thomasneirynck commented Jul 19, 2017

thomasneirynck commented Jul 19, 2017

thomasneirynck left a comment

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

thomasneirynck commented Jul 20, 2017

nreese commented Jul 21, 2017

Choose a reason for hiding this comment

nreese commented Jul 12, 2017 •

edited

Loading

thomasneirynck left a comment •

edited

Loading