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] Fix inaccuracy when jumping to a faraway time target #40303

Merged
merged 3 commits into from
Jul 9, 2019

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented Jul 3, 2019

Summary

Fixes #39944

This fixes a bug that would cause changing the timestamp in the Logs UI to a timestamp outside the currently loaded range of logs to end up landing you on a very far off place than you actually clicked on. It would also sometimes send you to a very far off timestamp when initially loading the page with a specific timestamp in the URL.

After a lot of investigation, I was able to track the issue down to the getVisibleChildren call in VerticalScrollPanel's getSnapshotBeforeUpdate. When the panel is mounted, it's supposed to behave like so:

  1. The user scrolls through the panel
  2. New log entries load when the user reaches the edge of loaded logs, prompting the component's children to change
  3. The component reports the new visible log entries to update the displayed time window in the log minimap and the toolbar

This works fine. The problem is that when the component is first mounted, its children, height, and width properties aren't finalized yet for another animation frame or so. So what happened was:

  1. The application loads log data for a large chunk of time before and after the target timestamp, with the intention of displaying the target in the very center of the log stream
  2. The component mounts and tries to jump to its target log entry. Since its height is 0, it doesn't actually scroll anywhere.
  3. The surrounding AutoSizer figures out how tall to make the component and updates it. The children also get re-rendered because they depend on some data being computed from WithColumnWidths
  4. The component updates again in response to the height and children changes, and calls getVisibleChildren. Because it hadn't actually scrolled down at all when its height was 0, the visible children are actually the beginning of available data
  5. The component reports that the beginning of the data (rather than the center) is visible, and the toolbar and log minimap change to reflect this.

This created a very annoying situation where the user might:

  1. Click on 3:00pm in the toolbar or log minimap
  2. Wait for the application to load logs from 12:00pm till 6:00pm
  3. Watch the logs for 12:00pm appear at the top of the logs stream and see the timestamp in the toolbar and the highlighted window in the log minimap both jump from 3:00pm to 12:00pm

This PR should prevent this from happening anymore.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@Zacqary Zacqary added [zube]: In Progress v8.0.0 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 v7.4.0 labels Jul 3, 2019
@Zacqary Zacqary requested a review from a team July 3, 2019 19:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Zacqary Zacqary added v7.3.0 bug Fixes for quality problems that affect the customer experience labels Jul 9, 2019
@weltenwort weltenwort self-requested a review July 9, 2019 20:56
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

This is great! Jumping around in the logs feels so much more accurate now. And extra credits for the explanation. The awesomeness of this fix is inversely proportional to its line count 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Logs UI Logs UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.3.0 v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] Jumping to a new time target outside the range of loaded entries is inaccurate
3 participants