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] Prevent summary request from piling up #148670

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Jan 10, 2023

📝 Summary

This prevents the execution of new log stream summary requests until the previous request has finished.

related to #86439

🕵️‍♀️ Review notes

Currently, the useLogSummary hook executes new summary requests when any of the parameters change. If parameter changes arrive quicker than the summary requests complete, they will pile up. While the useCancellableEffect prevented the requests from overtaking each other, the old in-flight requests would still put load on the cluster.

Ideally we would re-implement the summary fetching as cancellable async searches. But since we're going to change the orchestration of log stream requests soon anyway (as part of #134412), that would be wasteful. So this is an intermediate improvement to reduce the cluster load.

The changes to the use_observable.ts collection of hooks are twofold:

  • The previous implementation of useBehaviorSubject didn't keep the identity of the second return value (the next function) stable, which could lead to undesired re-rendering.
  • The new useReplaySubject hook is similar to the improved useBehaviorSubject, but powered by a n RxJS ReplaySubject, which doesn't have an initial value.

🩺 How to test

  • visit the Logs UI in a cluster with log entries
  • start streaming
  • set the refresh interval to a low value like 1s
  • throttle the network traffic such that requests to /api/log_entries/summary take longer than the refresh interval
  • observe that no two requests to that API are ever in flight in parallel

@weltenwort weltenwort added bug Fixes for quality problems that affect the customer experience 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 Jan 10, 2023
@weltenwort weltenwort self-assigned this Jan 10, 2023
@StephanErb
Copy link

StephanErb commented Jan 10, 2023

Ha, nice! Do you think it would be possible to get a backport of this to 6.X 8.6.x?

We are suffering from this quite a bit. A single user enabling log streaming without filters can overload the entire cluster for us.

@weltenwort
Copy link
Member Author

@StephanErb it would be pretty easy to backport to earlier 8.x versions. There are no releases for 6.x planned, so I'd prefer not to touch that branch.

@StephanErb
Copy link

Oh. I meant 8.6.x :)

@weltenwort weltenwort added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Jan 11, 2023
@weltenwort
Copy link
Member Author

hehe, ok. sure, I'll backport it to 8.6

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1237 1236 -1

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.3MB 1.3MB +219.0B

History

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

cc @weltenwort

@weltenwort weltenwort marked this pull request as ready for review January 11, 2023 15:04
@weltenwort weltenwort requested a review from a team as a code owner January 11, 2023 15:04
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

Copy link
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

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

LGTM, work as expected on my cluster, nice usage of exhaustMap 👏

@weltenwort weltenwort merged commit f01a61b into elastic:main Jan 12, 2023
@weltenwort weltenwort deleted the log-stream-avoid-overlapping-summary-requests branch January 12, 2023 11:04
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.6

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 12, 2023
Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit f01a61b)
kibanamachine added a commit that referenced this pull request Jan 12, 2023
…8792)

# Backport

This will backport the following commits from `main` to `8.6`:
- [[Logs UI] Prevent summary request from piling up
(#148670)](#148670)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Felix
Stürmer","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-01-12T11:04:05Z","message":"[Logs
UI] Prevent summary request from piling up (#148670)\n\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"f01a61b81ab0a4756dfe40233aa61a196aa64dbe","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","Feature:Logs
UI","Team:Infra Monitoring
UI","backport:prev-minor","v8.7.0"],"number":148670,"url":"https://github.com/elastic/kibana/pull/148670","mergeCommit":{"message":"[Logs
UI] Prevent summary request from piling up (#148670)\n\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"f01a61b81ab0a4756dfe40233aa61a196aa64dbe"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/148670","number":148670,"mergeCommit":{"message":"[Logs
UI] Prevent summary request from piling up (#148670)\n\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"f01a61b81ab0a4756dfe40233aa61a196aa64dbe"}}]}]
BACKPORT-->

Co-authored-by: Felix Stürmer <[email protected]>
jennypavlova pushed a commit to jennypavlova/kibana that referenced this pull request Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) 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 v8.6.1 v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants