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] Add pagination to the log stream shared component #81193

Merged
merged 15 commits into from
Nov 12, 2020

Conversation

afgomez
Copy link
Contributor

@afgomez afgomez commented Oct 20, 2020

Summary

Closes #77033
Closes #61848

This PR implements infinite scroll (AKA pagination) in the log stream shared component.

The first two commits change the existing /log_entries/entries API to return the hasMoreBefore and hasMoreAfter flags. The UI will use this information to decide if it should try to query the API to fetch more pages when the user scrolls.

The third commit uses this new flags in the existing log stream page.

The rest of the commits implement the pagination for the shared log stream component.

Extend the API response with `hasMoreBefore` and `hasMoreAfter`
attributes.

The value is a three-state boolean, where `undefined` means "we don't
know if there are more pages". The API calls go in one direction, either
before a `cursor`, or after it. To check if there are entries in the
opposite direction we would need to do an extra API call. Instead of
doing that for every request, we delegate the responsibility of doing
the extra API call to the clients.
@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 v7.11.0 labels Oct 20, 2020
@afgomez afgomez added this to the Logs UI 7.11 milestone Oct 20, 2020
@afgomez afgomez requested a review from a team as a code owner October 20, 2020 15:56
@elasticmachine
Copy link
Contributor

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

@afgomez afgomez added the release_note:skip Skip the PR/issue when compiling release notes label Oct 20, 2020
@afgomez
Copy link
Contributor Author

afgomez commented Nov 5, 2020

@elasticmachine merge upstream

@afgomez
Copy link
Contributor Author

afgomez commented Nov 9, 2020

@elasticmachine merge upstream

@weltenwort weltenwort self-requested a review November 9, 2020 17:17
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.

Not sure if it is related to these changes, but I'm not able to see the end-of-stream row at the bottom of the "view in context" modal:

image

This is on Chromium 86.0.4240.183. It doesn't occur on Firefox. 🤔

@afgomez
Copy link
Contributor Author

afgomez commented Nov 9, 2020

@weltenwort it's probably unrelated. I will take a look tomorrow morning though just to be sure and open a ticket for it.

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.

Thanks for providing such a clear progression of the implementation. This made it easier to read. 👍 It seemed work in general, but the "view-in-context" modal still behaves weird for me in Chromium (as mentioned in a previous comment).

Aside from a few style and edge-case nits I noticed that too many queries might be fired when scrolling towards an edge. I commented on that below.

@afgomez
Copy link
Contributor Author

afgomez commented Nov 10, 2020

The height issue with the "View in context" modal also happens in master. I'll open a ticket to fix it on a separate PR.

@afgomez afgomez requested a review from weltenwort November 11, 2020 12:03
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.

The "fetch storm" on scroll is gone, feels pretty solid - nice job 👏

The ACs mention documentation. Is there anything to be done here?

Comment on lines +68 to +69
const prevStartTimestamp = usePrevious(startTimestamp);
const prevEndTimestamp = usePrevious(endTimestamp);
Copy link
Member

Choose a reason for hiding this comment

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

nice, I love the expressiveness of usePrevious!

@afgomez
Copy link
Contributor Author

afgomez commented Nov 12, 2020

The ACs mention documentation. Is there anything to be done here?

I don't think so. Initially I thought it would be interesting to disable the pagination with a prop, but I cannot think of a use case for it. Since the consumers cannot control how it works there's nothing to add to the docs.

@afgomez
Copy link
Contributor Author

afgomez commented Nov 12, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 2.5MB 2.5MB +5.4KB

History

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

@afgomez afgomez merged commit 0e7bcf6 into elastic:master Nov 12, 2020
@afgomez afgomez deleted the 77033-log-stream-component-pagination branch November 12, 2020 14:11
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 12, 2020
* master:
  [Advanced Settings] Introducing telemetry (elastic#82860)
  [alerts] add executionStatus to event log doc for action execute (elastic#82401)
  Add additional sources routes (elastic#83227)
  [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149)
  [Logs UI] Add pagination to the log stream shared component (elastic#81193)
  [Index Management] Add an index template link to data stream details (elastic#82592)
  Add maps_oss folder to code_owners (elastic#83204)
  fix truncation issue (elastic#83000)
  [Ingest Manger] Move asset getters out of registry (elastic#83214)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 12, 2020
… alerts/action-groups-as-conditions

* origin/alerts/stack-alerts-public: (91 commits)
  removed import from plugin code as it causes FTR to fail
  [Advanced Settings] Introducing telemetry (elastic#82860)
  [alerts] add executionStatus to event log doc for action execute (elastic#82401)
  Add additional sources routes (elastic#83227)
  [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149)
  [Logs UI] Add pagination to the log stream shared component (elastic#81193)
  [Index Management] Add an index template link to data stream details (elastic#82592)
  Add maps_oss folder to code_owners (elastic#83204)
  fix truncation issue (elastic#83000)
  [Ingest Manger] Move asset getters out of registry (elastic#83214)
  make defaulted field non maybe
  Remove unused asciidoc file (elastic#83228)
  [Lens] Remove background from lens embeddable (elastic#83061)
  [Discover] Unskip flaky tests based on discover fixture index pattern (elastic#82991)
  Removing unnecessary trailing slash in CODEOWNERS
  Trying to fix CODEOWNERS again, where was a non-existent team prior (elastic#83236)
  Trying to fix CODEOWERS, missing a starting slash (elastic#83233)
  skip flaky suite (elastic#83231)
  Add enzyme rerender test helper (elastic#83208)
  Move Elasticsearch type definitions out of APM (elastic#83081)
  ...
afgomez pushed a commit that referenced this pull request Nov 13, 2020
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.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] Add pagination to embeddable <LogStream /> component Infinite scrolling in "view logs in context"
4 participants