Skip to content

Deprecate aggregate metric double default metric configuration#141877

Merged
gmarouli merged 24 commits intoelastic:mainfrom
gmarouli:deprecate-aggregate-metric-double-default-metric
Feb 12, 2026
Merged

Deprecate aggregate metric double default metric configuration#141877
gmarouli merged 24 commits intoelastic:mainfrom
gmarouli:deprecate-aggregate-metric-double-default-metric

Conversation

@gmarouli
Copy link
Copy Markdown
Contributor

@gmarouli gmarouli commented Feb 4, 2026

In this PR we deprecate the usage of the default_metric in the aggregate_metric_double. This consists of:

  • Deprecate the parameter.
  • Use max or another available metric to filter, display value and sort.
  • Update tests to handle the warning so bwc tests can keep working
  • Introduce a cluster feature, so we can update the downsample action to stop using the parameter.

@gmarouli gmarouli changed the title Deprecate aggregate metric double default metric [WIP] Deprecate aggregate metric double default metric configuration Feb 4, 2026
@gmarouli gmarouli changed the title [WIP] Deprecate aggregate metric double default metric configuration Deprecate aggregate metric double default metric configuration Feb 6, 2026
@gmarouli gmarouli added >enhancement :StorageEngine/Mapping The storage related side of mappings labels Feb 6, 2026
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Hi @gmarouli, I've created a changelog YAML for you.

@gmarouli gmarouli marked this pull request as ready for review February 6, 2026 08:35
@gmarouli gmarouli requested a review from a team as a code owner February 6, 2026 08:35
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

Copy link
Copy Markdown
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question, but LGTM otherwise.

@gmarouli
Copy link
Copy Markdown
Contributor Author

gmarouli commented Feb 9, 2026

anyone from @elastic/obs-ds-intake-services would like to review this?

Copy link
Copy Markdown
Member

@carsonip carsonip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, code change lgtm. Awaiting confirmation from UI team whether the queries / aggs on our aggregate_metric_double fields would be affected by the future change default_metric to avg.

Copy link
Copy Markdown
Member

@carsonip carsonip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving to unblock, based on the understanding:

  • only avg is used on transaction.duration.summary
  • only value_count and sum are used on event.success_count

@gmarouli gmarouli merged commit f19d2f1 into elastic:main Feb 12, 2026
35 checks passed
@gmarouli gmarouli deleted the deprecate-aggregate-metric-double-default-metric branch February 12, 2026 12:16
gmarouli added a commit that referenced this pull request Feb 25, 2026
In this PR we replace the "derived metric" introduced in #141877 with a script average or if the `aggregate_metric_double` has only one metric, with that metric.

We support the following scenarios:

- Single metric: we introduce the concept of a single metric, this is set only if the aggregate_metric_double has only one value. We saw this being used in tests when we aggregate using the min or max, but we did provide full support just in case it's used.
- Average is available: when aggregate_metric_double has the sum and value_count then we calculate the average. This is used in scripts, sorting and filtering queries.

Finally, we return no results in filtering, if none of the above apply.

Part of #128356
smalyshev pushed a commit to smalyshev/elasticsearch that referenced this pull request Feb 25, 2026
In this PR we replace the "derived metric" introduced in elastic#141877 with a script average or if the `aggregate_metric_double` has only one metric, with that metric.

We support the following scenarios:

- Single metric: we introduce the concept of a single metric, this is set only if the aggregate_metric_double has only one value. We saw this being used in tests when we aggregate using the min or max, but we did provide full support just in case it's used.
- Average is available: when aggregate_metric_double has the sum and value_count then we calculate the average. This is used in scripts, sorting and filtering queries.

Finally, we return no results in filtering, if none of the above apply.

Part of elastic#128356
elasticsearchmachine pushed a commit that referenced this pull request Mar 2, 2026
In #141877, we deprecated
the `default_metric` of an `aggregate_metric_double`. However, the
following tests (#142544,
#142964,
#142410 &
#142477) seem to
encounter this warning even if they are not expected to set the
`default_metric`. 

In this PR, we did the following: - For #142544, we added the api group
information in the warning failure exception, so that a yaml test will
not only log the warning but it will also share more info about the api
call. - For the other two, we catch the exception in
`FieldExtractorTestCase` and log the mapping of the index, we hope that
this can help us trace if there was a template used or not. - We also
unmute the test because we cannot reproduce them locally.
tballison pushed a commit to tballison/elasticsearch that referenced this pull request Mar 3, 2026
In elastic#141877, we deprecated
the `default_metric` of an `aggregate_metric_double`. However, the
following tests (elastic#142544,
elastic#142964,
elastic#142410 &
elastic#142477) seem to
encounter this warning even if they are not expected to set the
`default_metric`. 

In this PR, we did the following: - For elastic#142544, we added the api group
information in the warning failure exception, so that a yaml test will
not only log the warning but it will also share more info about the api
call. - For the other two, we catch the exception in
`FieldExtractorTestCase` and log the mapping of the index, we hope that
this can help us trace if there was a template used or not. - We also
unmute the test because we cannot reproduce them locally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants