Sum, Avg, Min and Max bucket pipeline aggregation #10070

ppisljar · 2017-01-25T20:36:39Z

adds bucket_sum , bucket_avg, bucket_min and bucket_max sibling pipeline aggregations

stacey-gammon · 2017-02-15T15:44:16Z

src/ui/public/agg_types/metrics/bucket_avg.js

+import { makeNestedLabel } from './lib/make_nested_label';
+import SiblingPipelineAggHelperProvider from './lib/sibling_pipeline_agg_helper';
+
+export default function AggTypeMetricDerivativeProvider(Private) {


These names should match how they are being imported. E.g. AggTypesMetricsBucketAvgProvider. This is why I like named exports, it forces you to use consistent naming. :) Here and other bucket_* files

stacey-gammon · 2017-02-15T15:46:04Z

src/ui/public/agg_types/metrics/lib/sibling_pipeline_agg_controller.js

+  return function ($scope) {
+
+    $scope.aggType = type;
+    //$scope.aggParam = $scope.agg.params[type];


commented out line (and below)

stacey-gammon · 2017-02-15T15:47:38Z

src/ui/public/agg_types/metrics/bucket_avg.js

+    name: 'avg_bucket',
+    title: 'Average Bucket',
+    makeLabel: agg => makeNestedLabel(agg, 'overall average'),
+    group: 'Pipeline Aggregations',


The string "Pipeline Aggregations" is repeated in many places. What about creating a metrics_constants file to put it in? Then if we want to change the string we only have to do it in one spot.

stacey-gammon · 2017-02-15T16:03:37Z

src/ui/public/agg_types/metrics/lib/sibling_pipeline_agg_controller.js

+    $scope.aggType = type;
+    //$scope.aggParam = $scope.agg.params[type];
+    $scope.aggTitle = type === 'customMetric' ? 'Metric' : 'Bucket';
+    $scope.aggGroup = type === 'customMetric' ? 'metrics' : 'buckets';


Looks like there is aggGroup and also agg.group and they mean different things. I'm not sure I have a suggestion on the naming, but it'd be nice if they could be distinguished better.

Almost seems like maybe typeName would be more fitting? I think I tracked this down to being used here:
aggTypes.byType[$scope.groupName];

Though that might be too far reaching to change it in this PR since that name already exists and group is the new name.

I think something else that would be worthwhile as a separate PR would be moving metrics and buckets to constants too. That would be easier to understand the importance of these strings and where else they are being referenced. Then you could do something like:

export const AggTypes = { METRICS: 'metrics', BUCKETS: 'buckets' }

Food for thought mostly, I understand if you prefer to leave as is for lack of a better alternative at this point.

i renamed agg.group to agg.subtype

stacey-gammon · 2017-02-15T16:10:12Z

src/ui/public/agg_types/metrics/lib/sibling_pipeline_agg_controller.js

+    $scope.aggGroup = type === 'customMetric' ? 'metrics' : 'buckets';
+    $scope.safeMakeLabel = safeMakeLabel;
+
+    $scope.rejectAgg = function (agg) {


Is this used at all? I'm a little confused at the interaction and differences between rejectAgg and the aggFilter. Since the aggFilter already contains all these, if I select for example Top Hits, I don't see any of the sibling drop downs.

you are right, this is not used here great find!... to much copy pasting from parent pipeline agg :)

stacey-gammon · 2017-02-15T16:13:44Z

Ran into an error using Sum Bucket and Terms agg:

ppisljar · 2017-02-16T09:15:50Z

@stacey-gammon the reason for error was sorting on bucket metric in your terms bucket. i updated it so its not allowed anymore. i also addressed your other comments. thanks a lot for your super fast response !

tbragin · 2017-02-21T03:11:41Z

@ppisljar A couple of quick comments:

I do love the dropdown calling out aggregation types, nice call @alexfrancoeur! However, instead of "Basic Aggregations" I think we should call them what ES documentation calls them, which is "Metrics Aggregations"

I also think we should order bucket aggregations the same as metrics aggregations, with "Average" preceding "Sum".
It'd be nice if, once fully configured, switching between Sum, Average, Min, and Max Bucket Aggregations didn't wipe out the configuration below it. You'll see that Time Series Metric Visualizations #9725 allows to switch between all these bucket aggs w/o user having to rebuild their config.
I got this error when selecting the following option

You kind of do have to know what you're doing with Bucket aggregations in the context of Date Histogram -- it's certainly not for the faint-hearted. I think I finally figured out how to configure something SLA-like "bucket_average(max(bytes,1h),24h), split by OS" (see screenshots). Anything we can do to reduce the vertical amount of space needed for these configuration would improve the experience, imo. For one, I think we should prioritize removing unnecessary custom labels: custom label is not necessarry on order by custom metric #10217 (I would go so far to even say that it should be a blocker for 5.4 if we plan to ship pipeline aggs as part of that release). Secondarily, I wonder if additional levels of collapsing would be useful, though I'd imagine it's a harder endeavor.

Metric configuration is less staggering, since you can't split by anything, but superflous labels make it look convoluted as well.

tbragin · 2017-02-21T17:38:23Z

@ppisljar A couple of other problems I ran into... Do we allow pipeline agg of a pipeline or should that be disabled in the vis editor?

ppisljar · 2017-02-21T18:15:33Z

@tanya seems i forgot to push some things .... i'll fix this tomorrow

ppisljar · 2017-02-22T10:44:47Z

i updated and rebased ....

sorting on buckets is not allowed (in terms)
buckets on derivative are not allowed
derivative on buckets IS allowed
removing the labels: i agree, but not sure if i would make it a blocker .... (the PR is ready and waiting review)
switching without rebuilding: this is gonna be hard at this point (for 5.4) unless we want to go forward with a hacky solution (i oppose). there is no mechanism in place at the moment to allow this. If you are ok with tracking this separately please open an issue and link it here.

thomasneirynck · 2017-02-28T01:30:20Z

Could we split 'pipeline aggregations' up between parent and sibling aggregations? ie. have 3 sections iso 2.

The reason for this is that the metric configuration is different for each. It is a little confusing to see the UI form change.

thomasneirynck

Don't have much to say about this one, given the incremental addition.

The metric-selection UI is getting top-heavy, so I would consider a few simplifications.

I would consider splitting parent/sibling into their own category in the selector. Not only to give some texture to the list in the dropdown, but also prime the user that metrics-selector are different for each (siblings allow a full bucket selections, while bucket aggregations only allows to select metrics).
we should consider hiding the "Custom metric" dropdown for parent aggregations, since there is only a single option.

thomasneirynck · 2017-02-28T01:04:42Z

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

@@ -16,7 +16,12 @@ export default function TermsAggDefinition(Private) {
  const createFilter = Private(AggTypesBucketsCreateFilterTermsProvider);
  const routeBasedNotifier = Private(routeBasedNotifierProvider);

-  const aggFilter = ['!top_hits', '!percentiles', '!median', '!std_dev', '!derivative'];
+  const aggFilter = [


resolve merge conflict

ppisljar · 2017-02-28T09:42:35Z

thanks @thomasneirynck

i split aggs in 3 sections
rebased on master
regarding custom metric dropdown ... it will have multiple values if you have more metrics. besides, thats out of scope of this pr (bucket pipeline aggs don't have the metrics dropdown)

ppisljar · 2017-02-28T11:35:20Z

jenkins, test this

stacey-gammon · 2017-02-28T15:27:32Z

My choices here don't seem to be updating the y axis at all. Is this a bug or am I doing something wrong? I'm trying to create a visualization like the person in that discuss ticket yesterday was hoping to create.

thomasneirynck · 2017-02-28T20:07:21Z

@stacey-gammon That's because of the nature of the bucket sum, in that particular configuration. It does an overall sum of the counts, which is identical for all those fields (given that the same docs are included in all those buckets-by-date).

choose a metric other than count, and then you will see different results (e.g. min of bytes, ...)

thomasneirynck

For me, it's good!

FWIW, while playing around, I triggered an uncaught error. But I can no longer reproduce this, I forget the exact steps. Perhaps this is familiar?

:5601/xye/bundles/commons.bundle.js?v=8467:34823 TypeError: Cannot read property 'displayName' of undefined
    at MetricAggType.makeLabel (https://localhost:5601/xye/bundles/kibana.bundle.js?v=8467:119764:52)
    at AggConfigFactory.AggConfig.makeLabel (https://localhost:5601/xye/bundles/kibana.bundle.js?v=8467:120597:30)
    at https://localhost:5601/xye/bundles/kibana.bundle.js?v=8467:158117:38
    at Array.map (native)
    at Object.fn (https://localhost:5601/xye/bundles/kibana.bundle.js?v=8467:158114:51)
    at Scope.$digest (https://localhost:5601/xye/bundles/commons.bundle.js?v=8467:38172:30)
    at Scope.$apply (https://localhost:5601/xye/bundles/commons.bundle.js?v=8467:38443:25)
    at HTMLSelectElement.<anonymous> (https://localhost:5601/xye/bundles/commons.bundle.js?v=8467:50871:16)
    at HTMLSelectElement.dispatch (https://localhost:5601/xye/bundles/commons.bundle.js?v=8467:17466:10)
    at HTMLSelectElement.elemData.handle (https://localhost:5601/xye/bundles/commons.bundle.js?v=8467:17152:29)

stacey-gammon · 2017-03-01T14:30:47Z

src/core_plugins/metrics/public/components/__tests__/add_delete_buttons.js

@@ -0,0 +1,67 @@
+// import React from 'react';


this sneak in by mistake? :)

stacey-gammon

I'd like to see some selenium tests for some of the bugs encountered along the way, but understanding our limited time availability, LGTM.

ppisljar · 2017-03-01T14:46:27Z

future improvements: #10633

Backports PR #10070 **Commit 1:** allow parent aggs to have sub aggs defined * Original sha: c8484e4 * Authored by ppisljar <[email protected]> on 2017-02-06T15:47:33Z **Commit 2:** adding bucket_sum aggregation * Original sha: aea3cbf * Authored by ppisljar <[email protected]> on 2017-02-06T16:12:36Z **Commit 3:** adding bucket_avg, bucket_min and bucket_max * Original sha: 7602ee1 * Authored by ppisljar <[email protected]> on 2017-02-07T08:49:58Z **Commit 4:** fixing based on UI review * Original sha: 8af734c * Authored by ppisljar <[email protected]> on 2017-02-15T11:57:40Z **Commit 5:** adding tests * Original sha: 690dc6b * Authored by ppisljar <[email protected]> on 2017-02-15T12:21:28Z **Commit 6:** disable terms sorting on pipeline aggs * Original sha: 7e3a32c * Authored by ppisljar <[email protected]> on 2017-02-16T09:11:54Z **Commit 7:** fixing based on Staceys review * Original sha: ad8b5b5 * Authored by ppisljar <[email protected]> on 2017-02-16T09:12:18Z **Commit 8:** adding defaults * Original sha: 0fb1d93 * Authored by ppisljar <[email protected]> on 2017-02-22T10:40:09Z **Commit 9:** updated based on review * Original sha: cc6c703 * Authored by ppisljar <[email protected]> on 2017-02-28T09:40:21Z **Commit 10:** fixing error with stacking * Original sha: 5848c74 * Authored by ppisljar <[email protected]> on 2017-03-01T14:17:28Z

epixa · 2017-03-01T15:57:58Z

@ppisljar Can you add a detailed description (with screenshots) of these features to this PR description? Whenever we make any progress on pipeline aggs, please also drop a comment on the pipeline agg ticket since a lot of people want to know about this.

epixa added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Jan 26, 2017

tbragin added the Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) label Feb 2, 2017

ppisljar force-pushed the agg/pipeline_sum branch from 7dffa74 to 7322bd0 Compare February 10, 2017 11:25

thomasneirynck self-assigned this Feb 14, 2017

ppisljar force-pushed the agg/pipeline_sum branch from 7322bd0 to 1eeba3e Compare February 15, 2017 10:59

ppisljar added the review label Feb 15, 2017

ppisljar requested review from thomasneirynck and stacey-gammon February 15, 2017 12:22

ppisljar changed the title ~~WIP: bucket_sum pipeline aggregation~~ bucket_sum pipeline aggregation Feb 15, 2017

ppisljar changed the title ~~bucket_sum pipeline aggregation~~ Sum, Avg, Min and Max bucket pipeline aggregation Feb 15, 2017

stacey-gammon reviewed Feb 15, 2017

View reviewed changes

ppisljar added v5.4.0 v6.0.0 labels Feb 15, 2017

ppisljar force-pushed the agg/pipeline_sum branch from 906c227 to b3bf1fb Compare February 16, 2017 09:16

thomasneirynck reviewed Feb 28, 2017

View reviewed changes

ppisljar force-pushed the agg/pipeline_sum branch from 7be517b to 47b7be7 Compare February 28, 2017 09:40

thomasneirynck approved these changes Feb 28, 2017

View reviewed changes

thomasneirynck removed their assignment Feb 28, 2017

ppisljar force-pushed the agg/pipeline_sum branch from 47b7be7 to eb2c7ba Compare March 1, 2017 14:17

stacey-gammon reviewed Mar 1, 2017

View reviewed changes

ppisljar added 10 commits March 1, 2017 15:38

allow parent aggs to have sub aggs defined

c8484e4

adding bucket_sum aggregation

aea3cbf

adding bucket_avg, bucket_min and bucket_max

7602ee1

fixing based on UI review

8af734c

adding tests

690dc6b

disable terms sorting on pipeline aggs

7e3a32c

fixing based on Staceys review

ad8b5b5

adding defaults

0fb1d93

updated based on review

cc6c703

fixing error with stacking

5848c74

ppisljar force-pushed the agg/pipeline_sum branch from eb2c7ba to 5848c74 Compare March 1, 2017 14:38

stacey-gammon approved these changes Mar 1, 2017

View reviewed changes

ppisljar merged commit 841a3d2 into elastic:master Mar 1, 2017

elastic-jasper mentioned this pull request Mar 1, 2017

[5.x] Sum, Avg, Min and Max bucket pipeline aggregation #10634

Merged

ppisljar deleted the agg/pipeline_sum branch March 1, 2017 15:55

ppisljar mentioned this pull request May 1, 2017

derivative metric aggregation #10020

Merged

tbragin mentioned this pull request May 4, 2017

Pipeline aggregations #4584

Closed

ppisljar restored the agg/pipeline_sum branch September 26, 2018 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sum, Avg, Min and Max bucket pipeline aggregation #10070

Sum, Avg, Min and Max bucket pipeline aggregation #10070

ppisljar commented Jan 25, 2017 •

edited

Loading

stacey-gammon Feb 15, 2017

stacey-gammon Feb 15, 2017 •

edited

Loading

stacey-gammon Feb 15, 2017

stacey-gammon Feb 15, 2017

ppisljar Feb 16, 2017

stacey-gammon Feb 15, 2017

ppisljar Feb 16, 2017 •

edited

Loading

stacey-gammon commented Feb 15, 2017

ppisljar commented Feb 16, 2017

tbragin commented Feb 21, 2017 •

edited

Loading

tbragin commented Feb 21, 2017

ppisljar commented Feb 21, 2017

ppisljar commented Feb 22, 2017 •

edited

Loading

thomasneirynck commented Feb 28, 2017

thomasneirynck left a comment

thomasneirynck Feb 28, 2017

ppisljar commented Feb 28, 2017

ppisljar commented Feb 28, 2017

stacey-gammon commented Feb 28, 2017

thomasneirynck commented Feb 28, 2017

thomasneirynck left a comment

stacey-gammon Mar 1, 2017

stacey-gammon left a comment

ppisljar commented Mar 1, 2017

epixa commented Mar 1, 2017

Sum, Avg, Min and Max bucket pipeline aggregation #10070

Sum, Avg, Min and Max bucket pipeline aggregation #10070

Conversation

ppisljar commented Jan 25, 2017 • edited Loading

stacey-gammon Feb 15, 2017

Choose a reason for hiding this comment

stacey-gammon Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

stacey-gammon Feb 15, 2017

Choose a reason for hiding this comment

stacey-gammon Feb 15, 2017

Choose a reason for hiding this comment

ppisljar Feb 16, 2017

Choose a reason for hiding this comment

stacey-gammon Feb 15, 2017

Choose a reason for hiding this comment

ppisljar Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

stacey-gammon commented Feb 15, 2017

ppisljar commented Feb 16, 2017

tbragin commented Feb 21, 2017 • edited Loading

tbragin commented Feb 21, 2017

ppisljar commented Feb 21, 2017

ppisljar commented Feb 22, 2017 • edited Loading

thomasneirynck commented Feb 28, 2017

thomasneirynck left a comment

Choose a reason for hiding this comment

thomasneirynck Feb 28, 2017

Choose a reason for hiding this comment

ppisljar commented Feb 28, 2017

ppisljar commented Feb 28, 2017

stacey-gammon commented Feb 28, 2017

thomasneirynck commented Feb 28, 2017

thomasneirynck left a comment

Choose a reason for hiding this comment

stacey-gammon Mar 1, 2017

Choose a reason for hiding this comment

stacey-gammon left a comment

Choose a reason for hiding this comment

ppisljar commented Mar 1, 2017

epixa commented Mar 1, 2017

ppisljar commented Jan 25, 2017 •

edited

Loading

stacey-gammon Feb 15, 2017 •

edited

Loading

ppisljar Feb 16, 2017 •

edited

Loading

tbragin commented Feb 21, 2017 •

edited

Loading

ppisljar commented Feb 22, 2017 •

edited

Loading