Don't trigger auto-refresh until previous refresh completes

[Dosant]

Summary

Closes #86947
Partially addresses #76521
Unblocks #95643

This pr changes how auto-refresh works: now when auto-refresh observable emits app needs to call a done() callback notifying the auto-refresh services that it is OK to start a new refresh loop.

Each app decides slightly differently what "complete" means:

1. Dashboard and Lens use underlying search session services. Assume it is OK to start a next auto-refresh loop when the next session completes
2. Discover awaits $scope.fetch()
3. Visualize awaits expressionHandler.update()

There are likely edge cases with these "complete" estimations, but in the end, it isn't really important if done() is called too often or too soon.

How to test

Makes sense to only test Discover, Visualize, Lens and Dashboard because only they use getAutoRefreshFetch$.
You can:

1. Slower your network using custom present with 5s latency
2. Set a timed refresh in 3s interval
3. You should see something like this in network table:

0s ---- requests
....
5s ---- request finishes 
....
8s ---- request starts

Please note,
I noticed that map embeddable doesn't use getAutoRefreshFetch$, but is using a custom window.interval so it behaves oddly on a dashboard with a "slowed down" network. (this is not a regression, but a bug). I will open a separate issue for the geo team.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

Pinging @elastic/kibana-app-services (Team:AppServices)

[flash1293]

I'm not sure whether this works 100% as expected in Lens. This is what I did:

• Comment out suggestions
• Add a network throttle profile that delays everything by 5 seconds
• Set a refresh once per second

This is what I'm seeing on the network:

If auto refresh hits (and no request is in flight) it starts a request which will run for 5 seconds. Then it will be cancelled and another request is started which succeeds and updates the chart, just to start the cycle again. This means there's always a 5s gap between succeeding request fetches which shouldn't be the case AFAICT.

Is this expected and due to the way request delaying in Chrome works? Is there a better way to test?

[Dosant]

@flash1293, not 100% sure but I think there is a different bug in the lens (expressions) that is visible on a slow network, I think it might be visibly only on the slow network because of this denounce:

kibana/src/plugins/expressions/public/react_expression_renderer.tsx

Line 187 in fe1ae92

if (expressionLoaderRef.current && !waitingForDebounceToComplete) {

. It is probably the same issue: #91519

This case you've described:

If auto refresh hits (and no request is in flight) it starts a request which will run for 5 seconds. Then it will be cancelled and another request is started which succeeds and updates the chart, just to start the cycle again. This means there's always a 5s gap between succeeding request fetches which shouldn't be the case AFAICT.

Is the same for me if instead of auto-refresh I just click "refresh"

It seems that auto-refresh behavior works correctly: there is a 5 seconds gap between series of requests.

[flash1293]

not 100% sure but I think there is a different bug in the lens (expressions) that is visible on a slow network, I think it might be visibly only on the slow network because of this denounce:

We are not using debounce in the expression renderer for the main workspace visualization (which is what I tested).

It seems that auto-refresh behavior works correctly: there is a 5 seconds gap between series of requests.

Why is this the correct behavior? I would expect there is at max a 1 second gap between the requests

[Dosant]

We are not using debounce in the expression renderer for the main workspace visualization (which is what I tested).

Sorry, I think I pointed to a wrong debounce. I think this one is in play (need to look deeper):

kibana/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx

Line 534 in fe1ae92

export const VisualizationWrapper = debouncedComponent(InnerVisualizationWrapper);

Why is this the correct behavior? I would expect there is at max a 1 second gap between the requests

Ooh, I see.
This is how it is implemented: start a new interval after previous data has completed loading and I didn't think about it differently 🤔
Not sure which approach should we proceed with.

[flash1293]

Sorry, I think I pointed to a wrong debounce. I think this one is in play (need to look deeper):

I don't think it has anything to do with this - it's just debouncing by a quarter of a second

This is how it is implemented: start a new interval after previous data has completed loading and I didn't think about it differently 🤔
Not sure which approach should we proceed with.

But it hasn't completed loading, it got aborted. What I would expect to see is:

• 0ms: First request starting loading (will take 5 seconds)
• 1000ms: Refresh attempts to start a new request, but as the old one is still in flight, it waits
• 2000ms: Same
• ...
• 5000ms: First request finishes, chart is rendered. A new request is started immediately because our goal is to refresh once a second
• 6000ms: Refresh attempts to start a new request, but as the old one is still in flight, it waits
• ...
• 10000ms: Request started at 5000ms finishes, chart is rendered. A new request is started immediately because our goal is to refresh once a second

This means, one successful render every 5000ms which is as good as we can do. What happening is this (AFAICT, maybe I am misunderstanding something):

• 0ms: First request starting loading (will take 5 seconds)
• 5000ms: First request is cancelled. A new request is started
• 10000ms: Request started at 5000ms finishes, chart is rendered

I'm not sure why the first request got aborted, maybe we need to fix it there

[flash1293]

@Dosant Maybe it gets cancelled because of some race condition? I know there is some logic to abort requests if the expression execution gets aborted, maybe that's what happening here:

• First request finishes, but expression isn't completed yet (post processing and chart rendering is still missing)
• Auto-refresh attempts to re-render the chart, because it's synchronously reacting to the first request being completed
• Expression run is aborted and starts over without using the data from the first request

Not sure whether that makes sense, just an idea based on my understanding

[flash1293]

Tested in Lens editor and Lens vis on dashboard and the behavior is as discussed - the next auto reload cycle begins after the previous load is finished. No cancelled requests anymore.

However I noticed an additional 1.5s gap between the loads on top of the scheduled auto-reload time (not sure what's introducing it)

no delay, once per second

5s delay, once per second

no delay, once every 5 seconds

5s delay, once every 5 seconds

[Dosant]

@elasticmachine merge upstream

[Dosant]

However I noticed an additional 1.5s gap between the loads on top of the scheduled auto-reload time (not sure what's introducing it)

Probably because of that is used in the lens and dashboard https://github.com/elastic/kibana/pull/93410/files#diff-d63d089cd42d01ad19e7feba35bf1e76df301fc64c52aff7fa8431f4411e38b2R30
This is a part of the heuristic to determine session is really completed

[Dosant]

@elasticmachine merge upstream

[Dosant]

@elasticmachine merge upstream

[ThomThomson]

Presentation team code changes LGTM. Tested locally in chrome, dashboard changes work as expected as long as the dashboard does not contain a map.

[Dosant]

@kertal, could you please review Discover part 🙏

Also, I think this pr introduced a small problem:

When you load Discover with frequent auto-refresh and slow network then it never displays the results, because new results are already loading when previous completed loading:

kibana/src/plugins/discover/public/application/angular/discover.js

Line 437 in 4342f26

$scope.resultState = getResultState($scope.fetchStatus, $scope.rows);

I think this wasn't an issue last week.

@kertal, if you'd helped me out with this edge case in this pr that would be awesome. Or I can just create a bug.
Please note, in master this edge case is also not stable. It could get into a situation where each auto-refresh just cancels the previous request.

[kertal]

ACK, will have a look, thx!

[Dosant]

@elasticmachine merge upstream

[kertal]

Also, I think this pr introduced a small problem:

When you load Discover with frequent auto-refresh and slow network then it never displays the results, because new results are already loading when previous completed loading:

So far my research: there seems to be an fetch triggered before the auto-refresh fetch is starting, and this causes the troubles with slow networks ... no solution currently, but working on it

[kertal]

Also, I think this pr introduced a small problem:
When you load Discover with frequent auto-refresh and slow network then it never displays the results, because new results are already loading when previous completed loading:

So far my research: there seems to be an fetch triggered before the auto-refresh fetch is starting, and this causes the troubles with slow networks ... no solution currently, but working on it

@Dosant QQ: while debugging setting interval with 1s and Fast 3G I noticed that the initial fetch is triggered by timefilter.getFetch$() followed by timefilter.getAutoRefreshFetch$, the second request cancels the first one. could we e.g. ignore timefilter.getAutoRefreshFetch$ while $scope.fetchStatus === fetchStatuses.LOADING?

[Dosant]

@kertal, I've added your suggestion and it looks like it fixed that issue 👍
Thanks!

[Dosant]

@elasticmachine merge upstream

[kertal]

Code LGTM, thanks for the last adaptation, works as expected now even with slow network as 1s refresh interval 👍

[Dosant]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 26dfb19
• Storybooks Preview

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
data	559	561	+2

Async chunks

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

id	before	after	diff
dashboard	134.6KB	134.8KB	+287.0B
discover	393.5KB	393.9KB	+380.0B
lens	941.5KB	941.7KB	+254.0B
visualize	91.3KB	91.4KB	+34.0B
total			+955.0B

Page load bundle

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

id	before	after	diff
dashboard	309.6KB	309.9KB	+269.0B
data	783.8KB	785.6KB	+1.8KB
expressions	193.0KB	193.0KB	+12.0B
visualizations	91.8KB	91.8KB	+23.0B
total			+2.1KB

History

• 💚 Build #117496 succeeded c6500e1
• 💚 Build #116137 succeeded b300f47
• 💔 Build #116109 failed 4b9b960
• 💚 Build #115775 succeeded 10ff0c2
• 💔 Build #115736 failed 69d77c9

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