-
Notifications
You must be signed in to change notification settings - Fork 8.5k
Streams landing page improvements #215629
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
Streams landing page improvements #215629
Conversation
…eams-landing-page-improvements
…eams-landing-page-improvements
…eams-landing-page-improvements
…eams-landing-page-improvements
…b.com/thomheymann/kibana into 180-streams-landing-page-improvements
flash1293
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good already!
Some nits:
It seems like a single bar isn't rendered in the histogram, not sure why - it seems like we have the same issue on the /app/streams/<stream name>/overview page as well. We can also solve it later, but we should definitely fix - the Lens chart in Discover doesn't have the same problem.
The loading state looks pretty blocky - not sure how to improve, but it feels like there are some low hanging fruits here (maybe something design can help with as well?):

I noticed that when switching around or using the filter, it's very easy to do the same ESQL query over and over again. We could think about a client side cache here, but it's probably overkill since these requests are fast and also cached on the Elasticsearch side. Something for a follow-up if even that.
| indexPattern, | ||
| numDataPoints, | ||
| }: { | ||
| timefilter: TimefilterHook; // Workaround to keep state in sync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this workaround? I guess alternatively we could call the hook in this compoent itself, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use the useTimerange hook more than once on the page since it uses local state and we need the time range to be in sync across the entire page.
I would suggest to create a Provider/hook that keeps the state in global context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They should all synchronize through the global timefilter service which is part of the data plugin (this is also what retains the current time range when moving from page to page within the streams UI):
kibana/src/platform/plugins/shared/data/public/query/timefilter/use_timefilter.ts
Line 53 in 86a7103
| timefilter.setTime(val); |
I'm not sure about the overhead though, a provider would work too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are only synchronised at the time filter service level.
The absolute time is set by the hook using local state so that is not in sync between different instances of the hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, the absolute time range would have small differences across the different visualizations, that is indeed weird. Passing it in works fine I guess - ideally the absolute time range should be controlled centrally in the service as well, but oh well.
| ); | ||
|
|
||
| const docCount = allTimeseries.reduce( | ||
| (acc, series) => acc + series.data.reduce((acc2, item) => acc2 + (item.doc_count || 0), 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't matter for the classic streams, but there is a detail here we need to think about for wired streams - should we show the count for the stream and its children or just for the current layer? @LucaWintergerst any opinions here?
x-pack/platform/plugins/shared/streams_app/public/components/stream_list_view/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/platform/plugins/shared/streams_app/public/components/streams_list/index.tsx
Outdated
Show resolved
Hide resolved
| onQuerySubmit={({ dateRange }, isUpdate) => { | ||
| if (dateRange && isUpdate) { | ||
| timefilter.setTimeRange(dateRange); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should refreshAbsoluteTimeRange be in an else branch here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. I want the absolute time range to be refreshed irregardless of whether the relative time range changed. For example when a user has now-15s selected, opens the time range picker and selects the same range again I think the histograms should refresh based on that selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setTimeRange is implicitly updating the absolute time range as well, right? It updates the timefilter service and that in turn updates the absolute time range here:
kibana/src/platform/plugins/shared/data/public/query/timefilter/use_timefilter.ts
Line 37 in 86a7103
| setAbsoluteTimeRange(() => timefilter.getAbsoluteTime()); |
Not sure whether it matters, just stood out to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hook is currently a half way house between both approaches. Consumers need to explicitly refresh the absolute time when some interaction happens but the relative time range did not change (e.g. when selecting the same time range again or when clicking the refresh button or enabling auto-refresh). Consumers do not need to do this however when selecting a different time range. In that case the absolute time is refreshed automatically. This is fairly confusing. The behaviour should be consistent. I think the easiest approach from a consumer perspective would be to handle each of these cases for the user implicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and created a hook with context provider that solves most of the issues with the current implementation. Might make sense for the streams app search bar component to integrate with that natively so that we don't need to manually set the callback methods everywhere but we can do that incrementally when all the different use cases are clearer.
|
@thomheymann You mentioned in Slack that the loading thing is delayed by 300ms - that's right, but the way it's used is that the old number doesn't stay around: Try to just hit refresh, and the old value will disappear immediately, then after 300ms the loading skeleton will appear. That's the jumpy behavior I was referring to. We should keep the old value in view until the skeleton is shown |
|
@thomheymann did you address this point somewhere? #215629 (comment) |
How would you expect this work instead? |
If the state switches to loading, the old value is still shown for 300ms. If the request returns within these 300ms, we update it right away, otherwise we go to the spinner. Right now if you change the time range, we immediately hide the old value and show an empty cell, then after 300ms we show the skeleton. If the value returns quickly that causes all numbers all charts to blink. I put up a version of what I mean here: thomheymann#2 You can go even further than that and make it nicer for edge cases, but there are diminishing returns IMHO - I was mostly triggered by the blinking. Current stateFast refresh time (everything blinks): Slow refresh time (show nothing for 300ms, then the skeleton): Keep old value for 300msFast refresh time (update everything in place): Slow refresh time (show old value for 300ms, then switch to skeleton, then to value): |
flash1293
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix with the chart, LGTM I think it would be good to address the loading thing in my last comment, but nothing beyond that blocking the PR
|
Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs) |
…eams-landing-page-improvements
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
The CI Stats report is too large to be displayed here, check out the CI build annotation for this information. History
|
|
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/14106909123 |
## Summary Streams landing page improvements: - Added table view for wired and classic streams - Added documents count and histogram column (loaded asynchronous) - Added time range picker to change view - Added effective retention policy column - Added pagination - Added granular loading states - Added empty state with link to onboarding page ## Screenshots   --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit 8a29087)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
# Backport This will backport the following commits from `main` to `8.x`: - [Streams landing page improvements (#215629)](#215629) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Thom Heymann","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-03-27T12:45:16Z","message":"Streams landing page improvements (#215629)\n\n## Summary\n\nStreams landing page improvements:\n\n- Added table view for wired and classic streams\n- Added documents count and histogram column (loaded asynchronous)\n- Added time range picker to change view\n- Added effective retention policy column\n- Added pagination\n- Added granular loading states\n- Added empty state with link to onboarding page\n\n## Screenshots\n\n\n\n\n\n\n\n---------\n\nCo-authored-by: kibanamachine <[email protected]>","sha":"8a29087d70a79a8f8bddddfe980c95e8163adbc5","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:obs-ux-logs","backport:version","Feature:Streams","v9.1.0","v8.19.0"],"title":"Streams landing page improvements","number":215629,"url":"https://github.com/elastic/kibana/pull/215629","mergeCommit":{"message":"Streams landing page improvements (#215629)\n\n## Summary\n\nStreams landing page improvements:\n\n- Added table view for wired and classic streams\n- Added documents count and histogram column (loaded asynchronous)\n- Added time range picker to change view\n- Added effective retention policy column\n- Added pagination\n- Added granular loading states\n- Added empty state with link to onboarding page\n\n## Screenshots\n\n\n\n\n\n\n\n---------\n\nCo-authored-by: kibanamachine <[email protected]>","sha":"8a29087d70a79a8f8bddddfe980c95e8163adbc5"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/215629","number":215629,"mergeCommit":{"message":"Streams landing page improvements (#215629)\n\n## Summary\n\nStreams landing page improvements:\n\n- Added table view for wired and classic streams\n- Added documents count and histogram column (loaded asynchronous)\n- Added time range picker to change view\n- Added effective retention policy column\n- Added pagination\n- Added granular loading states\n- Added empty state with link to onboarding page\n\n## Screenshots\n\n\n\n\n\n\n\n---------\n\nCo-authored-by: kibanamachine <[email protected]>","sha":"8a29087d70a79a8f8bddddfe980c95e8163adbc5"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Thom Heymann <[email protected]>
## Summary Streams landing page improvements: - Added table view for wired and classic streams - Added documents count and histogram column (loaded asynchronous) - Added time range picker to change view - Added effective retention policy column - Added pagination - Added granular loading states - Added empty state with link to onboarding page ## Screenshots   --------- Co-authored-by: kibanamachine <[email protected]>






Summary
Streams landing page improvements:
Screenshots