Skip to content

[Infrastructure UI] Filter out null bucket items from average calculation#152333

Merged
crespocarlos merged 5 commits intoelastic:mainfrom
crespocarlos:152328-average-calculation-fix
Mar 1, 2023
Merged

[Infrastructure UI] Filter out null bucket items from average calculation#152333
crespocarlos merged 5 commits intoelastic:mainfrom
crespocarlos:152328-average-calculation-fix

Conversation

@crespocarlos
Copy link
Contributor

@crespocarlos crespocarlos commented Feb 28, 2023

Summary

Closes #152328

This PR fixes the average calculation in the Snapshot API, filtering out buckets with null values from it, which are more likely to appear with queries that use small data ranges.

The results after this change are equal to what Elasticsearch would calculate in the avg aggregation

How to test

  • Make sure you have metrics data (either through enabling the system module in metricbeat or connecting your local kibana to an oblt-cli cluster)
  • Navigate to Infrastructure > Hosts
  • Filter the results to see a single host
  • Change the data range filter and compare the KPIs against the table.
  • Validate other pages that use the Snapshot API (Inventory UI and Metrics UI to see if the results there are still correct

@crespocarlos crespocarlos added Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor v8.7.0 v8.8.0 Feature:ObsHosts Hosts feature within Observability labels Feb 28, 2023
@crespocarlos crespocarlos force-pushed the 152328-average-calculation-fix branch from d450c85 to 61a3dd9 Compare February 28, 2023 11:34
value: null,
max: 0.47105555555555556,
avg: 0.0672936507936508,
avg: 0.47105555555555556,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what the query returns:

[{ timestamp: 1562786355035, metric_0: null },
{ timestamp: 1562786415035, metric_0: null },
{ timestamp: 1562786475035, metric_0: null },
{ timestamp: 1562786535035, metric_0: null },
{ timestamp: 1562786595035, metric_0: null },
{ timestamp: 1562786655035, metric_0: 0.47105555555555556 },
{ timestamp: 1562786715035, metric_0: null }]

So, if we're filtering out null values from the average calculation, it makes sense for the expected result to be 0.47105555555555556

@crespocarlos crespocarlos marked this pull request as ready for review February 28, 2023 16:01
@crespocarlos crespocarlos requested a review from a team as a code owner February 28, 2023 16:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)


const calculateAvg = (rows: MetricsAPIRow[]): number => {
return sum(rows.map(getMetricValue)) / rows.length || 0;
const values = rows.map(getMetricValue).filter(Boolean);
Copy link
Contributor

Choose a reason for hiding this comment

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

doubt: not sure if this scenario is possible, but in case the metric value is 0, this will be filtered out and wrongly compute as the expected value.
For example

[0, 0, 9] // expected (0 + 0 + 9) / 3 === 3 ✅

but with the current .filter(Boolean) it would result in

[0, 0, 9].filter(Boolean) // expected 9 / 1 === 9 ❌

It may be safer to only filter out only non-numeric values

[0, 0, 9].filter(isFinite) 

Copy link
Contributor Author

@crespocarlos crespocarlos Mar 1, 2023

Choose a reason for hiding this comment

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

This is absolutely correct. Thanks for catching it.

[null,0, 0, 9].filter(Number.isFinite) -> [0,0,9]

Copy link
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

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

I'm not sure why, but it looks like the values between the KPI tiles and the table are still mismatching on different time-range selections, here are some examples:

Time range Screenshot Avg RX matches? Avg TX matches?
Last 5 mins Screenshot 2023-03-01 at 09 29 36
Last 10 mins Screenshot 2023-03-01 at 09 29 53
Last 15 mins Screenshot 2023-03-01 at 09 30 07
Last 20 mins Screenshot 2023-03-01 at 09 30 27
Last 30 mins Screenshot 2023-03-01 at 09 31 01

@crespocarlos
Copy link
Contributor Author

crespocarlos commented Mar 1, 2023

I'm not sure why, but it looks like the values between the KPI tiles and the table are still mismatching on different time-range selections, here are some examples:

Yeah. My tests were clearly biased and didn't catch these big differences. Thanks for thoroughly testing this.

Pushing another change to mitigate it:

image

image

image

image

image

I believe this fix won't solve all scenarios, though. e.g: querying for the last 15m in a real environment might possibly return different values because both KPI and table queries run at different times.

Absolute time ranges should always return the same values, in theory. But they run different queries (one with a date histogram and another without it), so it's possible that small differences can be seen.

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for applying the changes :)

@kibana-ci
Copy link

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #33 / saved objects tagging - functional tests visualize integration editing allows to assign tags to an existing visualization

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 1.3MB 1.3MB +170.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 428 430 +2

Total ESLint disabled count

id before after diff
securitySolution 506 508 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@crespocarlos crespocarlos merged commit f6a0b88 into elastic:main Mar 1, 2023
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Mar 1, 2023
…tion (elastic#152333)

## Summary

Closes [elastic#152328](elastic#152328)

This PR fixes the average calculation in the Snapshot API, filtering out
buckets with null values from it, which are more likely to appear with
queries that use small data ranges.

The results after this change are equal to what Elasticsearch would
calculate in the avg aggregation

### How to test

- Make sure you have metrics data (either through enabling the system
module in metricbeat or connecting your local kibana to an oblt-cli
cluster)
- Navigate to `Infrastructure` > `Hosts`
- Filter the results to see a single host
- Change the data range filter and compare the KPIs against the table.
- Validate other pages that use the Snapshot API (Inventory UI and
Metrics UI to see if the results there are still correct

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit f6a0b88)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.7

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 Mar 1, 2023
…alculation (#152333) (#152461)

# Backport

This will backport the following commits from `main` to `8.7`:
- [[Infrastructure UI] Filter out null bucket items from average
calculation (#152333)](#152333)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT [{"author":{"name":"Carlos
Crespo","email":"crespocarlos@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-03-01T14:31:35Z","message":"[Infrastructure
UI] Filter out null bucket items from average calculation
(#152333)\n\n## Summary\r\n\r\nCloses
[#152328](https://github.com/elastic/kibana/issues/152328)\r\n\r\nThis
PR fixes the average calculation in the Snapshot API, filtering
out\r\nbuckets with null values from it, which are more likely to appear
with\r\nqueries that use small data ranges.\r\n\r\nThe results after
this change are equal to what Elasticsearch would\r\ncalculate in the
avg aggregation\r\n\r\n\r\n### How to test \r\n\r\n- Make sure you have
metrics data (either through enabling the system\r\nmodule in metricbeat
or connecting your local kibana to an oblt-cli\r\ncluster)\r\n- Navigate
to `Infrastructure` > `Hosts`\r\n- Filter the results to see a single
host\r\n- Change the data range filter and compare the KPIs against the
table.\r\n- Validate other pages that use the Snapshot API (Inventory UI
and\r\nMetrics UI to see if the results there are still
correct\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"f6a0b886b143dea6b3b6f8101e21be5c1816558a","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Metrics
UI","Team:Infra Monitoring
UI","release_note:skip","backport:prev-minor","v8.7.0","Feature:ObsHosts","v8.8.0"],"number":152333,"url":"https://github.com/elastic/kibana/pull/152333","mergeCommit":{"message":"[Infrastructure
UI] Filter out null bucket items from average calculation
(#152333)\n\n## Summary\r\n\r\nCloses
[#152328](https://github.com/elastic/kibana/issues/152328)\r\n\r\nThis
PR fixes the average calculation in the Snapshot API, filtering
out\r\nbuckets with null values from it, which are more likely to appear
with\r\nqueries that use small data ranges.\r\n\r\nThe results after
this change are equal to what Elasticsearch would\r\ncalculate in the
avg aggregation\r\n\r\n\r\n### How to test \r\n\r\n- Make sure you have
metrics data (either through enabling the system\r\nmodule in metricbeat
or connecting your local kibana to an oblt-cli\r\ncluster)\r\n- Navigate
to `Infrastructure` > `Hosts`\r\n- Filter the results to see a single
host\r\n- Change the data range filter and compare the KPIs against the
table.\r\n- Validate other pages that use the Snapshot API (Inventory UI
and\r\nMetrics UI to see if the results there are still
correct\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"f6a0b886b143dea6b3b6f8101e21be5c1816558a"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/152333","number":152333,"mergeCommit":{"message":"[Infrastructure
UI] Filter out null bucket items from average calculation
(#152333)\n\n## Summary\r\n\r\nCloses
[#152328](https://github.com/elastic/kibana/issues/152328)\r\n\r\nThis
PR fixes the average calculation in the Snapshot API, filtering
out\r\nbuckets with null values from it, which are more likely to appear
with\r\nqueries that use small data ranges.\r\n\r\nThe results after
this change are equal to what Elasticsearch would\r\ncalculate in the
avg aggregation\r\n\r\n\r\n### How to test \r\n\r\n- Make sure you have
metrics data (either through enabling the system\r\nmodule in metricbeat
or connecting your local kibana to an oblt-cli\r\ncluster)\r\n- Navigate
to `Infrastructure` > `Hosts`\r\n- Filter the results to see a single
host\r\n- Change the data range filter and compare the KPIs against the
table.\r\n- Validate other pages that use the Snapshot API (Inventory UI
and\r\nMetrics UI to see if the results there are still
correct\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"f6a0b886b143dea6b3b6f8101e21be5c1816558a"}}]}]
BACKPORT-->

Co-authored-by: Carlos Crespo <crespocarlos@users.noreply.github.com>
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
…tion (elastic#152333)

## Summary

Closes [elastic#152328](elastic#152328)

This PR fixes the average calculation in the Snapshot API, filtering out
buckets with null values from it, which are more likely to appear with
queries that use small data ranges.

The results after this change are equal to what Elasticsearch would
calculate in the avg aggregation


### How to test 

- Make sure you have metrics data (either through enabling the system
module in metricbeat or connecting your local kibana to an oblt-cli
cluster)
- Navigate to `Infrastructure` > `Hosts`
- Filter the results to see a single host
- Change the data range filter and compare the KPIs against the table.
- Validate other pages that use the Snapshot API (Inventory UI and
Metrics UI to see if the results there are still correct

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Metrics UI Metrics UI feature Feature:ObsHosts Hosts feature within Observability release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.7.0 v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Infrastructure UI] Mismatch between table and KPI averages for RX and TX

5 participants