Summary of Improvements on aggregate_metric_double in 9.4#145742
Summary of Improvements on aggregate_metric_double in 9.4#145742gmarouli merged 18 commits intoelastic:mainfrom
9.4#145742Conversation
|
Hi @gmarouli, I've created a changelog YAML for you. Note that since this PR is labelled |
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
Hi @gmarouli, I've updated the changelog YAML for you. Note that since this PR is labelled |
|
@gmarouli docs eng had to some security work to harden the docs build pipelines, and a side effect is that when working from a fork, we now need your profile to be public to verify elastic org membership and then enable automatic URL previews of updated pages This is especially useful when we need to check that the applies_to tags render as expected :) You can do that simply by flipping to public here: https://github.com/orgs/elastic/people?query=gmarouli The alternative is to build locally: https://www.elastic.co/docs/contribute-docs/locally#install-docs-builder |
|
Preview from parameter deprecation: 
Preview from updating the usage: 
|
docs/changelog/145742.yaml
Outdated
| [source] | ||
| ---- | ||
| FROM k8s-downsampled | ||
| | STATS max = max(network.eth0.tx), std_dev = ROUND(STD_DEV(network.eth0.tx), 7) by pod |
There was a problem hiding this comment.
Nit: maybe skip round? It doesn't add much here.
leemthompo
left a comment
There was a problem hiding this comment.
Few writing and syntax things here from me :)
| : (Required, string) Default metric sub-field to use for queries, scripts, and aggregations that don’t use a sub-field. Must be a value from the `metrics` array. | ||
| `default_metric` {applies_to}`stack: deprecated 9.4` {applies_to}`serverless: deprecated` | ||
| : :::{admonition} Deprecated in 9.4 | ||
| The average value will be used as the default metric value, calculated using the `sum` and `value_count`, unless there is only a single sub-field, in which case it will use that metric. |
There was a problem hiding this comment.
Ideally we'd have a link to the detailed release note here
the structure of the deprecations page is deterministic, so could link to https://www.elastic.co/docs/release-notes/elasticsearch/deprecations initially and then update to a more targeted link once 9.4 is released or else just wait until release day to merge this update with the full link (https://www.elastic.co/docs/release-notes/elasticsearch/deprecations#elasticsearch-9.4.0-deprecations)
There was a problem hiding this comment.
Then we need a deprecation changelog as well, right?
There was a problem hiding this comment.
I updated the changelog of the PR that deprecated the default_metric to be a deprecation. This also helped narrow down the scope of the 145742.yaml which I think reads better now. WDYT?
docs/reference/elasticsearch/mapping-reference/aggregate-metric-double.md
Outdated
Show resolved
Hide resolved
kkrik-es
left a comment
There was a problem hiding this comment.
Stamping to get you going, please apply Liam's corrections too.
Co-authored-by: Kostas Krikellas <131142368+kkrik-es@users.noreply.github.com> Co-authored-by: Liam Thompson <leemthompo@gmail.com>
|
|
Hi @gmarouli, I've updated the changelog YAML for you. Note that since this PR is labelled |
|
Hi @gmarouli, I've updated the changelog YAML for you. Note that since this PR is labelled |
|
Some wording could be clarified IMO this sentence is a bit long: - The average value is used as the default metric value, calculated using the `sum` and `value_count`, unless there is only a single sub-field, in which case it uses that metric.
+ The default metric value depends on the number of sub-fields:
+
+ - When there are multiple sub-fields, the default is the average, calculated from `sum` and `value_count`.
+ - When there is only a single sub-field, that sub-field's value is used directly.I'd recommend commenting out the link to the deprecations page because it isn't live yet, by putting the link on its own line (outside of parentheses) and prepending |
There was a problem hiding this comment.
thanks for iterating, looks good to me with the latest diff suggestion I made :)
Co-authored-by: Liam Thompson <leemthompo@gmail.com>
|
Hi @gmarouli, I've updated the changelog YAML for you. Note that since this PR is labelled |
|
Thank you both for the good feedback! |
In this PR we update the docs with the improvements we accomplished for the
aggregate_metric_doubletype of fields.default_metricconfigurationmaxES|QLsupportaggregate_metric_doublefor non-native operations using the average, such asstd_dev.