review feedback

elastic · Feb 25, 2020 · 7a0ffa8 · 7a0ffa8
1 parent 4de141a
commit 7a0ffa8
Show file tree

Hide file tree

Showing 4 changed files with 143 additions and 120 deletions.
diff --git a/...eo_grid_source/convert_to_geojson.test.js → ...eo_grid_source/convert_to_geojson.test.ts b/...eo_grid_source/convert_to_geojson.test.js → ...eo_grid_source/convert_to_geojson.test.ts
diff --git a/x-pack/legacy/plugins/maps/public/layers/sources/es_geo_grid_source/es_geo_grid_source.js b/x-pack/legacy/plugins/maps/public/layers/sources/es_geo_grid_source/es_geo_grid_source.js
@@ -164,126 +164,163 @@ export class ESGeoGridSource extends AbstractESAggSource {
     );
   }
 
-  async getGeoJsonWithMeta(layerName, searchFilters, registerCancelCallback, isRequestStillActive) {
-    const indexPattern = await this.getIndexPattern();
-    const searchSource = await this._makeSearchSource(searchFilters, 0);
-
-    let bucketsPerGrid = 1;
-    this.getMetricFields().forEach(metricField => {
-      if (metricField.getAggType() === AGG_TYPE.TERMS) {
-        // each terms aggregation increases the overall number of buckets per grid
-        bucketsPerGrid++;
-      }
-    });
-
-    const features = [];
-    // Do not use composite aggregation when there are no terms sub-aggregations
-    // see https://github.com/elastic/kibana/pull/57875#issuecomment-590515482 for explanation on using separate code paths
-    if (bucketsPerGrid === 1) {
-      searchSource.setField('aggs', {
-        gridSplit: {
-          geotile_grid: {
-            field: this._descriptor.geoField,
-            precision: searchFilters.geogridPrecision,
-          },
-          aggs: {
-            gridCentroid: {
-              geo_centroid: {
-                field: this._descriptor.geoField,
+  async compositeAggRequest({
+    searchSource,
+    indexPattern,
+    precision,
+    layerName,
+    registerCancelCallback,
+    bucketsPerGrid,
+    isRequestStillActive,
+  }) {
+    const gridsPerRequest = Math.floor(DEFAULT_MAX_BUCKETS_LIMIT / bucketsPerGrid);
+    const aggs = {
+      compositeSplit: {
+        composite: {
+          size: gridsPerRequest,
+          sources: [
+            {
+              gridSplit: {
+                geotile_grid: {
+                  field: this._descriptor.geoField,
+                  precision,
+                },
               },
             },
-            ...this.getValueAggsDsl(indexPattern),
+          ],
+        },
+        aggs: {
+          gridCentroid: {
+            geo_centroid: {
+              field: this._descriptor.geoField,
+            },
           },
+          ...this.getValueAggsDsl(indexPattern),
         },
-      });
+      },
+    };
+
+    const features = [];
+    let requestCount = 0;
+    let afterKey = null;
+    while (true) {
+      if (!isRequestStillActive()) {
+        // Stop paging through results if request is obsolete
+        throw new DataRequestAbortError();
+      }
+
+      requestCount++;
+
+      // circuit breaker to ensure reasonable number of requests
+      if (requestCount > 5) {
+        throw new Error(
+          i18n.translate('xpack.maps.source.esGrid.compositePaginationErrorMessage', {
+            defaultMessage: `{layerName} is causing too many requests. Reduce "Grid resolution" and/or reduce the number of top term "Metrics".`,
+            values: { layerName },
+          })
+        );
+      }
 
+      if (afterKey) {
+        aggs.compositeSplit.composite.after = afterKey;
+      }
+      searchSource.setField('aggs', aggs);
+      const requestId = afterKey ? `${this.getId()} afterKey ${afterKey.geoSplit}` : this.getId();
       const esResponse = await this._runEsQuery({
-        requestId: this.getId(),
-        requestName: layerName,
+        requestId,
+        requestName: `${layerName} (${requestCount})`,
         searchSource,
         registerCancelCallback,
-        requestDescription: i18n.translate('xpack.maps.source.esGrid.inspectorDescription', {
-          defaultMessage: 'Elasticsearch geo grid aggregation request',
-        }),
+        requestDescription: i18n.translate(
+          'xpack.maps.source.esGrid.compositeInspectorDescription',
+          {
+            defaultMessage: 'Elasticsearch geo grid aggregation request: {requestId}',
+            values: { requestId },
+          }
+        ),
       });
 
-      features.push(...convertRegularRespToGeoJson(esResponse, this._descriptor.requestType));
-    } else {
-      const gridsPerRequest = Math.floor(DEFAULT_MAX_BUCKETS_LIMIT / bucketsPerGrid);
-      const aggs = {
-        compositeSplit: {
-          composite: {
-            size: gridsPerRequest,
-            sources: [
-              {
-                gridSplit: {
-                  geotile_grid: {
-                    field: this._descriptor.geoField,
-                    precision: searchFilters.geogridPrecision,
-                  },
-                },
-              },
-            ],
-          },
-          aggs: {
-            gridCentroid: {
-              geo_centroid: {
-                field: this._descriptor.geoField,
-              },
+      features.push(...convertCompositeRespToGeoJson(esResponse, this._descriptor.requestType));
+
+      afterKey = esResponse.aggregations.compositeSplit.after_key;
+      if (esResponse.aggregations.compositeSplit.buckets.length < gridsPerRequest) {
+        // Finished because request did not get full resultset back
+        break;
+      }
+    }
+
+    return features;
+  }
+
+  // Do not use composite aggregation when there are no terms sub-aggregations
+  // see https://github.com/elastic/kibana/pull/57875#issuecomment-590515482 for explanation on using separate code paths
+  async nonCompositeAggRequest({
+    searchSource,
+    indexPattern,
+    precision,
+    layerName,
+    registerCancelCallback,
+  }) {
+    searchSource.setField('aggs', {
+      gridSplit: {
+        geotile_grid: {
+          field: this._descriptor.geoField,
+          precision,
+        },
+        aggs: {
+          gridCentroid: {
+            geo_centroid: {
+              field: this._descriptor.geoField,
             },
-            ...this.getValueAggsDsl(indexPattern),
           },
+          ...this.getValueAggsDsl(indexPattern),
         },
-      };
-
-      let requestCount = 0;
-      let afterKey = null;
-      while (true) {
-        if (!isRequestStillActive()) {
-          // Stop paging through results if request is obsolete
-          throw new DataRequestAbortError();
-        }
-
-        requestCount++;
-
-        // circuit breaker to ensure reasonable number of requests
-        if (requestCount > 5) {
-          throw new Error(
-            i18n.translate('xpack.maps.source.esGrid.compositePaginationErrorMessage', {
-              defaultMessage: `{layerName} is causing too many requests. Reduce "Grid resolution" and/or reduce the number of top term "Metrics".`,
-              values: { layerName },
-            })
-          );
-        }
-
-        if (afterKey) {
-          aggs.compositeSplit.composite.after = afterKey;
-        }
-        searchSource.setField('aggs', aggs);
-        const requestId = afterKey ? `${this.getId()} afterKey ${afterKey.geoSplit}` : this.getId();
-        const esResponse = await this._runEsQuery({
-          requestId,
-          requestName: `${layerName} (${requestCount})`,
-          searchSource,
-          registerCancelCallback,
-          requestDescription: i18n.translate(
-            'xpack.maps.source.esGrid.compositeInspectorDescription',
-            {
-              defaultMessage: 'Elasticsearch geo grid aggregation request: {requestId}',
-              values: { requestId },
-            }
-          ),
-        });
-
-        features.push(...convertCompositeRespToGeoJson(esResponse, this._descriptor.requestType));
-
-        afterKey = esResponse.aggregations.compositeSplit.after_key;
-        if (esResponse.aggregations.compositeSplit.buckets.length < gridsPerRequest) {
-          // Finished because request did not get full resultset back
-          break;
-        }
+      },
+    });
+
+    const esResponse = await this._runEsQuery({
+      requestId: this.getId(),
+      requestName: layerName,
+      searchSource,
+      registerCancelCallback,
+      requestDescription: i18n.translate('xpack.maps.source.esGrid.inspectorDescription', {
+        defaultMessage: 'Elasticsearch geo grid aggregation request',
+      }),
+    });
+
+    return convertRegularRespToGeoJson(esResponse, this._descriptor.requestType);
+  }
+
+  async getGeoJsonWithMeta(layerName, searchFilters, registerCancelCallback, isRequestStillActive) {
+    const indexPattern = await this.getIndexPattern();
+    const searchSource = await this._makeSearchSource(searchFilters, 0);
+
+    let bucketsPerGrid = 1;
+    this.getMetricFields().forEach(metricField => {
+      if (metricField.getAggType() === AGG_TYPE.TERMS) {
+        // each terms aggregation increases the overall number of buckets per grid
+        bucketsPerGrid++;
       }
-    }
+    });
+
+    const features =
+      bucketsPerGrid === 1
+        ? await this.nonCompositeAggRequest({
+            searchSource,
+            indexPattern,
+            precision: searchFilters.geogridPrecision,
+            layerName,
+            registerCancelCallback,
+          })
+        : await this.compositeAggRequest({
+            searchSource,
+            indexPattern,
+            precision: searchFilters.geogridPrecision,
+            layerName,
+            registerCancelCallback,
+            bucketsPerGrid,
+            isRequestStillActive,
+          });
 
     return {
       data: {

diff --git a/x-pack/legacy/plugins/maps/public/layers/sources/es_geo_grid_source/geo_tile_utils.js b/x-pack/legacy/plugins/maps/public/layers/sources/es_geo_grid_source/geo_tile_utils.js
@@ -118,17 +118,3 @@ export function expandToTileBoundaries(extent, zoom) {
     maxLat: tileToLatitude(upperLeftY, tileCount),
   };
 }
-
-export function getTileCountInExtent(extent, zoom) {
-  const tileCount = getTileCount(zoom);
-
-  const upperLeftX = longitudeToTile(extent.minLon, tileCount);
-  const upperLeftY = latitudeToTile(Math.min(extent.maxLat, 90), tileCount);
-  const lowerRightX = longitudeToTile(extent.maxLon, tileCount);
-  const lowerRightY = latitudeToTile(Math.max(extent.minLat, -90), tileCount);
-
-  const tilesY = Math.abs(upperLeftY - lowerRightY);
-  const tilesX = Math.abs(upperLeftX - lowerRightX);
-
-  return tilesX * tilesY;
-}
diff --git a/...s_pew_pew_source/convert_to_lines.test.js → ...s_pew_pew_source/convert_to_lines.test.ts b/...s_pew_pew_source/convert_to_lines.test.js → ...s_pew_pew_source/convert_to_lines.test.ts