Add Normalize Pipeline Aggregation by talevy · Pull Request #56399 · elastic/elasticsearch

talevy · 2020-05-08T03:06:19Z

This aggregation will perform normalizations of metrics
for a given series of data in the form of bucket values.

The aggregations supports the following normalizations

rescale 0-1
rescale 0-100
percentage of sum
mean normalization
z-score normalization
softmax normalization

To specify which normalization is to be used, it can be specified
in the normalize agg's normalizer field.

For example:

{
  "normalize": {
    "buckets_path": <>,
    "normalizer": "percent"
  }
}

Closes #51005.

elasticmachine · 2020-05-08T03:06:21Z

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

This aggregation will perform normalizations of metrics for a given series of data in the form of bucket values. The aggregations supports the following normalizations - rescale 0-1 - rescale 0-100 - percentage of sum - mean normalization - z-score normalization - softmax normalization To specify which normalization is to be used, it can be specified in the normalize agg's `normalizer` field. For example: ``` { "normalize": { "buckets_path": <>, "normalizer": "percent" } } ``` Closes elastic#51005.

nik9000

I left some comments because I'm excited about this!

nik9000 · 2020-05-08T12:12:09Z

+    static final ParseField NORMALIZER_FIELD = new ParseField("normalizer");
+
+    @SuppressWarnings("unchecked")
+    public static final ConstructingObjectParser<NormalizePipelineAggregationBuilder, String> PARSER = new ConstructingObjectParser<>(


Check out InstantiatingObjectParser!

so, I tried changing that parser to work here, but I think it deserves its own change. The InstantiatingObjectParser does not expose the Context in such a way that more constructor arguments can be passed in. I believe this can change, but I'd rather not do that here

nik9000 · 2020-05-08T12:15:34Z

+                normalizedBucketValue = normalizer.normalize(thisBucketValue);
+            }
+
+            List<InternalAggregation> aggs = StreamSupport.stream(bucket.getAggregations().spliterator(), false)


bucket.getAggregations().copyResults() does this without so much boiler plate.

unfortunately, that method does not work in this context. I think a more dedicated cleanup for this boilerplate can be tackled outside of this PR

nik9000

LGTM. Merge whenever you are happy with the docs!

polyfractal

Left a few comments. I think the only notable one is about handling terms agg as the parent bucket :)

Looks good!

polyfractal · 2020-05-12T17:43:22Z

+--------------------------------------------------
+// NOTCONSOLE
+
+[[normalizer_pipeline-params]]


Should we make a note somewhere that this pipeline always uses a skip gap policy?

polyfractal · 2020-05-12T17:45:40Z

+
+public class NormalizePipelineAggregationBuilder extends AbstractPipelineAggregationBuilder<NormalizePipelineAggregationBuilder> {
+    public static final String NAME = "normalize";
+    static final ParseField NORMALIZER_FIELD = new ParseField("normalizer");


Fine with normalizer, but wanted to also suggest method as a potential param name. No strong opinion though :)

I was wishy washy on the naming here as well, and decided not to fret, but I too have leaned towards method earlier, so I am happy to do so here. especially given the overloading of the term across the stack.

I've updated the naming to be method

polyfractal · 2020-05-12T17:50:12Z

+        if (bucketsPaths.length != 1) {
+            context.addBucketPathValidationError("must contain a single entry for aggregation [" + name + "]");
+        }
+    }


Should we also check context.validateHasParent() to make sure this isn't at the top level?

ah, yes. I wasn't aware of this. thanks for bringing it up

added a check and a test for this!

polyfractal · 2020-05-12T17:55:48Z

+                histo = (InternalMultiBucketAggregation<? extends InternalMultiBucketAggregation, ? extends
+                InternalMultiBucketAggregation.InternalBucket>) aggregation;
+        List<? extends InternalMultiBucketAggregation.InternalBucket> buckets = histo.getBuckets();
+        HistogramFactory factory = (HistogramFactory) histo;


Do we know if this works with a terms agg as the parent? It feels like it should (e.g. it doesn't require any specific ordering of the buckets, unlike something like a moving avg which needs an ordering).

If we think it should work with terms we should tweak this to not use a HistogramFactory directly. BucketScriptPipelineAggregator has an example of how to generically build buckets from any InternalMultiBucketAggregation (the internal agg can create buckets too, not just the factory).

thanks! I was slightly loose in my interpretation of the HistogramFactory's comment

/** Implemented by histogram aggregations and used by pipeline aggregations to insert buckets. */

Will look at how BucketScript does things and add a test for terms agg!

Yikes! I'm sorry I didn't notice this one!

thanks, I've updated to include a test for terms and use a more generic way to make new buckets

polyfractal

LGTM!

This aggregation will perform normalizations of metrics for a given series of data in the form of bucket values. The aggregations supports the following normalizations - rescale 0-1 - rescale 0-100 - percentage of sum - mean normalization - z-score normalization - softmax normalization To specify which normalization is to be used, it can be specified in the normalize agg's `normalizer` field. For example: ``` { "normalize": { "buckets_path": <>, "normalizer": "percent" } } ``` Closes elastic#51005.

This aggregation will perform normalizations of metrics for a given series of data in the form of bucket values. The aggregations supports the following normalizations - rescale 0-1 - rescale 0-100 - percentage of sum - mean normalization - z-score normalization - softmax normalization To specify which normalization is to be used, it can be specified in the normalize agg's `normalizer` field. For example: ``` { "normalize": { "buckets_path": <>, "normalizer": "percent" } } ```

Relates: elastic/elasticsearch#56399 This commit adds the normalize aggregation to the high level client.

Relates: elastic/elasticsearch#56399 This commit adds the normalize aggregation to the high level client. Co-authored-by: Russ Cam <russ.cam@elastic.co>

talevy added WIP :Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels May 8, 2020

talevy force-pushed the normalize branch from 17e0108 to 7e8510d Compare May 8, 2020 03:07

talevy force-pushed the normalize branch from 7e8510d to 5427899 Compare May 8, 2020 03:15

nik9000 self-requested a review May 8, 2020 12:09

nik9000 reviewed May 8, 2020

View reviewed changes

talevy added 5 commits May 8, 2020 11:28

Merge remote-tracking branch 'elastic/master' into normalize

15e5ca8

moar

9782c2a

respond to rev

2dae977

Merge remote-tracking branch 'elastic/master' into normalize

8cd088b

revert change

9718e01

talevy requested a review from nik9000 May 12, 2020 05:30

talevy added v7.9.0 v8.0.0 and removed WIP labels May 12, 2020

talevy commented May 12, 2020

View reviewed changes

Comment thread docs/reference/aggregations/pipeline/normalize-aggregation.asciidoc

nik9000 approved these changes May 12, 2020

View reviewed changes

Merge remote-tracking branch 'elastic/master' into normalize

9c64e36

$polyfractal$

polyfractal reviewed May 12, 2020

View reviewed changes

talevy added 5 commits May 13, 2020 15:43

respond to changes

023c34f

Merge remote-tracking branch 'elastic/master' into normalize

4e76dbd

update docs

8da4960

format

5d4737c

touch up

aa9eebc

$polyfractal$

polyfractal approved these changes May 14, 2020

View reviewed changes

talevy added 2 commits May 14, 2020 10:08

use format in example

72f29e7

comma

1e9f015

final fix

e6db0f9

talevy merged commit 79367e4 into elastic:master May 14, 2020

talevy deleted the normalize branch May 14, 2020 20:32

rayafratkina mentioned this pull request May 14, 2020

[Meta] Kibana support for ES aggregations elastic/kibana#58628

Closed

3 of 7 tasks

jakelandis mentioned this pull request May 26, 2020

test failure: org.elasticsearch.xpack.analytics.normalize.NormalizeAggregatorTests.testTermsAggParent #57164

Closed

pugnascotia added >enhancement >feature and removed >enhancement labels Jul 16, 2020

russcam mentioned this pull request Jul 23, 2020

7.9.0 Meta ticket elastic/elasticsearch-net#4872

Closed

29 tasks done

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jul 28, 2020

Add normalize aggregation

ff6c49f

Relates: elastic/elasticsearch#56399 This commit adds the normalize aggregation to the high level client.

russcam mentioned this pull request Jul 28, 2020

Add normalize aggregation elastic/elasticsearch-net#4886

Merged

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jul 31, 2020

Add normalize aggregation (#4886)

8bd1908

Relates: elastic/elasticsearch#56399 This commit adds the normalize aggregation to the high level client.

github-actions Bot pushed a commit to elastic/elasticsearch-net that referenced this pull request Jul 31, 2020

Add normalize aggregation (#4886)

e4f6d75

Relates: elastic/elasticsearch#56399 This commit adds the normalize aggregation to the high level client.

github-actions Bot pushed a commit to elastic/elasticsearch-net that referenced this pull request Jul 31, 2020

Add normalize aggregation (#4886)

83fd7d7

Relates: elastic/elasticsearch#56399 This commit adds the normalize aggregation to the high level client.

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

talevy commented May 8, 2020

Uh oh!

elasticmachine commented May 8, 2020

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

polyfractal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polyfractal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal left a comment