-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Support global ordinals in top_metrics
#64967
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
Conversation
Adds support for fetching `keyword` and `ip` fields in `top_metrics`. This only works if we can get global ordinals for those fields so we can't support the entire `keyword` family, but this gets us *somewhere*. Closes elastic#64774
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
...alytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregator.java
Show resolved
Hide resolved
0384f91 to
59e7cf5
Compare
|
run elasticsearch-ci/2 |
not-napoleon
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.
I think this is fine. I like that we're able to leverage the VSRegistry for top metrics now. I'm still not sure the MultiValuesSourceConfig logic is the right long term solution, but I don't have a better plan and I have no interest in digging into it now, so there's no reason to block on that.
Yeah. I didn't have any good ideas other than "don't do this now and tangle it up in support for bytes". It can come later, yeah. |
Adds support for fetching `keyword` and `ip` fields in `top_metrics`. This only works if we can get global ordinals for those fields so we can't support the entire `keyword` family, but this gets us *somewhere*. Closes elastic#64774
top_metrics
Adds support for fetching
keywordandipfields intop_metrics.This only works if we can get global ordinals for those fields so we
can't support the entire
keywordfamily, but this gets us somewhere.Closes #64774