Skip to content
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

Conversation

salvatore-campagna
Copy link
Contributor

@salvatore-campagna salvatore-campagna commented Jul 26, 2022

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.

@salvatore-campagna salvatore-campagna self-assigned this Jul 26, 2022
@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v8.4.0 labels Jul 26, 2022
@salvatore-campagna salvatore-campagna added :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data :StorageEngine/TSDB You know, for Metrics and removed needs:triage Requires assignment of a team area label v8.4.0 labels Jul 26, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 26, 2022
@salvatore-campagna salvatore-campagna changed the title feature: support metrics for multi value fields Support multi valued metrics Jul 26, 2022
@mark-vieira mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022
Copy link
Contributor

@csoulios csoulios left a 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) {
Copy link
Contributor

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.

Copy link
Member

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.

@salvatore-campagna salvatore-campagna added auto-backport-and-merge Automatically create backport pull requests and merge when ready v8.4.1 labels Aug 3, 2022
@salvatore-campagna
Copy link
Contributor Author

salvatore-campagna commented Aug 8, 2022

I need an approval for this. I didn't manage to use MultiValueMode because the change is larger than expected. As a result, I would proceed to merge this and work on other issues. Maybe @nik9000 or @romseygeek can help with this?

);
private void collectMetric(final String field, final Object[] values) {
for (var value : values) {
if (value instanceof Number number) {
Copy link
Member

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.

@nik9000
Copy link
Member

nik9000 commented Aug 18, 2022

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.

@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.4

weizijun added a commit to weizijun/elasticsearch that referenced this pull request Aug 19, 2022
* 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
@mark-vieira mark-vieira added v8.4.0 and removed v8.4.1 labels Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport-and-merge Automatically create backport pull requests and merge when ready >non-issue :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data :StorageEngine/TSDB You know, for Metrics Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.4.0 v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants