-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Support aggregate_double_metric fields in the Field API #88534
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
Support aggregate_double_metric fields in the Field API #88534
Conversation
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
@elasticmachine test this please |
romseygeek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - a lot simpler than I thought it would need to be!
|
I will merge this after the other one...because this will give me some merge conflicts and I prefer to fix merge conflicts here rather than in the other just because this is simpler. |
| indices.get: | ||
| index: test | ||
|
|
||
| - do: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good to have those tests in the rollups code, but perhaps it would be simpler to test aggregate_metric_double field in isolation at the more basic aggregate-metrics yaml tests
|
|
||
| - match: { hits.hits.3.fields.metricset.0: pod } | ||
| - match: { hits.hits.3.fields.count.0: 16 } | ||
| - match: { hits.hits.3.fields.k8s\.pod\.network\.tx.0.min: 1.434587694E9 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, those tests should be skipped on all versions before 8.4.0
...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java
Show resolved
Hide resolved
|
Pinging @elastic/clients-team (Team:Clients) |
csoulios
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| metricset: | ||
| type: keyword | ||
| time_series_dimension: true | ||
| count: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this field being used anywhere in this test. Is this a left over?
|
@elasticmachine update branch |
|
@elasticmachine merge upstream |
For metric fields of type
aggregate_double_metric, we would like to return all metric values such asmax,min,sumandvalue_countfor agauge, instead of returning just the value for thedefaultmetric.Context
In the past, we had taken the decision to expose fields of type
aggregate_metric_doubleas regulardoublefields in the fields API by only returning thedefault_valuemetric sub-field. This decision intended to hide the complexity of this field from Kibana and only expose it as a simple numeric field. After recent discussions with the Kibana team, we decided that Elasticsearch should be transparent as to when a field is anaggregate_metric_doublefield. Elasticsearch should return all metric sub-fields and Kibana will deal with managing the field type.Consequences
This is a breaking change
Relates to #74660