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

[DataUsage][Serverless] Data usage charts enhancements #196559

Conversation

ashokaditya
Copy link
Member

@ashokaditya ashokaditya commented Oct 16, 2024

Summary

follow up of:

Adds a lot of enhancements to the datastream dropdown including:

  • shows storage sizes on the data stream dropdown
  • preselects all data streams on the first page load
  • updates selected data streams to URL params
  • selects data streams based on URL load
  • doesn't allow deselecting all data streams
  • cancels older API requests

screen

Screenshot 2024-10-16 at 16 57 43

clip

metrics-ux-16-10

related PRs

Checklist

@neptunian
Copy link
Contributor

neptunian commented Oct 17, 2024

Whenever i select or unselect a data stream I get two requests to /metrics with the same payload. I'm also still getting the initial 400 error when i load the page due to sending the empty data streams array.

@ashokaditya ashokaditya force-pushed the feat/data-usage-charts-enhancements-192965-192966 branch from 6cc7453 to 2f56a29 Compare October 17, 2024 14:22
@ashokaditya
Copy link
Member Author

Whenever i select or unselect a data stream I get two requests to /metrics with the same payload. I'm also still getting the initial 400 error when i load the page due to sending the empty data streams array.

yeah, I fixed that as well.

@ashokaditya ashokaditya force-pushed the feat/data-usage-charts-enhancements-192965-192966 branch from b8b3160 to c7b16e7 Compare October 17, 2024 15:37
@neptunian
Copy link
Contributor

neptunian commented Oct 17, 2024

Looks great! I do still get the issue where if I select a data stream (not deselect), i get two of the same metrics requests/responses. It doesn't do it if I'm only selecting a total of 1 data streams, though (from zero).

@ashokaditya
Copy link
Member Author

ashokaditya commented Oct 17, 2024

Looks great! I do still get the issue where if I select a data stream (not deselect), i get two of the same metrics requests/responses. It doesn't do it if I'm only selecting a total of 1 data streams, though (from zero).

Oh right. I see that too. It's cause the list is being sorted and that is a new query key so it makes a new request. fixed in 6cd61ba

@neptunian
Copy link
Contributor

neptunian commented Oct 17, 2024

We can do this in a followup - but I'm thinking there is no reason to make the /metrics request when removing a datastream from the filter dropdown. We aren't getting any new data unless we are adding a datastream or the time range has changed. Given we default by selecting all, if a user wants to remove several data streams, making requests for no reason and causing the charts to have to wait and re render would not be a great experience.

@neptunian
Copy link
Contributor

neptunian commented Oct 17, 2024

It was easy for me to create many requests (and likely race conditions) by selecting or deselecting data streams from the dropdown. Can we make sure to abort any inflight requests when another request happens to avoid race conditions?

@ashokaditya
Copy link
Member Author

It was easy for me to create many requests (and likely race conditions) by selecting or deselecting or some combination of data streams from the dropdown. Can we make sure to abort any inflight requests when another request happens to avoid race conditions?

Not sure I follow. Are you saying that you were able to create parallel requests? Do you have a screen clip to illustrate what you mean by race conditions you're seeing on your end. We create a metrics API request every time we change the datastreams list or date ranges. That said, we can ensure that we don't make a metrics API request if we're also fetching a list of data streams.

@neptunian
Copy link
Contributor

neptunian commented Oct 17, 2024

Are you saying that you were able to create parallel requests?

yes

Use a settimeout or throttle your connection and click on and off the the data streams:

Screenshot 2024-10-17 at 3 36 01 PM

If you have a look at other fetcher hooks they usually use the AbortController to avoid this.

@ashokaditya
Copy link
Member Author

Are you saying that you were able to create parallel requests?

yes

Use a settimeout or throttle your connection and click on and off the the data streams:

Ah I see what you mean. Fixed in 47d22ee

@ashokaditya
Copy link
Member Author

We can do this in a followup - but I'm thinking there is no reason to make the /metrics request when removing a datastream from the filter dropdown.

I think we should allow users to see less data for fewer indices/datastreams should they like. They might want to see fewer indices on the charts instead of being overwhelmed by looking at data for several at a time.

We aren't getting any new data unless we are adding a datastream or the time range has changed.

I would expect to get different or less data based on changes to time-range and which data streams are selected. Is that not the case with the auto-ops API?

Given we default by selecting all, if a user wants to remove several data streams, making requests for no reason and causing the charts to have to wait and re-render would not be a great experience.

Yeah, we need to make the chart UX better. I would expect to see chart grids while data is loading for the first time. On subsequent requests, I would expect the chart to be updated if there's a delta. I imagine the bars changing in height/frequency on subsequent requests. What do you think? @neptunian @YulNaumenko

@ashokaditya ashokaditya force-pushed the feat/data-usage-charts-enhancements-192965-192966 branch from 92e2c2d to c99ad95 Compare October 18, 2024 09:10
@neptunian
Copy link
Contributor

neptunian commented Oct 18, 2024

I think we should allow users to see less data for fewer indices/datastreams should they like. They might want to see fewer indices on the charts instead of being overwhelmed by looking at data for several at a time.

I agree. My question is why do we need to make a request to /metrics when they remove data streams? We are not getting any new data in that case? We need to update the UI state but we don't need to make a request. If we didn't have to make a request as they removed data streams the chart would update immediately instead of having to wait for the request. This is a performance optimization I'm getting at and it adds a little bit of complexity. I mainly mention it because this request can take some time due to the external call - so it may be worth it.

return http.post<UsageMetricsResponseSchemaBody>(DATA_USAGE_METRICS_API_ROUTE, {
signal,
Copy link
Contributor

Choose a reason for hiding this comment

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

wow, that was easy 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, one of many reasons a lot of us moved away from redux to manage server state and use tanstack-query.

@ashokaditya
Copy link
Member Author

I think we should allow users to see less data for fewer indices/datastreams should they like. They might want to see fewer indices on the charts instead of being overwhelmed by looking at data for several at a time.

I agree. My question is why do we need to make a request to /metrics when they remove data streams? We are not getting any new data in that case? We need to update the UI state but we don't need to make a request. If we didn't have to make a request as they removed data streams the chart would update immediately instead of having to wait for the request. This is a performance optimization I'm getting at and it adds a little bit of complexity. I mainly mention it because this request can take some time due to the external call - so it may be worth it.

Ah, I see what you mean by improving performance. That makes sense if we can correctly assume that "there's no new data" meaning non of the metrics data changes after the intiial request.

I would expect that ingestion rate and storage rate is in fact constantly changing while (any arbitrary of time) users are looking at the charts.

@neptunian
Copy link
Contributor

I would expect that ingestion rate and storage rate is in fact constantly changing while (any arbitrary of time) users are looking at the charts.

That's true. I was thinking that so long as the time range didn't change, nothing would change, but since we default to a relative time range, it's probably an unlikely scenario and not worth pursuing.

@ashokaditya
Copy link
Member Author

I would expect that ingestion rate and storage rate is in fact constantly changing while (any arbitrary of time) users are looking at the charts.

That's true. I was thinking that so long as the time range didn't change, nothing would change, but since we default to a relative time range, it's probably an unlikely scenario and not worth pursuing.

Oh right. Thanks for clarifying. Yeah if the end date is not set to now then it makes sense to optimize requests. I can try and test it out when I work on the new PR.

@ashokaditya ashokaditya enabled auto-merge (squash) October 18, 2024 13:01
@ashokaditya ashokaditya merged commit 13e19cb into elastic:main Oct 18, 2024
33 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16

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

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #19 / useGetCaseUserActionsStats returns proper state on getCaseUserActionsStats

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
dataUsage 237.8KB 237.6KB -230.0B

Page load bundle

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

id before after diff
dataUsage 4.6KB 4.5KB -54.0B

History

cc @ashokaditya

@ashokaditya ashokaditya deleted the feat/data-usage-charts-enhancements-192965-192966 branch October 18, 2024 13:13
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 18, 2024
## Summary

follow up of:
- elastic/pull/195556

Adds a lot of enhancements to the datastream dropdown including:

- [x] shows storage sizes on the data stream dropdown
- [x] preselects all data streams on the first page load
- [x] updates selected data streams to URL params
- [x] selects data streams based on URL load
- [x] doesn't allow deselecting all data streams
- [x] cancels older API requests

### screen
![Screenshot 2024-10-16 at 16 57
43](https://github.com/user-attachments/assets/38db2d93-f531-4269-88ea-51b4926b6a72)

### clip

![metrics-ux-16-10](https://github.com/user-attachments/assets/7913d1b6-31df-48e6-a3a9-f4dad0dc1b1e)

related PRs
- elastic/pull/193966

### Checklist
- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ] [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit 13e19cb)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.16

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 Oct 18, 2024
… (#196876)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[DataUsage][Serverless] Data usage charts enhancements
(#196559)](#196559)

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

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

<!--BACKPORT
[{"author":{"name":"Ash","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-18T13:10:30Z","message":"[DataUsage][Serverless]
Data usage charts enhancements (#196559)\n\n## Summary\r\n\r\nfollow up
of:\r\n- /pull/195556\r\n\r\nAdds a lot of enhancements to
the datastream dropdown including:\r\n\r\n- [x] shows storage sizes on
the data stream dropdown\r\n- [x] preselects all data streams on the
first page load\r\n- [x] updates selected data streams to URL
params\r\n- [x] selects data streams based on URL load\r\n- [x] doesn't
allow deselecting all data streams\r\n- [x] cancels older API
requests\r\n\r\n### screen\r\n![Screenshot 2024-10-16 at 16
57\r\n43](https://github.com/user-attachments/assets/38db2d93-f531-4269-88ea-51b4926b6a72)\r\n\r\n###
clip\r\n\r\n![metrics-ux-16-10](https://github.com/user-attachments/assets/7913d1b6-31df-48e6-a3a9-f4dad0dc1b1e)\r\n\r\nrelated
PRs\r\n- /pull/193966 \r\n \r\n### Checklist\r\n- [x] Any
text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[ ] [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- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] Any UI touched in this PR does
not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[x] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[x] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"13e19cb645e3e3b037ea40809dfbfdaf93529169","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","v8.16.0","backport:version"],"title":"[DataUsage][Serverless]
Data usage charts
enhancements","number":196559,"url":"https://github.com/elastic/kibana/pull/196559","mergeCommit":{"message":"[DataUsage][Serverless]
Data usage charts enhancements (#196559)\n\n## Summary\r\n\r\nfollow up
of:\r\n- /pull/195556\r\n\r\nAdds a lot of enhancements to
the datastream dropdown including:\r\n\r\n- [x] shows storage sizes on
the data stream dropdown\r\n- [x] preselects all data streams on the
first page load\r\n- [x] updates selected data streams to URL
params\r\n- [x] selects data streams based on URL load\r\n- [x] doesn't
allow deselecting all data streams\r\n- [x] cancels older API
requests\r\n\r\n### screen\r\n![Screenshot 2024-10-16 at 16
57\r\n43](https://github.com/user-attachments/assets/38db2d93-f531-4269-88ea-51b4926b6a72)\r\n\r\n###
clip\r\n\r\n![metrics-ux-16-10](https://github.com/user-attachments/assets/7913d1b6-31df-48e6-a3a9-f4dad0dc1b1e)\r\n\r\nrelated
PRs\r\n- /pull/193966 \r\n \r\n### Checklist\r\n- [x] Any
text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[ ] [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- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] Any UI touched in this PR does
not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[x] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[x] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"13e19cb645e3e3b037ea40809dfbfdaf93529169"}},"sourceBranch":"main","suggestedTargetBranches":["8.16"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196559","number":196559,"mergeCommit":{"message":"[DataUsage][Serverless]
Data usage charts enhancements (#196559)\n\n## Summary\r\n\r\nfollow up
of:\r\n- /pull/195556\r\n\r\nAdds a lot of enhancements to
the datastream dropdown including:\r\n\r\n- [x] shows storage sizes on
the data stream dropdown\r\n- [x] preselects all data streams on the
first page load\r\n- [x] updates selected data streams to URL
params\r\n- [x] selects data streams based on URL load\r\n- [x] doesn't
allow deselecting all data streams\r\n- [x] cancels older API
requests\r\n\r\n### screen\r\n![Screenshot 2024-10-16 at 16
57\r\n43](https://github.com/user-attachments/assets/38db2d93-f531-4269-88ea-51b4926b6a72)\r\n\r\n###
clip\r\n\r\n![metrics-ux-16-10](https://github.com/user-attachments/assets/7913d1b6-31df-48e6-a3a9-f4dad0dc1b1e)\r\n\r\nrelated
PRs\r\n- /pull/193966 \r\n \r\n### Checklist\r\n- [x] Any
text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[ ] [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- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] Any UI touched in this PR does
not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[x] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[x] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"13e19cb645e3e3b037ea40809dfbfdaf93529169"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Ash <[email protected]>
@jbudz
Copy link
Member

jbudz commented Oct 18, 2024

@ashokaditya there's a version gap with this backport. Can you check if this should also be backported to 8.x/8.17?

@ashokaditya
Copy link
Member Author

@ashokaditya there's a version gap with this backport. Can you check if this should also be backported to 8.x/8.17?

yeah it should be backported to 8.x/8.17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes v8.16.0 v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants