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

[Transform] add support for top metrics #71850

Merged
merged 7 commits into from
Apr 27, 2021

Conversation

hendrikmuhs
Copy link
Contributor

@hendrikmuhs hendrikmuhs commented Apr 19, 2021

add support for the stats and top metrics aggregation in transform. With this change it became easier
to add more multi value aggregations to transform

Limitations:

  • only the 1st element of top_metrics gets consumed by transform[*].
  • all values of stats will be mapped to double if mapping deduction is used, including count, sum, min, max

fixes #52236
relates #51925

Docs preview: https://elasticsearch_71850.docs-preview.app.elstc.co/diff

@hendrikmuhs
Copy link
Contributor Author

I rebased this PR after #71738 got merged.

I think maybe we should stop top_metrics from extending InternalNumericMetricsAggregation.MultiValue. Like you said somewhere else - it ain't really numeric any more.

I created a new branch to explore this idea, see #71903. It is a nice cleanup and would make a lot of changes in this PR unnecessary.

@hendrikmuhs hendrikmuhs force-pushed the transform-top_metrics branch 2 times, most recently from 525a22a to 773076f Compare April 23, 2021 12:24
@hendrikmuhs hendrikmuhs marked this pull request as ready for review April 23, 2021 12:34
@elasticmachine elasticmachine added the Team:ML Meta label for the ML team label Apr 23, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

@hendrikmuhs
Copy link
Contributor Author

After re-factorings of aggregations have been moved into another PR (#71903, merged) this PR only touches transform code.

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

Overall, great change. Its fantastic that we can reuse this dot delimited formatting for aggs. Shows that we have a nice framework design.

Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transform support for top_metrics
6 participants