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

[Maps] add Top term aggregation #57875

Merged
merged 40 commits into from
Feb 26, 2020
Merged

[Maps] add Top term aggregation #57875

merged 40 commits into from
Feb 26, 2020

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Feb 18, 2020

Closes #11223

This PR adds Terms aggregation to aggregation selection list. This allows users to create maps with clusters and joins that can be styled by top term.

This feature is required for blended layer. This will allow clusters in blended layer to have roll-up style when documents are styled by category.

Screen Shot 2020-02-18 at 8 04 34 AM

@nreese nreese added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation enhancement New value added to drive a business result v8.0.0 v7.7.0 labels Feb 18, 2020
@nreese nreese requested a review from a team as a code owner February 18, 2020 16:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@nreese nreese added release_note:enhancement and removed enhancement New value added to drive a business result labels Feb 18, 2020
@nreese nreese added the review label Feb 18, 2020
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So sweet getting these additional new features dialed in. It really matures the feature-set tremendously.

@@ -178,12 +164,23 @@ export class ESGeoGridSource extends AbstractESAggSource {
async getGeoJsonWithMeta(layerName, searchFilters, registerCancelCallback) {
const indexPattern = await this.getIndexPattern();
const searchSource = await this._makeSearchSource(searchFilters, 0);
const aggConfigs = new AggConfigs(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 getting rid of agg-configs.

Probably can remove corresponding mock as well:

cc @aaronjcaldwell I think we're only down to usage for bounds-agg after this.

import { AggConfigs } from 'ui/agg_types';
We could probably remove in similar fashion

@nreese
Copy link
Contributor Author

nreese commented Feb 24, 2020

@elasticmachine merge upstream

@thomasneirynck
Copy link
Contributor

thomasneirynck commented Feb 24, 2020

Thanks for investigating the composite agg.

I would be hesitant to replace the existing implementation.

  1. The reason for introducing composite-aggs is to support the top-terms nested aggregation for the fine-finest resolution of grid layers and to support top-terms in term-joining. Composite-aggs paginate through all the results, and are similar to _scroll in that respect. Paging through a result-set is generally not recommended for real-time user-requests, and more often applicable in offline workflows (e.g. reporting etc...). Using composite-agg would be a new paradigm for loading data from Elasticsearch in Maps (and Kibana).
  2. Composite-aggs are not needed to support top-terms in context of the blended-layer feature [Maps] Blended layer that switches between documents and clusters #57879 (which is what triggered this PR in the first place). For blended-layers, Maps will default to the "coarse"-resolution (makes more sense for "clusters"/labeling anyway).
  3. We have not had user-feedback in Maps yet, that the grid-layers or term-joining is hitting bucket-limit issues with the current-implementation for fine/finest.

I would only create a composite-aggs, if a top-terms is present in the metric-config. It seems like the most conservative choice. It would prevent us from pre-emptively fixing an issue that has not been raised, but still allow us to introduce the top-terms functionality more generically instead of limiting it to just using it in blended-layers.

Later, we can always still remove the default implementation if using composite-aggs did not introduce any unintended consequences.

@nreese
Copy link
Contributor Author

nreese commented Feb 24, 2020

I would only create a composite-aggs, if a top-terms is present in the metric-config. It seems like the most conservative choice.

I have added a separate path for when there are no term sub-aggregations.

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small cleanup suggestions. Can you create a follow-up ticket for the getMetrics issue?

@nreese
Copy link
Contributor Author

nreese commented Feb 26, 2020

Can you create a follow-up ticket for the getMetrics issue?

Created #58560

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small comment left-over. Thanks, great functionality. not only to make blended-layers work, but also as stand-alone!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@nreese nreese merged commit 3212754 into elastic:master Feb 26, 2020
nreese added a commit to nreese/kibana that referenced this pull request Feb 26, 2020
* [Maps] add Top term aggregation

* update pew-pew source to handle terms agg

* make helper function for pulling values from bucket

* update terms source

* better join labels

* categoricla meta

* remove unused constant

* remove unused changes

* remove unused constant METRIC_SCHEMA_CONFIG

* update jest expect

* fix auto complete suggestions for top term

* get category autocomplete working with style props from joins

* pluck categorical style meta with real field name

* mock MetricsEditor to fix jest test

* review feedback

* es_agg_utils.js to es_agg_utils.ts

* typing updates

* use composit agg to avoid search.buckets limit

* i18n update and functional test fix

* stop paging through results when request is aborted

* remove unused file

* do not use composite agg when no terms sub-aggregations

* clean up

* pass indexPattern to getValueAggsDsl

* review feedback

* more review feedback

* ts-ignore for untyped imports in tests

* more review feedback

* add bucket.hasOwnProperty check

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
nreese added a commit that referenced this pull request Feb 26, 2020
* [Maps] add Top term aggregation

* update pew-pew source to handle terms agg

* make helper function for pulling values from bucket

* update terms source

* better join labels

* categoricla meta

* remove unused constant

* remove unused changes

* remove unused constant METRIC_SCHEMA_CONFIG

* update jest expect

* fix auto complete suggestions for top term

* get category autocomplete working with style props from joins

* pluck categorical style meta with real field name

* mock MetricsEditor to fix jest test

* review feedback

* es_agg_utils.js to es_agg_utils.ts

* typing updates

* use composit agg to avoid search.buckets limit

* i18n update and functional test fix

* stop paging through results when request is aborted

* remove unused file

* do not use composite agg when no terms sub-aggregations

* clean up

* pass indexPattern to getValueAggsDsl

* review feedback

* more review feedback

* ts-ignore for untyped imports in tests

* more review feedback

* add bucket.hasOwnProperty check

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sub-buckets also in the tilemap.
5 participants