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

[Logs UI] Refactor log stream rendering #53044

Closed
Zacqary opened this issue Dec 13, 2019 · 3 comments
Closed

[Logs UI] Refactor log stream rendering #53044

Zacqary opened this issue Dec 13, 2019 · 3 comments
Assignees
Labels
Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services

Comments

@Zacqary
Copy link
Contributor

Zacqary commented Dec 13, 2019

Sub-issue of #49154

Acceptance Criteria

  • Use react-virtualized to render the log stream rows
  • Investigate and implement a more performant way of determining row height and scroll position
@Zacqary Zacqary added [zube]: Ready Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Dec 13, 2019
@Zacqary Zacqary self-assigned this Dec 13, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@Zacqary
Copy link
Contributor Author

Zacqary commented Jan 8, 2020

Update on where we're at right now:

  • I'm using react-window, a newer successor to react-virtualized
  • The exact height of each element needs to be calculated and passed into the react-window component. Rather than calling an expensive getBoundingRect for every single log entry, I've developed a system to measure the character size, the width of the log entry message column, and then use these plus the actual length of the string to determine how tall to make each log entry
  • I think it's safe to assume that the log message field is always going to be the tallest item? This doesn't account for edge cases where a ridiculously long field name is longer than the log message it's connected to, does that matter?
  • May need to do a refactor of getVisibleChildren to report the target position to account for this new system. It will be simpler to calculate the line number currently displayed directly from whatever the scroll position is. I also removed the HOCs to measure children in order to be more easily compatible with react-window so this is probably necessary.

@Zacqary
Copy link
Contributor Author

Zacqary commented Feb 18, 2020

A lot of discussion on this happened in Slack DMs and Zoom, here's some summaries and copypastes. Tl;dr react-window or react-virtualized's use of position: absolute to put log entries in the right spot visually and create the illusion of scrolling creates insurmountable problems for our use case, and we may need to roll our own solution based on position: static.

After getting react-window working for almost all features of the log stream (with greatly improved performance), I was stuck on implementing live-streaming, which wasn't working properly:

The scroll position is just not cooperating, though, because the windowing component is constantly changing what bits of the list it's actually rendering, and that affects the possible scroll position, and all sorts of other complexity.

The furthest along I've gotten is to just use bottom: 0 to lock the logs stream to the bottom of the page during streaming so that new entries appear in the right place, and then unlocking it in response to an onwheel event, but I still don't know how to give it the seamless feeling of scrolling up from the bottom of the stream. Because that necessitates giving it an accurate scroll position near the bottom.

Spoke with @weltenwort about this, and we determined that:

  • The goal of this project isn't just increasing performance, but also simplifying the codebase and making it more extensible
  • It doesn't make sense to measure the height of each entry based on the message string content and the size of the monospace characters; we're trying to make the log stream extensible to include more than just text entries in the future, so we really do need to use ResizeObserver or getBoundingRect for these
  • react-window's onItemsRendered callback already tells us the index of the topmost displayed item and the bottommost displayed item. We don't need to make our own calculations on getVisibleChildren anymore, and that simplifies the code.
  • onItemsRendered can't tell us what the center child is. If we decide that the target now corresponds to the bottom entry instead of the center, then not only does this simplify the code but potentially make it easier to update the scroll position accurately for live streaming

I worked on implementing these changes, and found that react-window doesn't play nicely with elements that it doesn't know the height of before it renders.

I switched to measuring the DOM heights of each log entry after they're rendered, but react-window doesn't render an item until the user scrolls to it. So as you scroll up and down, there's a significant and noticeable delay before each log entry "pops" to its actual height. That also makes the scrollbar jump around as the size of the internal div changes.
Not much of a performance hit when you measure it in raw milliseconds, but it adds a significant perception of jank to the application.

So I switched to using the more feature-rich react-virtualized in order to use its CellMeasurer component. Rendering pop-in disappeared, but scrolling became very jumpy. It was hard to accurately center on a desired timestamp as new items rendered and measured themselves.

At this point we floated the idea of virtualizing on a per-page basis instead of a per-entry basis:

Render a whole page of entries with static layout, calculate the total height of that page, and then swap it out with an empty div of the same height value when that page isn't visible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants