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] Use async search in the log stream page #90303

Merged

Conversation

afgomez
Copy link
Contributor

@afgomez afgomez commented Feb 4, 2021

Summary

Part of #86439

This PR modifies the log stream page to make use of async searches. To do so it replaces the existing useLogEntries container with useLogStream, which already uses async search.

Summary of changes

  • Removed <WithStreamItems> HOC.
  • Modify the fetchPrevious and fetchNext callbacks in useLogStream, to allow fetching before or after the timestamp range. This is necessary for the "extend range" buttons to work correctly.
  • Tweak (fix) streaming. Instead of fetching the next page after the last loaded entry, fetch the last entries. This prevents an issue where the streaming falls behind if the amount of entries between each refresh is bigger than the page size.
  • Clean up the useLogEntries hook.
  • Clean up the /log_entries/entries endpoint and its related types.

@afgomez afgomez added 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 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Feb 4, 2021
@afgomez afgomez requested a review from a team as a code owner February 4, 2021 16:56
@elasticmachine
Copy link
Contributor

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

@Kerry350 Kerry350 self-requested a review February 8, 2021 11:17
Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

Code looks good, nice to see the higher order component gone 👍

I did find a couple of bugs in the functionality though:

  1. Using the filter bar seems to throw an error.

filter bar

Screenshot 2021-02-08 at 12 43 21

  1. When I load some log entries, then go to another page, and then come back to the Stream page, it says "There are no log messages to display." I have to click "Check for new data" to see entries again. On that navigation back to the Stream there isn't a request in the network panel.

no_data

import { useLogStreamContext } from '../../../containers/logs/log_stream';
import { datemathToEpochMillis, isValidDatemath } from '../../../utils/datemath';

// FIXME Duplicated from <LogStream />. See where to put this
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be fixed before merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm OK with the duplication. We might want the pagination to work differently here than in the <LogStream /> component.

Alejandro Fernández Gómez and others added 8 commits February 8, 2021 16:08
The subscription timing is slightly unpredictable due to React's
effect execution order. This way initial requests won't get lost. The
response$ is multicast already due to `share()`, so I don't expect any
negative consequences. (famous last words)
@afgomez afgomez requested a review from Kerry350 February 10, 2021 14:51
Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

The two bugs I experienced are fixed now. Great work 🎉

@afgomez
Copy link
Contributor Author

afgomez commented Feb 11, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1050 1046 -4

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.9MB 1.9MB -7.6KB

History

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

@afgomez afgomez merged commit 1fbea8c into elastic:master Feb 11, 2021
@afgomez afgomez deleted the 86439-async-log-stream-entries-logs-page branch February 11, 2021 20:03
afgomez pushed a commit that referenced this pull request Feb 12, 2021
)

Co-authored-by: Felix Stürmer <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Felix Stürmer <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
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 release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants