cumulative sum, moving avg and serial diff pipeline aggregation metric #10033

ppisljar · 2017-01-24T08:15:06Z

adds:
cumulative sum pipeline aggregation - closes #740
moving average pipeline aggregation -
serial diff pipeline aggregation

stacey-gammon · 2017-02-07T15:40:48Z

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

+          },
+          deserialize: function (state, agg) {
+            return this.makeAgg(agg, state);
+          },


Up to you but you can compress these 6 lines to two via:

serialize: (customMetric) => customMetric.toJSON(), deserialize: (state, agg) => this.makeAgg(agg, state),

One minor note, I like to keep the order of params somewhat consistent and here we are switching the order of agg and state, but I'm guessing this is part or the interface and wouldn't be a simple thing to change, so fine as is.

i agree (Regarding order of params) but as you noted its part of the interface

stacey-gammon · 2017-02-07T15:48:01Z

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

+    {
+      group: 'none',
+      name: 'metricAgg',
+      title: 'Metric Agg',


Is Metric Agg specific to derivatives or a certain type of visualization? I think it's only used for derivates right now, but the name is so generic, it sounds like it can be used elsewhere? I guess I don't completely understand what a metric agg is. In the UI, is it the Y-Axis Aggregation spot? But only when it's not one of the aggregations listed above. Like a Count is a Metric Agg but Top Hits is some other kind of Agg? Or is only derivative a metric agg?

right now its only used by derivative (and comulative sum), we'll be adding other pipeline aggregations shortly and they all use this.

its the metric you define on the derivative (either custom metric or one of the pre existing metric aggs) .... list is filtered down as not all metrics are supported with derivative (you can't calculate a derivative on top hits for example)

stacey-gammon · 2017-02-07T15:51:40Z

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

+import { parentPipelineAggController } from './lib/parent_pipeline_agg_controller';
+import { parentPipelineAggWritter } from './lib/parent_pipeline_agg_writter';
+
+const ParentPipelineAggHelperProvider = function (Private) {


Just to confirm I understand the term parentPipeline, it s when in the Y-Axis you have a sub aggregation, and right now is only derivatives, but you split this logic out because it will be relevant to the cumulative sum you mentioned that you are also adding?

stacey-gammon

For the 07bd1f55233a6918699ee0b3c47ec74623726a39 commit specifically - LGTM

stacey-gammon · 2017-02-07T15:54:47Z

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

+    title: 'Cumulative Sum',
+    makeLabel: agg => makeNestedLabel(agg, 'cumulative sum'),
+    params: [
+      ...parentPipelineAggHelper.params()


Interesting. So how is this different from derivatives? Looks the exact same but with a different name & title. The specific calculation differences must be somewhere. :)

... maybe in makeAgg: function (termsAgg, state) { the termsAgg.vis is different?

It could may be even be refactored further because the only difference between the two is three strings, that could really even be one string if you applied the same normalizations to them (lowercase for nested label, lc and spaces => underscore for name, title as is).

Maybe unnecessary, just a thought. I'm still trying to figure out how choosing cumulative sum matches to the different calculations.

so yeah, its pretty much the same, but different ES query will get executed (the name defines the name used in ES query .... so this one will run cumulative_sum instead of derivative ... but apart from that they are the same.

why i don't want to refactor this further: at this point they are indeed the same (cumulative sum and derivative) but they actually have different sub options (which we don't expose right now but probably will in the future).

with the current form it'll be really easy to add the parameters that are different + still not be duplicating logic for the parameters that are the same.

ppisljar · 2017-02-07T16:28:05Z

@stacey-gammon i added moving_avg and serial_diff to this PR as well as it might make it clearer.
once you confirm this looks OK i'll start adding tests and documentation

stacey-gammon · 2017-02-07T17:20:58Z

Awesome, looks good and thanks for the explanations, helped a lot.

thomasneirynck · 2017-02-15T16:32:28Z

src/ui/public/agg_types/__tests__/metrics/parent_pipeline.js

+import VisProvider from 'ui/vis';
+import StubbedIndexPattern from 'fixtures/stubbed_logstash_index_pattern';
+
+const metrics = [


in the UI, consider using the same division that you used for pipeline aggs. it presents well.

i will add it in the bucket PR (will rebase in on master once this gets merged)

thomasneirynck · 2017-02-15T16:40:00Z

jenkins, test this

thomasneirynck

LGTM, but let's wait on jenkins...

ppisljar · 2017-02-15T16:50:08Z

Peter Pisljar [17:46]
are pipeline aggregations 'hard' on the cluster ?
we are adding pipeline aggregations to kibana and we allow you to do multiple levels (like 5th derivative ...) and i am wondering if this could have a big performance problems ?

Colin Goodheart-Smithe [17:49]
@ppisljar no, shouldn’t have particularly big performance issues since the pipeline aggregation only act on the final reduced results of the aggregations rather than on the documents themselves. Having said that, if you did something like 1 millisecond interval on a histogram over the range of a year then I would expect there to be issues, but I would expect that regardless of whether there are pipeline aggs involved

ppisljar · 2017-02-15T18:12:49Z

jenkins, test this

ppisljar · 2017-02-15T18:35:28Z

jenkins, test this

Backports PR #10033 **Commit 1:** extracting to parent agg helper * Original sha: b92535f * Authored by ppisljar <[email protected]> on 2017-02-07T09:38:00Z **Commit 2:** adding comulative sum agg * Original sha: 457338f * Authored by ppisljar <[email protected]> on 2017-02-07T09:38:19Z **Commit 3:** adding serial diff and moving avg * Original sha: c104481 * Authored by ppisljar <[email protected]> on 2017-02-07T16:26:35Z **Commit 4:** adding tests * Original sha: 02b3371 * Authored by ppisljar <[email protected]> on 2017-02-09T12:43:20Z

#10376) Backports PR #10033 **Commit 1:** extracting to parent agg helper * Original sha: b92535f * Authored by ppisljar <[email protected]> on 2017-02-07T09:38:00Z **Commit 2:** adding comulative sum agg * Original sha: 457338f * Authored by ppisljar <[email protected]> on 2017-02-07T09:38:19Z **Commit 3:** adding serial diff and moving avg * Original sha: c104481 * Authored by ppisljar <[email protected]> on 2017-02-07T16:26:35Z **Commit 4:** adding tests * Original sha: 02b3371 * Authored by ppisljar <[email protected]> on 2017-02-09T12:43:20Z

olivierlambert · 2017-02-16T22:37:56Z

@ppisljar that's great news! Any idea in which release this will be available? (therefore, when?)

ppisljar · 2017-02-20T10:48:52Z

hopefully in the 5.4

olivierlambert · 2017-02-20T11:30:54Z

👍 Thanks for your quick answer!

jimgoodwin · 2017-03-01T19:31:54Z

How do you specify "lag" for serial diff or for moving average "window size" ?

ppisljar · 2017-03-01T22:00:50Z

click the advanced setting under your seriial diff agg and in json input enter your additional configuration like { "lag": 10 }

ppisljar added Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Feature:Visualizations Generic visualization features (in case no more specific feature label is available) blocked labels Jan 24, 2017

ppisljar requested a review from thomasneirynck January 24, 2017 08:15

ppisljar force-pushed the agg/comulative-sum branch 2 times, most recently from 55e1d09 to e52d0b6 Compare February 7, 2017 09:44

stacey-gammon reviewed Feb 7, 2017

View reviewed changes

stacey-gammon approved these changes Feb 7, 2017

View reviewed changes

stacey-gammon reviewed Feb 7, 2017

View reviewed changes

ppisljar changed the title ~~WIP: cumulative sum metric aggregation~~ cumulative sum, moving avg and serial diff pipeline aggregation metric Feb 9, 2017

ppisljar added the review label Feb 9, 2017

ppisljar added 4 commits February 13, 2017 19:57

extracting to parent agg helper

b92535f

adding comulative sum agg

457338f

adding serial diff and moving avg

c104481

adding tests

02b3371

ppisljar force-pushed the agg/comulative-sum branch from c7954a4 to 02b3371 Compare February 13, 2017 18:59

ppisljar removed the blocked label Feb 13, 2017

thomasneirynck reviewed Feb 15, 2017

View reviewed changes

thomasneirynck approved these changes Feb 15, 2017

View reviewed changes

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

ppisljar merged commit 723c7c1 into elastic:master Feb 15, 2017

elastic-jasper mentioned this pull request Feb 15, 2017

[5.x] cumulative sum, moving avg and serial diff pipeline aggregation metric #10376

Merged

ppisljar deleted the agg/comulative-sum branch February 20, 2017 10:49

alexfrancoeur mentioned this pull request Mar 30, 2017

[DOCS] Update visualization documentation for pipeline aggregations #10955

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cumulative sum, moving avg and serial diff pipeline aggregation metric #10033

cumulative sum, moving avg and serial diff pipeline aggregation metric #10033

ppisljar commented Jan 24, 2017 •

edited

Loading

stacey-gammon Feb 7, 2017

ppisljar Feb 7, 2017

stacey-gammon Feb 7, 2017

ppisljar Feb 7, 2017

stacey-gammon Feb 7, 2017

ppisljar Feb 7, 2017

stacey-gammon left a comment •

edited

Loading

stacey-gammon Feb 7, 2017 •

edited

Loading

ppisljar Feb 7, 2017

ppisljar Feb 7, 2017

ppisljar commented Feb 7, 2017 •

edited

Loading

stacey-gammon commented Feb 7, 2017

thomasneirynck Feb 15, 2017

ppisljar Feb 15, 2017

thomasneirynck commented Feb 15, 2017

thomasneirynck left a comment

ppisljar commented Feb 15, 2017

ppisljar commented Feb 15, 2017

ppisljar commented Feb 15, 2017

olivierlambert commented Feb 16, 2017

ppisljar commented Feb 20, 2017

olivierlambert commented Feb 20, 2017

jimgoodwin commented Mar 1, 2017

ppisljar commented Mar 1, 2017

cumulative sum, moving avg and serial diff pipeline aggregation metric #10033

cumulative sum, moving avg and serial diff pipeline aggregation metric #10033

Conversation

ppisljar commented Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon left a comment • edited Loading

Choose a reason for hiding this comment

stacey-gammon Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppisljar commented Feb 7, 2017 • edited Loading

stacey-gammon commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck commented Feb 15, 2017

thomasneirynck left a comment

Choose a reason for hiding this comment

ppisljar commented Feb 15, 2017

ppisljar commented Feb 15, 2017

ppisljar commented Feb 15, 2017

olivierlambert commented Feb 16, 2017

ppisljar commented Feb 20, 2017

olivierlambert commented Feb 20, 2017

jimgoodwin commented Mar 1, 2017

ppisljar commented Mar 1, 2017

ppisljar commented Jan 24, 2017 •

edited

Loading

stacey-gammon left a comment •

edited

Loading

stacey-gammon Feb 7, 2017 •

edited

Loading

ppisljar commented Feb 7, 2017 •

edited

Loading