Add aggregation list to node info#60074
Conversation
There are at least two use cases (transform tests and telemetry mapping updated) in which we need to have an access to full list of supported aggregations. This PR adds this list to the node info API. Fixes elastic#59774
|
Pinging @elastic/es-core-features (:Core/Features/Features) |
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
jakelandis
left a comment
There was a problem hiding this comment.
changes look good. a couple comments. do you have an example of the output ?
| } | ||
| } | ||
|
|
||
| public Map<String, Set<String>> getProcessors() { |
There was a problem hiding this comment.
getProcessors() ? I assume the name is off, but is this even needed ?
There was a problem hiding this comment.
Copy and paste error. Will fix.
| metric: [ aggregations ] | ||
|
|
||
| # if this test failed because a new aggregation was added, please open an issues in the elastic/telemetry repository | ||
| # so they can update the mapping accordingly |
There was a problem hiding this comment.
this comment seems abit out of place, and probably shouldn't reference a private repo.
also, I guess I don't understand the output. is it aggregrations.??.types = ?? . Is there anything that prevents checking for known types
There was a problem hiding this comment.
I was sure I removed it. This comment is out of place. Good catch.
There was a problem hiding this comment.
The output looks like this:
"aggregations" : {
"adjacency_matrix" : {
"types" : [
"other"
]
},
"auto_date_histogram" : {
"types" : [
"boolean",
"date",
"numeric"
]
},
"avg" : {
"types" : [
"boolean",
"date",
"histogram",
"numeric"
]
},
"boxplot" : {
"types" : [
"histogram",
"numeric"
]
},
"cardinality" : {
"types" : [
"boolean",
"bytes",
"date",
"geopoint",
"geoshape",
"ip",
"numeric",
"range"
]
},
....
and both types and aggs change depending on license that this is ran under.
not-napoleon
left a comment
There was a problem hiding this comment.
I think this is fine to merge, but if we can make AggregationInfo immutable, I think that would be better.
| } | ||
|
|
||
| public Map<String, Set<String>> getAggregations() { | ||
| return aggs; |
There was a problem hiding this comment.
We're leaking a mutable reference here. It doesn't look like we mutate it anywhere, might be worth wrapping it to be immutable, if the object creation cost for the wrapper isn't too much.
Adds a full list of supported aggregations to the node info API. This list will be used in transform tests and telemetry mapping tests that will be added as follow-up PRs. Fixes elastic#59774
There are at least two use cases (transform tests and telemetry mapping
updated) in which we need to have an access to full list of supported
aggregations. This PR adds this list to the node info API.
Fixes #52057