Skip to content

[Aggs] Force return 0 on empty buckets on count if null flag is disabled#207308

Merged
dej611 merged 11 commits intoelastic:mainfrom
dej611:fix/206555
Feb 5, 2025
Merged

[Aggs] Force return 0 on empty buckets on count if null flag is disabled#207308
dej611 merged 11 commits intoelastic:mainfrom
dej611:fix/206555

Conversation

@dej611
Copy link
Contributor

@dej611 dej611 commented Jan 21, 2025

Summary

Fixes #206555

This PR is an attempt to address the null bucket issue with count in Lens formula via the emptyAsNull flag.

Checklist

Risks

This PR introduces potentially some breaking changes, as count null values, in particular coming from shifted computations, as now converted to 0 if the flag has been enabled.
This change is not news in the code base as other aggs like distinct_count or value_count already implements it, but not count.
Apparently no test failed with this change, I've also added new unit ones to freeze the current behaviour and detect future changes.

@dej611 dej611 added release_note:fix Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// Feature:Lens backport:prev-minor labels Jan 21, 2025
@dej611
Copy link
Contributor Author

dej611 commented Jan 21, 2025

/ci

@dej611 dej611 marked this pull request as ready for review January 21, 2025 15:33
@dej611 dej611 requested review from a team as code owners January 21, 2025 15:33
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@dej611 dej611 requested a review from ppisljar January 22, 2025 08:26
Copy link
Contributor

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Code & functionality LGTM

Copy link
Contributor

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

This doesn't fully solve what was requested in the original issue.
null/zero is one thing, but the fact that count with timeshift doesn't use the extended_bounds is not addressed by the PR.
Actually not just count but also other metrics doesn't include the extended_bounds when using timeshift.
This results is missing buckets at the beginning and end of the table if they are empty/zero.

@markov00
Copy link
Contributor

If I'm not wrong the current concept of emptyAsNull means: that if we find a bucket with a zero value, we will display it as null/missing.

 if (value === 0 && agg.params.emptyAsNull) {
        return null;
      }

But in this PR we are also doing the opposite: we are saying that null buckets can be shown as "empty" (actually zero) even if this wasn't requested in the issue and even not expressed by the parameter emptyAsNull.

@dej611
Copy link
Contributor Author

dej611 commented Jan 29, 2025

This doesn't fully solve what was requested in the original issue. null/zero is one thing, but the fact that count with timeshift doesn't use the extended_bounds is not addressed by the PR. Actually not just count but also other metrics doesn't include the extended_bounds when using timeshift. This results is missing buckets at the beginning and end of the table if they are empty/zero.

I do not think the problem is related with the extended_bounds.
All the aggs can return null values when there's no data, with few exceptions which are count, distinct_count, values_count and sum, who expose a specific flag emptyAsNull to enforce this behaviour: now this null behaviour has a side effect within a Lens formula context which is to wipe out other results within a bucket. To resolve this the defaults Lens formula function has been introduced.
Now, back to the exceptions, the 4 functions above, in the scenario where count was using a time_shift was not honouring the opposite case: return 0 by default when the flag was not enabled.
This PR addresses this specific case, which will solve the issue with count and time_shift.

If we want to extends this behaviour to other aggs as well I think we'll need to rethink the agg model and how they work.

dej611 and others added 2 commits January 29, 2025 11:25
…unt.test.ts

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
@dej611
Copy link
Contributor Author

dej611 commented Jan 30, 2025

Discussed offline with @markov00 this PR.
The linked issue #206555 has a wider scope, which is about forcing ES to return all the buckets in any scenario when using a time shift.
That issue can lead to some wrong results in some cases OR partial results where values should be 0 in others.
This PR provides a solution for specific scenario where the count with shift is used together with another metric in a Lens formula, which resolves the "wrong results" part in the issue above. Not the partial result section tho.

For the partial result we've tested a PoC who seems to work, but needs to be validated in a follow up PR.

Copy link
Contributor

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #113 / Rules Management - Rule Bulk Action API @ess perform_bulk_action - ESS specific logic should disable rules and migrate actions

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 423.6KB 423.6KB +36.0B

History

@dej611 dej611 merged commit 0d9ce86 into elastic:main Feb 5, 2025
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 9.0

https://github.com/elastic/kibana/actions/runs/13155950588

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 5, 2025
…led (elastic#207308)

## Summary

Fixes elastic#206555

This PR is an attempt to address the `null` bucket issue with `count` in
Lens formula via the `emptyAsNull` flag.

### Checklist

* [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### Risks

This PR introduces potentially some breaking changes, as count `null`
values, in particular coming from shifted computations, as now converted
to `0` if the flag has been enabled.
This change is not news in the code base as other aggs like
`distinct_count` or `value_count` already implements it, but not
`count`.
Apparently no test failed with this change, I've also added new unit
ones to freeze the current behaviour and detect future changes.

---------

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
Co-authored-by: Peter Pisljar <peter.pisljar@gmail.com>
(cherry picked from commit 0d9ce86)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
9.0

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Feb 5, 2025
… disabled (#207308) (#209737)

# Backport

This will backport the following commits from `main` to `9.0`:
- [[Aggs] Force return 0 on empty buckets on count if null flag is
disabled (#207308)](#207308)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Marco
Liberati","email":"dej611@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-02-05T11:04:09Z","message":"[Aggs]
Force return 0 on empty buckets on count if null flag is disabled
(#207308)\n\n## Summary\r\n\r\nFixes #206555 \r\n\r\nThis PR is an
attempt to address the `null` bucket issue with `count` in\r\nLens
formula via the `emptyAsNull` flag.\r\n\r\n### Checklist\r\n\r\n* [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n###
Risks\r\n\r\nThis PR introduces potentially some breaking changes, as
count `null`\r\nvalues, in particular coming from shifted computations,
as now converted\r\nto `0` if the flag has been enabled.\r\nThis change
is not news in the code base as other aggs like\r\n`distinct_count` or
`value_count` already implements it, but not\r\n`count`.\r\nApparently
no test failed with this change, I've also added new unit\r\nones to
freeze the current behaviour and detect future
changes.\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Vettorello
<vettorello.marco@gmail.com>\r\nCo-authored-by: Peter Pisljar
<peter.pisljar@gmail.com>","sha":"0d9ce86d0b81089598e8483d8e67bafa210a510b","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Visualizations","Feature:Lens","backport:prev-minor","v9.1.0"],"title":"[Aggs]
Force return 0 on empty buckets on count if null flag is
disabled","number":207308,"url":"https://github.com/elastic/kibana/pull/207308","mergeCommit":{"message":"[Aggs]
Force return 0 on empty buckets on count if null flag is disabled
(#207308)\n\n## Summary\r\n\r\nFixes #206555 \r\n\r\nThis PR is an
attempt to address the `null` bucket issue with `count` in\r\nLens
formula via the `emptyAsNull` flag.\r\n\r\n### Checklist\r\n\r\n* [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n###
Risks\r\n\r\nThis PR introduces potentially some breaking changes, as
count `null`\r\nvalues, in particular coming from shifted computations,
as now converted\r\nto `0` if the flag has been enabled.\r\nThis change
is not news in the code base as other aggs like\r\n`distinct_count` or
`value_count` already implements it, but not\r\n`count`.\r\nApparently
no test failed with this change, I've also added new unit\r\nones to
freeze the current behaviour and detect future
changes.\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Vettorello
<vettorello.marco@gmail.com>\r\nCo-authored-by: Peter Pisljar
<peter.pisljar@gmail.com>","sha":"0d9ce86d0b81089598e8483d8e67bafa210a510b"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/207308","number":207308,"mergeCommit":{"message":"[Aggs]
Force return 0 on empty buckets on count if null flag is disabled
(#207308)\n\n## Summary\r\n\r\nFixes #206555 \r\n\r\nThis PR is an
attempt to address the `null` bucket issue with `count` in\r\nLens
formula via the `emptyAsNull` flag.\r\n\r\n### Checklist\r\n\r\n* [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n###
Risks\r\n\r\nThis PR introduces potentially some breaking changes, as
count `null`\r\nvalues, in particular coming from shifted computations,
as now converted\r\nto `0` if the flag has been enabled.\r\nThis change
is not news in the code base as other aggs like\r\n`distinct_count` or
`value_count` already implements it, but not\r\n`count`.\r\nApparently
no test failed with this change, I've also added new unit\r\nones to
freeze the current behaviour and detect future
changes.\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Vettorello
<vettorello.marco@gmail.com>\r\nCo-authored-by: Peter Pisljar
<peter.pisljar@gmail.com>","sha":"0d9ce86d0b81089598e8483d8e67bafa210a510b"}}]}]
BACKPORT-->

Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
drewdaemon pushed a commit to drewdaemon/kibana that referenced this pull request Feb 6, 2025
…led (elastic#207308)

## Summary

Fixes elastic#206555 

This PR is an attempt to address the `null` bucket issue with `count` in
Lens formula via the `emptyAsNull` flag.

### Checklist

* [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### Risks

This PR introduces potentially some breaking changes, as count `null`
values, in particular coming from shifted computations, as now converted
to `0` if the flag has been enabled.
This change is not news in the code base as other aggs like
`distinct_count` or `value_count` already implements it, but not
`count`.
Apparently no test failed with this change, I've also added new unit
ones to freeze the current behaviour and detect future changes.

---------

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
Co-authored-by: Peter Pisljar <peter.pisljar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Lens release_note:fix Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v9.0.0 v9.1.0

6 participants