-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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 multi valued metrics #88818
Support multi valued metrics #88818
Conversation
Pinging @elastic/es-analytics-geo (Team:Analytics) |
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.
Change looks simple and clean. And the code does the right thing when dealing with multi-values
I look forward to seeing how the code forms after leveraging the MultiValueMode
enum.
); | ||
private void collectMetric(final String field, final Object[] values) { | ||
for (var value : values) { | ||
if (value instanceof Number number) { |
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 wonder if you could add one instanceof
check outside the for
loop would work.
Something like this: if (values instanceof Number[])
On the other hand we should totally remove the instanceof
checks.
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.
Yeah, I think since we want to get the instanceof
checks out entirely it's fine to do it either way.
I need an approval for this. I didn't manage to use |
); | ||
private void collectMetric(final String field, final Object[] values) { | ||
for (var value : values) { | ||
if (value instanceof Number number) { |
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.
Yeah, I think since we want to get the instanceof
checks out entirely it's fine to do it either way.
I think I'm ok to approve this while @csoulios is out for a bit. We have a bit of time before it fully lands in production and we expect a fair bit of refactoring over here eventually. |
💚 Backport successful
|
* upstream/main: (265 commits) Disable openid connect tests due to missing fixture (elastic#89478) Add periodic job for single processor node testing Updates to changelog processing after docs redesign (elastic#89463) Better support for multi cluster for run task (elastic#89442) Mute failing tests (elastic#89465) [ML] Performance improvements related to ECS Grok pattern usage (elastic#89424) Add source fallback support for date and date_nanos mapped types (elastic#89440) Reuse Info in lifecycle step (elastic#89419) feature: support metrics for multi value fields (elastic#88818) Upgrade OpenTelemetry API and remove workaround (elastic#89438) Remove LegacyClusterTaskResultActionListener (elastic#89459) Add YAML spec docs about matching errors (elastic#89370) Remove redundant cluster upgrade tests for auth tokens (elastic#89417) Return 400 error for GetUserPrivileges call with API keys (elastic#89333) User Profile - Detailed errors in hasPrivileges response (elastic#89224) Rollover min_* conditions docs and highlight (elastic#89434) REST tests for percentiles_bucket agg (elastic#88029) REST tests for cumulative pipeline aggs (elastic#88966) Clean-up file watcher keys. (elastic#89429) fix a typo in Security.java (elastic#89248) ... # Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
With this PR we handle collection of multi valued metrics
The idea is just to iterate on each value and collect all of
them as if each value belongs to a different document.