Support multiple metrics in top_metrics agg
#52965
Conversation
This adds support for returning multiple metrics to the `top_metrics`
agg. It looks like:
```
POST /test/_search?filter_path=aggregations
{
"aggs": {
"tm": {
"top_metrics": {
"metrics": [
{"field": "v"},
{"field": "m"}
],
"sort": {"s": "desc"}
}
}
}
}
```
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
|
Adding a second metric to fetch is pretty cheap! Some benchmarks that I'm playing with have the 90% latency for one metric at 923ms and two metrics at 931ms. I'm kind of surprised it is in the neighborhood of 1% cost. |
...analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetrics.java
Show resolved
Hide resolved
| return topMetrics.get(0).metricValue; | ||
| int index = metricNames.indexOf(name); | ||
| if (index < 0) { | ||
| throw new IllegalArgumentException("known metric [" + name + "]"); |
There was a problem hiding this comment.
I don't understand what this error message means. Should it read "Unknown metric..."?
There was a problem hiding this comment.
+1 - it should be unknown metric. I don't believe we'll ever throw it because of the interplay with hasValue. Which bothers me, but is a problem for another PR.
| private final int size; | ||
| private final MultiValuesSourceFieldConfig metricField; | ||
| private final List<MultiValuesSourceFieldConfig> metricFields; | ||
| // TODO MultiValuesSourceFieldConfig has more things than we support and less things than we want to support |
There was a problem hiding this comment.
Can you expand on this a bit, and/or open a ticket for it? Feel free to ignore this comment if you're planing to resolve this TODO soon, but I'm worried that if we find this again in a few months we won't remember what it is we intended to do here.
Also, it looks like later on, we just turn this into a list of ValuesSourceConfig. Why not just start with ValuesSourceConfig at this point?
There was a problem hiding this comment.
I inherited this from @polyfractal's original implementation. I can look into changing to directly to ValuesSourceConfig.
There was a problem hiding this comment.
ha, that's exactly the kind of problem I want to prevent ;)
We could leave this as a TODO for now, since we're hoping to merge the new Values Source work soon-ish, and that dramatically reduces the complexity of ValuesSourceConfig, making it more like an actual config object and less like a magic box.
There was a problem hiding this comment.
(but if we do leave it as a todo, we should still clarify what we want to do and when)
| for (MultiValuesSourceFieldConfig config : fieldsConfig) { | ||
| ValuesSourceConfig<ValuesSource.Numeric> resolved = ValuesSourceConfig.resolve(ctx, ValueType.NUMERIC, | ||
| config.getFieldName(), config.getScript(), config.getMissing(), config.getTimeZone(), null); | ||
| if (resolved == null) { |
There was a problem hiding this comment.
Does ValuesSourceConfig#resolve actually return null in some cases? I didn't see a path that would cause that. Or is this just being defensive?
There was a problem hiding this comment.
I thought it did. Looks like I was confused. I'll dig.
| values[i++] = new MissingMetricValues(); | ||
| continue; | ||
| } | ||
| ValuesSource.Numeric valuesSource = resolved.toValuesSource(ctx); |
There was a problem hiding this comment.
Be aware that this can throw on invalid configs (i.e. configs containing neither a script nor a field)
| int i = 0; | ||
| for (MultiValuesSourceFieldConfig config : fieldsConfig) { | ||
| ValuesSourceConfig<ValuesSource.Numeric> resolved = ValuesSourceConfig.resolve(ctx, ValueType.NUMERIC, | ||
| config.getFieldName(), config.getScript(), config.getMissing(), config.getTimeZone(), null); |
There was a problem hiding this comment.
This seems late in the game to be resolving values sources. Usually we do that at the border between the builder and the factory, and pass the values source(s) into the aggregator. If it has to be this way, let's at least leave a comment as to why that's the case, please.
There was a problem hiding this comment.
I believe this is right at aggregator construction time - so it is on the boarder, just on the Aggregator side of it. I can see if I can move it if it'd make things nicer.
|
@not-napoleon I've updated this based on our conversation and believe it should be ready for another round. |
❤️ @elasticsearchmachine run elasticsearch-ci/2 |
…53163) This adds support for returning multiple metrics to the `top_metrics` agg. It looks like: ``` POST /test/_search?filter_path=aggregations { "aggs": { "tm": { "top_metrics": { "metrics": [ {"field": "v"}, {"field": "m"} ], "sort": {"s": "desc"} } } } } ```
This adds support for returning multiple metrics to the
top_metricsagg. It looks like:
Relates to #51813.