Skip to content

Implement aggregate_metric field mapper#49830

Merged
csoulios merged 31 commits intoelastic:feature/aggregate-metricsfrom
csoulios:rollups
Mar 31, 2020
Merged

Implement aggregate_metric field mapper#49830
csoulios merged 31 commits intoelastic:feature/aggregate-metricsfrom
csoulios:rollups

Conversation

@csoulios
Copy link
Copy Markdown
Contributor

@csoulios csoulios commented Dec 4, 2019 •
edited
Loading

This PR addresses part of #42720 creating a field mapper for storing pre-computed aggregate metrics such as min, max, sum and value_count

@csoulios csoulios added :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data WIP labels Dec 4, 2019
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Dec 4, 2019

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

@csoulios csoulios added the v8.0.0 label Dec 4, 2019
@csoulios csoulios requested review from iverase and jimczi December 4, 2019 14:02
iverase
iverase requested changes Dec 5, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @csoulios.

I think the parse logic should be on the parse method not the parseCreateField method. The latter is used (I think) for dynamic mapping and I don't think this field allow that.

I am not sure the leniency in the parse logic. I would like to confirm it but if there is an error on one of the metrics (does not exist or have an incorrect value) we should stop generating the doc value if ignore malformed is true.

jimczi
jimczi reviewed Dec 5, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @csoulios . I think you can use a dedicated module for this field mapper (see mapper-flattened), no need to put it in the rollup module.
I left the other comments in the pr.

polyfractal
polyfractal reviewed Dec 9, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@polyfractal polyfractal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments! :)

Open question for discussion: since this is isolated functionality at the moment (can configure but not actually use the fields yet), should we potentially merge to a feature branch instead? Not sure of our plans to backport to 7x, but it would make a potential backport (or revert) easier too.

...src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateMetricFieldMapper.java Outdated
"] must contain all metrics " + metrics.value().toString());
}

if (fieldType().hasDocValues()) {
Copy link
Copy Markdown
Contributor

@polyfractal polyfractal Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should override setHasDocValues() on the MappedFieldType so that it throws an exception if the user tries to disable doc values? Since we don't index anything and only store doc values, seems like we shouldn't let the user disable them?

Not sure, WDYT?

@csoulios
Copy link
Copy Markdown
Contributor Author

csoulios commented Dec 16, 2019

@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/2

@csoulios
Merge branch 'master' into rollups
0eed690
@csoulios
Fixes to address code review comments.
29a1eb3 
Make parsing of malformed values stricter and
ignore all field metrics even if a single metric
is wrong
@csoulios
Fix broken integration tests
471f23b
@csoulios
Copy link
Copy Markdown
Contributor Author

csoulios commented Dec 17, 2019

@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/default-distro

jimczi
jimczi reviewed Jan 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good @csoulios, I left some comments regarding the tests. Can you also create a branch to merge this pr (can be in your fork) since we said that we'll need metrics aggregation support to publish in master ?

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java Outdated
builder.store(false);
builder.index(true);
builder.docValues(true);
builder.fieldType().setDocValuesType(DocValuesType.NUMERIC);
Copy link
Copy Markdown
Contributor

@jimczi jimczi Dec 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these options are set by default in the NumberFieldMapper.Builder

Copy link
Copy Markdown
Contributor Author

@csoulios csoulios Jan 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java
public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, QueryShardContext context) {
return delegateFieldType().rangeQuery(lowerTerm, upperTerm, includeLower, includeUpper, context);
}

Copy link
Copy Markdown
Contributor

@jimczi jimczi Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also override isFieldWithinQuery to handle range queries efficiently, fielddataBuilder to handle aggregations and sort, and docValueFormat to format double correctly.

iverase
iverase reviewed Jan 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the idea now. It is looking good, just left a few comments for clarification.

@csoulios csoulios changed the base branch from master to feature/aggregate-metrics January 14, 2020 14:55
@axw axw mentioned this pull request Jan 16, 2020
Closed
1 task
@axw axw mentioned this pull request Jan 23, 2020
Closed
@jimczi jimczi mentioned this pull request Feb 27, 2020
Closed
@csoulios csoulios mentioned this pull request Mar 23, 2020
Merged
iverase
iverase reviewed Mar 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @csoulios. Easy to follow the mapping logic. I would only recommend to add a test that uses a parent object in the mapping so we make sure we are resolving the path correctly in the metric's docValues in that case.

@csoulios
Copy link
Copy Markdown
Contributor Author

csoulios commented Mar 31, 2020

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/packaging-sample-unix-docker

@csoulios csoulios merged commit 7b64d83 into elastic:feature/aggregate-metrics Mar 31, 2020
@csoulios csoulios deleted the rollups branch March 31, 2020 10:16
@polyfractal polyfractal mentioned this pull request Mar 31, 2020
Closed
21 tasks
csoulios added a commit that referenced this pull request May 14, 2020
@csoulios
Implement aggregations on aggregate metrics (#53986)
d89f9f3 
Following the implementation of the aggregate_metric_double field mapper(#49830) we are implementing the Min, Max, ValueCount, Sum and Average aggregations on aggregate metrics.

The code builds on the excellent work done for #42949 and uses the extensible ValuesSources infrastructure to wire up common metric aggregation on the aggregate_metric_double field type.

This PR is part of the rollups v2 refactoring as described in meta issue #42720
@talevy talevy added the Team:Deployment Management Meta label for Management Experience - Deployment Management team label Jul 6, 2020
@axw axw mentioned this pull request Sep 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimczi jimczi jimczi left review comments

@iverase iverase iverase requested changes

@polyfractal polyfractal Awaiting requested review from polyfractal

Assignees

No one assigned

Labels

backport pending :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data Team:Deployment Management Meta label for Management Experience - Deployment Management team v8.0.0-alpha1 WIP

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@csoulios @elasticmachine @polyfractal @jimczi @iverase @talevy @jakelandis