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 helper hooks with search strategy request cancellation #83906

Merged
merged 30 commits into from
Dec 11, 2020

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Nov 20, 2020

Summary

This adds helper React hooks for interacting with data plugin search strategies. Using these hooks the log entry fly-out now indicates the loading progress, shows errors and cancels requests when closed.

closes #78003

Release Notes

Displays progress for asynchronous loading of the log entry fly-out content and cancels pendig requests when closing the fly-out.

Review and implementation notes

The log entry fly-out structure has been cleaned up to remove timing issues and avoid eager fetching.

The fly-out now sports a loading progress bar with the option to cancel the running request. The request is also canceled when the fly-out is closed while loading is in progress. The cancellation can take the form of a canceled request if one is pending. Otherwise it just means that no further polling of the async results is performed.

To achieve that I'm tried to build a toolbox of hooks and components that can be used in future async search requests as well:

The producer hook useDataSearch spawns new requests using data.search.search and returns an observable that emits an object describing each new request. The response observables associated with each request are amended such that unsubscribing from them leads to cancellation of pending requests and polling cycles.

The consumer hook useLatestPartialDataSearchRequest consumes that observable and subscribes to each request's response observable in a way that...

  1. newer requests obviate and cancel the previous pending ones (hence the "latest" in the name)
  2. unmounting the component cancels pending requests
  3. make partial results received during polling available (hence the "partial" in the name)

Some utility hooks form the glue between observables and the react rendering cycle. Among them are useObservable to create stable pipe()s, useSubscription to tie a subscription to the component life-cycle and useObservableState to turn an observable's emitted values into React component state.

Several new "data search" stories in the infra storybook showcase some of the components used in the fly-out. They consist of a progress bar, an error call-out and fly-out layout helpers.

I tried to expand on the above description in a MDX storybook, which can be built using yarn storybook infra.

Previews

Display progress based on the searched/total number of shards:

image

Display errors that occur during loading without partial results available: (this weird error message is caused by the chrome devtools request blocking feature and will differ depending on the cause of the failure)

image

Display a mixture of partial results and shard failures:

image

Task breakdown

  • create request producer hook
  • create request consumer hook
  • clean up fly-out structure
  • make sure requests are cancelled when hook unmounts
  • make sure requests are cancelled when newer request comes in
  • display errors originating from the browser
  • display errors originating from the server
  • document the hooks
  • write unit tests for the hooks

@weltenwort weltenwort 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 Nov 20, 2020
@weltenwort weltenwort added this to the Logs UI 7.11 milestone Nov 20, 2020
@weltenwort weltenwort self-assigned this Nov 20, 2020
@weltenwort weltenwort force-pushed the logs-ui-cancel-log-entry-request branch from fbdc783 to d6db953 Compare November 25, 2020 17:45
@weltenwort weltenwort force-pushed the logs-ui-cancel-log-entry-request branch 2 times, most recently from 55eb746 to 11d7b79 Compare November 30, 2020 15:58
@weltenwort weltenwort force-pushed the logs-ui-cancel-log-entry-request branch from d9f6709 to 9af62cd Compare December 3, 2020 18:22
@weltenwort weltenwort force-pushed the logs-ui-cancel-log-entry-request branch from 9af62cd to b4b4ff9 Compare December 5, 2020 00:24
@weltenwort weltenwort marked this pull request as ready for review December 8, 2020 14:45
@weltenwort weltenwort requested a review from a team as a code owner December 8, 2020 14:45
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@afgomez afgomez left a comment

Choose a reason for hiding this comment

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

On a first pass the code looks good. I don't feel comfortable yet commenting on the RxJS parts but I promise I'll give it a shot tomorrow :D.


One thing I've noticed that I'm not sure it matters in practical effects: there seems to be a distinction between explicitly aborting a request and implicitly doing it when the containing component unmounts.

When the request is explicitly cancelled, there's an extra DELETE request, probably made to do housekeeping. However, when the component unmounts (by closing the flyout, for example), this request doesn't seem to happen.

logEntryId: string | null | undefined;
}) => {
const { search: fetchLogEntry, requests$: logEntrySearchRequests$ } = useDataSearch({
getRequest: useCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the useCallback required here? What would happen if the identity of this function changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

The consequence is that the returned search() function's identity changes every render. That's probably not a problem depending on the downstream use of search. So this is a performance optimization.

Comment on lines 71 to 83
if (newUrlState && newUrlState.flyoutId) {
setFlyoutId(newUrlState.flyoutId);
setLogEntryId(newUrlState.flyoutId);
}
if (newUrlState && newUrlState.surroundingLogsId) {
setSurroundingLogsId(newUrlState.surroundingLogsId);
}
if (newUrlState && newUrlState.flyoutVisibility === FlyoutVisibility.visible) {
setFlyoutVisibility(true);
openFlyout();
}
if (newUrlState && newUrlState.flyoutVisibility === FlyoutVisibility.hidden) {
setFlyoutVisibility(false);
closeFlyout();
}
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can simplify this block a bit. What do you think?

if (newUrlState && newUrlState.flyoutId && newUrlState.flyoutVisibility === FlyoutVisibility.visible) {
  openFlyout(newUrlState.flyoutId);
}
if (newUrlState && newUrlState.flyoutVisibility === FlyoutVisibility.hidden) {
  closeFlyout();
}
if (newUrlState && newUrlState.surroundingLogsId) {
  setSurroundingLogsId(newUrlState.surroundingLogsId);
}

That way I think that we no longer need to expose the setLogEntryId() callback from useLogEntryFlyoutContext()


There's something about this block that github doesn't like; it doesn't let me add a suggestion.

Copy link
Member Author

@weltenwort weltenwort Dec 10, 2020

Choose a reason for hiding this comment

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

But wouldn't that mean that we can't represent the scenario that the URL contains a flyoutId but the visibility is false? I personally don't like that split anyway, but I wonder how risky it would be to change the URL semantics.

Comment on lines 85 to 96
if (initialUrlState && initialUrlState.flyoutId) {
setFlyoutId(initialUrlState.flyoutId);
setLogEntryId(initialUrlState.flyoutId);
}
if (initialUrlState && initialUrlState.surroundingLogsId) {
setSurroundingLogsId(initialUrlState.surroundingLogsId);
}
if (initialUrlState && initialUrlState.flyoutVisibility === FlyoutVisibility.visible) {
setFlyoutVisibility(true);
openFlyout();
}
if (initialUrlState && initialUrlState.flyoutVisibility === FlyoutVisibility.hidden) {
setFlyoutVisibility(false);
closeFlyout();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above

@weltenwort
Copy link
Member Author

When the request is explicitly cancelled, there's an extra DELETE request, probably made to do housekeeping. However, when the component unmounts (by closing the flyout, for example), this request doesn't seem to happen.

That's an interesting observation. I suspect it's a matter of timing. The DELETE is supposed to cancel the async search after the server side has returned a partial result with an id. It won't happen before the polling has started or after the final results have been delivered. I can't trigger it reliably enough to verify that pattern, though. Can you?

@afgomez
Copy link
Contributor

afgomez commented Dec 10, 2020

I suspect it's a matter of timing. The DELETE is supposed to cancel the async search after the server side has returned a partial result with an id. It won't happen before the polling has started or after the final results have been delivered. I can't trigger it reliably enough to verify that pattern, though. Can you?

That seems the case. If I cancel the request when the "Searching log entries in shards" message appears, it always sends a DELETE request, but only when clicking on the red ❌ button. If I close the flyout the request doesn't happen.

I don't think it's a big issue, but we can check with the data plugin owners to see if it is. If that's the case we can always fix it after feature freeze.

@weltenwort
Copy link
Member Author

It might indicate that we're leaking async search results on the ES side, so it could become a performance or stability problem. I'll investigate further.

Copy link
Contributor

@afgomez afgomez left a comment

Choose a reason for hiding this comment

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

Although I couldn't take a deep look at all the RxJS-related code, I feel comfortable approving the PR to move forward since the code works as intended.

I can take a look later at the code and write a proposal with changes if I find anything :)

Good job. LGTM!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1085 1094 +9

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.7MB 2.7MB +16.6KB

Distributable file count

id before after diff
default 47010 47771 +761

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
infra 181.7KB 181.8KB +137.0B

History

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

@weltenwort weltenwort merged commit efe62ac into elastic:master Dec 11, 2020
@weltenwort weltenwort deleted the logs-ui-cancel-log-entry-request branch December 11, 2020 10:25
weltenwort added a commit that referenced this pull request Dec 11, 2020
…ation (#83906) (#85697)

Backports the following commits to 7.x:
 - [Logs UI] Add helper hooks with search strategy request cancellation (#83906)
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:enhancement 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 App] Create logs data plugin hook for handling our custom search strategy
4 participants