[AggConfigs] Add TopMetrics agg #125936

Dosant · 2022-02-17T14:20:45Z

Summary

Adds top_metrics agg to AggConfigs
close #124535

Needed for Lens - #102956

Questions

Q: Keep it enabled in visualize or hide it?

Since top_metrics could also be useful in visualize, I decided to keep it ON. But we can also turn it off similar to how we did to other recently implemented aggs like multi terms, rare terms, etc. Here are some issues I noticed with it:

Unpredictable result for multi-value fields Top Metrics should support multi-value fields like Top Hits does. elasticsearch#82281
Sorting terms agg doesn't work with top-metrics that produce non-numeric results. error: pipeline aggregations may not refer to non-numeric metrics collected by top_metrics

A: Keeping it enabled in visualize

Release notes

Add top metrics aggregation to AggConfigs, Expressions and Visualize

Dosant

I didn't update any viz editor related configs. Should we exclude top_metrics from all the configs or somehow adjust them properly so vis editors properly supported this new metric?

Dosant · 2022-02-17T14:21:44Z

src/plugins/data/common/search/aggs/param_types/field.ts

@@ -43,6 +43,7 @@ export class FieldParamType extends BaseParamType {

 this.filterFieldTypes = config.filterFieldTypes || '*';
 this.onlyAggregatable = config.onlyAggregatable !== false;
+ this.scriptable = config.scriptable !== false;


Strange... we had this in types and a default scripted = true, but it was never set from config.scriptable 🤔

flash1293

Looks pretty good, there is just one thing that might be relevant, added a comment about it.

test/interpreter_functional/test_suites/run_pipeline/esaggs_topmetrics.ts

elasticmachine · 2022-02-22T15:43:21Z

Pinging @elastic/kibana-app-services (Team:AppServicesSv)

gchaps · 2022-02-22T20:24:19Z

@Dosant Can you please add screenshots so we can review the UI copy?

Dosant · 2022-02-24T13:29:30Z

@elasticmachine merge upstream

Dosant · 2022-02-24T13:41:53Z

@gchaps, this adds a new aggregation to a service layer that later will be used internally in Lens.
There is an open question if we want to expose this aggregation in visualize as is. Currently, it is exposed and this is how the current copy looks:

Code: https:/elastic/kibana/pull/125936/files#diff-64e1ac64d660f12d49f984724a03d247559f137306135ee84fc3e4402953186cR32-R77
Example in UI:

gchaps

docslinks service looks good. A few comments on copy.

src/plugins/data/common/search/aggs/metrics/top_metrics_fn.ts

src/plugins/data/common/search/aggs/metrics/top_metrics.ts

src/plugins/data/common/search/aggs/metrics/top_metrics_fn.ts

flash1293

Code wise this LGTM.

FYI @ghudgins - this is adding "top metrics" as an option to agg based visualizations:

In general I like the feature it could be slightly confusing though because it's relatively similar to top hits. What do you think?

ghudgins · 2022-03-01T21:50:08Z

I think it's okay...the agg based visualizations always seem to map to the API directly so this to me doesn't really change the experience too negatively (despite raising a "when to use" question that probably exists already)

Dosant · 2022-03-02T09:59:18Z

@elasticmachine merge upstream

kibanamachine · 2022-03-02T09:59:30Z

expected head sha didn’t match current head ref.

ppisljar

code LGTM

Dosant · 2022-03-07T10:01:56Z

@elasticmachine merge upstream

kibana-ci · 2022-03-07T11:29:06Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`data`	532	534	+2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`data`	2811	2822	+11

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`visDefaultEditor`	142.4KB	142.5KB	+86.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	295.9KB	296.0KB	+63.0B
`data`	453.1KB	457.9KB	+4.8KB
total			+4.8KB

Unknown metric groups

API count

id	before	after	diff
`data`	3408	3419	+11

History

💚 Build #27063 succeeded 81bc3ff
💚 Build #26581 succeeded 712fd13
💔 Build #26507 failed 943eb8b
💚 Build #26183 succeeded 6aba14e
💚 Build #25522 succeeded 278813c
💚 Build #25474 succeeded 0604fd1

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

gchaps

LGTM

kibanamachine · 2022-03-09T15:16:19Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 125936 or prevent reminders by adding the backport:skip label.

Dosant commented Feb 17, 2022

View reviewed changes

Dosant requested a review from flash1293 February 17, 2022 14:23

flash1293 reviewed Feb 17, 2022

View reviewed changes

test/interpreter_functional/test_suites/run_pipeline/esaggs_topmetrics.ts Show resolved Hide resolved

Dosant changed the title ~~basic top metrics~~ [AggConfigs] Add TopMetrics agg Feb 22, 2022

Dosant added release_note:enhancement Team:AppServicesSv Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) v8.2.0 labels Feb 22, 2022

Dosant marked this pull request as ready for review February 22, 2022 15:43

Dosant requested review from a team as code owners February 22, 2022 15:43

Dosant requested a review from flash1293 February 22, 2022 15:43

KOTungseth added the ui-copy Review of UI copy with docs team is recommended label Feb 22, 2022

gchaps reviewed Feb 24, 2022

View reviewed changes

src/plugins/data/common/search/aggs/metrics/top_metrics_fn.ts Outdated Show resolved Hide resolved

src/plugins/data/common/search/aggs/metrics/top_metrics.ts Outdated Show resolved Hide resolved

src/plugins/data/common/search/aggs/metrics/top_metrics_fn.ts Outdated Show resolved Hide resolved

flash1293 approved these changes Mar 1, 2022

View reviewed changes

top metrics

81bc3ff

Dosant force-pushed the d/2022-02-16-top-metrics-agg branch from 36ac87e to 81bc3ff Compare March 2, 2022 10:43

ppisljar approved these changes Mar 2, 2022

View reviewed changes

Merge branch 'main' into d/2022-02-16-top-metrics-agg

bdd63c0

Dosant requested a review from gchaps March 7, 2022 14:09

gchaps approved these changes Mar 7, 2022

View reviewed changes

Dosant merged commit eca203c into elastic:main Mar 7, 2022

lucasfcosta pushed a commit to lucasfcosta/kibana that referenced this pull request Mar 8, 2022

[AggConfigs] Add TopMetrics agg (elastic#125936)

7c831b5

lucasfcosta pushed a commit to lucasfcosta/kibana that referenced this pull request Mar 8, 2022

[AggConfigs] Add TopMetrics agg (elastic#125936)

9fbdce1

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 9, 2022

Dosant added the backport:skip This commit does not require backporting label Mar 9, 2022

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AggConfigs] Add TopMetrics agg #125936

[AggConfigs] Add TopMetrics agg #125936

Dosant commented Feb 17, 2022 •

edited

Loading

Dosant left a comment

Dosant Feb 17, 2022

flash1293 left a comment

elasticmachine commented Feb 22, 2022

gchaps commented Feb 22, 2022

Dosant commented Feb 24, 2022

Dosant commented Feb 24, 2022

gchaps left a comment

flash1293 left a comment

ghudgins commented Mar 1, 2022

Dosant commented Mar 2, 2022

kibanamachine commented Mar 2, 2022

ppisljar left a comment

Dosant commented Mar 7, 2022

kibana-ci commented Mar 7, 2022

API count

gchaps left a comment

kibanamachine commented Mar 9, 2022

[AggConfigs] Add TopMetrics agg #125936

[AggConfigs] Add TopMetrics agg #125936

Conversation

Dosant commented Feb 17, 2022 • edited Loading

Summary

Questions

Q: Keep it enabled in visualize or hide it?

A: Keeping it enabled in visualize

Release notes

Dosant left a comment

Choose a reason for hiding this comment

Dosant Feb 17, 2022

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

elasticmachine commented Feb 22, 2022

gchaps commented Feb 22, 2022

Dosant commented Feb 24, 2022

Dosant commented Feb 24, 2022

gchaps left a comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

ghudgins commented Mar 1, 2022

Dosant commented Mar 2, 2022

kibanamachine commented Mar 2, 2022

ppisljar left a comment

Choose a reason for hiding this comment

Dosant commented Mar 7, 2022

kibana-ci commented Mar 7, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

gchaps left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 9, 2022

Dosant commented Feb 17, 2022 •

edited

Loading