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

Add referenced pipeline aggs to every level of query #31121

Merged
merged 3 commits into from Feb 21, 2019

Conversation

@flash1293
Copy link
Contributor

flash1293 commented Feb 14, 2019

Summary

Fixes #25974

When using sibling pipeline aggregations as metric and activating "Show metric at all levels", the resulting ES query didn't correctly add the referenced pipeline aggs to every level.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials

For maintainers

@elasticmachine

This comment has been minimized.

Copy link
Contributor

elasticmachine commented Feb 14, 2019

@flash1293 flash1293 changed the title 25974/data table bucket metrics Add referenced pipeline aggs to every level of query Feb 14, 2019
@flash1293

This comment has been minimized.

Copy link
Contributor Author

flash1293 commented Feb 14, 2019

Would it make sense to add a functional test for this?

@elasticmachine

This comment has been minimized.

Copy link
Contributor

elasticmachine commented Feb 14, 2019

@ppisljar

This comment has been minimized.

Copy link
Member

ppisljar commented Feb 15, 2019

unit test is perfect imo

@timroes timroes requested a review from lukeelmers Feb 19, 2019
@flash1293 flash1293 added the v7.2.0 label Feb 19, 2019
Copy link
Member

lukeelmers left a comment

Code LGTM, tested Chrome OSX and not seeing any issues 👍

Agreed unit test should suffice here; this is ultimately a bug in agg_configs so a functional test for table vis is probably just adding unnecessary overhead at this point.

vis.isHierarchical = _.constant(true);

const topLevelDsl = vis.aggs.toDsl(vis.isHierarchical())['2'];
expect(topLevelDsl.aggs).to.have.keys(['1', '1-bucket']);

This comment has been minimized.

Copy link
@lukeelmers

lukeelmers Feb 21, 2019

Member

nit: It might be nice to include an assertion for the correct buckets_path as well; something like:

expect(topLevelDsl.aggs['1'].avg_bucket).to.have.property('buckets_path', '1-bucket>_count');

I realize that isn't validating the precise problem you are fixing here, but for those looking at this test in the future, it could make it clearer how everything fits together & why we need to check that the parent aggs have been added at each level.

@@ -193,6 +193,13 @@ class AggConfigs extends IndexedArray {
if (subAggs && nestedMetrics) {
nestedMetrics.forEach(agg => {
subAggs[agg.config.id] = agg.dsl;
// if a nested metric agg has parent aggs, we have to add them to every level of the tree
// to make sure "bucket_path" references in the nested metric agg itself are still working

This comment has been minimized.

Copy link
@lukeelmers

lukeelmers Feb 21, 2019

Member

++ This comment is super helpful, thanks for adding.

@elasticmachine

This comment has been minimized.

Copy link
Contributor

elasticmachine commented Feb 21, 2019

Copy link
Member

ppisljar left a comment

LGTM! thanks!

flash1293 added 3 commits Feb 14, 2019
@flash1293 flash1293 force-pushed the flash1293:25974/data-table-bucket-metrics branch from 09a2a82 to fae9520 Feb 21, 2019
@elasticmachine

This comment has been minimized.

Copy link
Contributor

elasticmachine commented Feb 21, 2019

@flash1293 flash1293 merged commit 59c6c8f into elastic:master Feb 21, 2019
2 checks passed
2 checks passed
CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details
flash1293 added a commit to flash1293/kibana that referenced this pull request Feb 21, 2019
flash1293 added a commit to flash1293/kibana that referenced this pull request Feb 21, 2019
flash1293 added a commit to flash1293/kibana that referenced this pull request Feb 21, 2019
flash1293 added a commit that referenced this pull request Feb 22, 2019
flash1293 added a commit that referenced this pull request Feb 22, 2019
flash1293 added a commit that referenced this pull request Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.