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

[ObsUX][Profiling, Infra] Show "No Data" messages when there is no profiling data #173633

Merged

Conversation

mykolaharmash
Copy link
Contributor

@mykolaharmash mykolaharmash commented Dec 19, 2023

Closes #173153

Summary

Adds a message when there is no data for flamegraph or top functions.

CleanShot.2023-12-19.at.15.05.04.mp4

Copy link
Contributor

Documentation preview:

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@mykolaharmash mykolaharmash added release_note:skip Skip the PR/issue when compiling release notes v8.12.0 Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Dec 19, 2023
@mykolaharmash mykolaharmash changed the title 173153 profiling no data messages [ObsUX][Profiling, Infra] Show "No Data" messages when there is no profiling data Dec 19, 2023
@mykolaharmash mykolaharmash force-pushed the 173153-profiling-no-data-messages branch from 065791c to cd686fe Compare December 20, 2023 08:42
@mykolaharmash mykolaharmash marked this pull request as ready for review December 20, 2023 08:43
@mykolaharmash mykolaharmash requested a review from a team as a code owner December 20, 2023 08:43
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

@crespocarlos
Copy link
Contributor

Hey! After watching the screen recording, I noticed that when switching tabs, the components re-render, but the date-picker remains unchanged. Are we re-fetching the data every time the components become visible?

@cauemarcondes
Copy link
Contributor

Hey! After watching the screen recording, I noticed that when switching tabs, the components re-render, but the date-picker remains unchanged. Are we re-fetching the data every time the components become visible?

I took a look at this, and this is indeed happening. But, the response is cached by the browser. So even though it shows the loading spinner for a short amount of time, the response comes very fast compared with the first call that was not cached.

@mykolaharmash
Copy link
Contributor Author

Hey! After watching the screen recording, I noticed that when switching tabs, the components re-render, but the date-picker remains unchanged. Are we re-fetching the data every time the components become visible?

I took a look at this, and this is indeed happening. But, the response is cached by the browser. So even though it shows the loading spinner for a short amount of time, the response comes very fast compared with the first call that was not cached.

Hey @crespocarlos! @cauemarcondes is correct, I've decided to rely on browser caching, in this case, to make the frontend implementation simpler. In order to do this in one of the previous PRs I've switched profiling endpoints to use GET and include a caching header. The flashing spinner is a bit annoying, but there are ways to mitigate it (like throttling the spinner visibility) if it becomes an issue.

@mykolaharmash
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@mykolaharmash
Copy link
Contributor Author

@elasticmachine merge upstream

@mykolaharmash
Copy link
Contributor Author

Hey! After watching the screen recording, I noticed that when switching tabs, the components re-render, but the date-picker remains unchanged. Are we re-fetching the data every time the components become visible?

I took a look at this, and this is indeed happening. But, the response is cached by the browser. So even though it shows the loading spinner for a short amount of time, the response comes very fast compared with the first call that was not cached.

Hey @crespocarlos! @cauemarcondes is correct, I've decided to rely on browser caching, in this case, to make the frontend implementation simpler. In order to do this in one of the previous PRs I've switched profiling endpoints to use GET and include a caching header. The flashing spinner is a bit annoying, but there are ways to mitigate it (like throttling the spinner visibility) if it becomes an issue.

Went digging on this topic a bit and turns out there is a Cache Storage API, never heard of it before. This could be another way to eliminate flashing spinners, we could use a custom cache for the requests and then check if a particular URL is already in the cache and decide whether to show the spinner. There are so many cool APIs in the browser these days 🤯

@crespocarlos
Copy link
Contributor

Went digging on this topic a bit and turns out there is a Cache Storage API, never heard of it before. This could be another way to eliminate flashing spinners, we could use a custom cache for the requests and then check if a particular URL is already in the cache and decide whether to show the spinner. There are so many cool APIs in the browser these days 🤯

Nice! There's a bunch of new browser stuff I need to catch up on too. I'll have a look at the cache storage. Thanks for sharing it.

Another alternative is to use react to cache the content as we did in Asset Details the tabbed content: https://github.com/elastic/kibana/blob/main/x-pack/plugins/infra/public/components/asset_details/content/content.tsx#L66

@mykolaharmash mykolaharmash force-pushed the 173153-profiling-no-data-messages branch from e90b153 to 0a62ffa Compare January 2, 2024 14:59
@mykolaharmash
Copy link
Contributor Author

mykolaharmash commented Jan 2, 2024

Just fyi, forced-push trying to unblock the CI, no code changes were made. But it seems like CI issue is not related to this PR and is a wider problem.

@mykolaharmash
Copy link
Contributor Author

Another alternative is to use react to cache the content as we did in Asset Details the tabbed content: https://github.com/elastic/kibana/blob/main/x-pack/plugins/infra/public/components/asset_details/content/content.tsx#L66

Right, I considered caching rendered nodes at first, but then thought that the main bottleneck in this case is not actually rendering but the network, so tried to address it first. For now, it seems like it produces an acceptable result without any extra logic on the FE, kind of for free.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1330 1331 +1

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 +643.0B

History

  • 💔 Build #185423 failed 21a79cfe8fa14999b22c169b12177da1a0658f06
  • 💚 Build #185395 succeeded e90b153e2e6980b2691516dab28f17a760cff9a0
  • 💚 Build #185322 succeeded cb507beb5c9f20831587712f75a26cd4653577ed
  • 💚 Build #185136 succeeded 1c9b6915c3730c2753045cb790886a7d9f406ce3

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

@mykolaharmash mykolaharmash merged commit 9215e17 into elastic:main Jan 3, 2024
21 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 3, 2024
…ofiling data (elastic#173633)

Closes elastic#173153

## Summary

Adds a message when there is no data for flamegraph or top functions.

https://github.com/elastic/kibana/assets/793851/2a6158ca-86d3-4b23-9807-dc177ce0361b
(cherry picked from commit 9215e17)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.12

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 Jan 3, 2024
…en there is no profiling data (#173633) (#174163)

# Backport

This will backport the following commits from `main` to `8.12`:
- [[ObsUX][Profiling, Infra] Show "No Data" messages when
there is no profiling data
(#173633)](#173633)

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

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

<!--BACKPORT [{"author":{"name":"Mykola
Harmash","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-01-03T13:13:09Z","message":"[ObsUX][Profiling,
Infra] Show \"No Data\" messages when there is no profiling data
(#173633)\n\nCloses
https://github.com/elastic/kibana/issues/173153\r\n\r\n##
Summary\r\n\r\nAdds a message when there is no data for flamegraph or
top
functions.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/793851/2a6158ca-86d3-4b23-9807-dc177ce0361b","sha":"9215e17cb7c9ce49e7b9347ea578ec363d50dd8e","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v8.12.0","Team:obs-ux-infra_services","v8.13.0"],"title":"[ObsUX][Profiling,
Infra] Show \"No Data\" messages when there is no profiling
data","number":173633,"url":"https://github.com/elastic/kibana/pull/173633","mergeCommit":{"message":"[ObsUX][Profiling,
Infra] Show \"No Data\" messages when there is no profiling data
(#173633)\n\nCloses
https://github.com/elastic/kibana/issues/173153\r\n\r\n##
Summary\r\n\r\nAdds a message when there is no data for flamegraph or
top
functions.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/793851/2a6158ca-86d3-4b23-9807-dc177ce0361b","sha":"9215e17cb7c9ce49e7b9347ea578ec363d50dd8e"}},"sourceBranch":"main","suggestedTargetBranches":["8.12"],"targetPullRequestStates":[{"branch":"8.12","label":"v8.12.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.13.0","branchLabelMappingKey":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/173633","number":173633,"mergeCommit":{"message":"[ObsUX][Profiling,
Infra] Show \"No Data\" messages when there is no profiling data
(#173633)\n\nCloses
https://github.com/elastic/kibana/issues/173153\r\n\r\n##
Summary\r\n\r\nAdds a message when there is no data for flamegraph or
top
functions.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/793851/2a6158ca-86d3-4b23-9807-dc177ce0361b","sha":"9215e17cb7c9ce49e7b9347ea578ec363d50dd8e"}}]}]
BACKPORT-->

Co-authored-by: Mykola Harmash <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.12.0 v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ObsUX][Profiling, Infra] Display "No data found" message instead of empty components
7 participants