[searchService] Dedupe shard error toasts

[mattkime]

Summary

Shard error toasts can appear in overwhelming numbers if several queries are performed at the same time and break in the same way. Displaying many shard errors doesn't help the user resolve the problem so lets only display the first one for a given sessionId. All errors without a sessionId will be treated as having the same sessionId.

The easiest way I found to reproduce this problem was by modifying a functional test -

• x-pack/test/functional/apps/discover/async_scripted_fields.js:70 - add an extra zero to the retryForTime value.
• line 74 - check for 5 of 3 - so the check breaks.
• when the test runs, you'll see a shard error toast. On main, if you set the timestamp date back a year you should see a second shard error toast. On this branch you can see exactly the same thing but only get one toast.

Part of #116686

Release note -

Shard failure notifications have been reduced when many queries fail at the same time.

[elasticmachine]

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

[ppisljar]

this came up a couple times (the issue in the description has a wrong link i think) but i think this should be solved on the notifications service level and not per specific errors (like shard error in this case) cc @lukasolson

It also seems we'll start to go in the direction of actual consumers handling this errors. They are the only ones who can be smarter about the error that is shown to the user. cc @flash1293

[flash1293]

Given how often this came up already and then the discussion got stalled I'm very much in favor of a "quick fix" that at least mitigates the issue while we figure out to solve the bigger problem - depending on app and screen size used this can be very distracting (and even make certain apps impossible to use effectively)

[mattkime]

@ppisljar

but i think this should be solved on the notifications service level and not per specific errors

That would require agreement with core which we haven't made much progress on. I have mixed feelings about that route. It would certainly be nice to fix this at the service level but I'm not completely certain that we want to debounce the same way in all cases. An intermediate step could be creating a wrapper around the notification service which also debounces. It would require a bit more effort but it wouldn't be stuck on getting agreement from the service owner.

It also seems we'll start to go in the direction of actual consumers handling this errors.

I agree thats a better design but its also a much larger project. We'd need a transition plan and careful accounting of consumer apps to make sure errors aren't swallowed.

Overall I agree with @ppisljar but I'd like to deliver something for our customers now while we resolve our internal alignment.

[lukasolson]

Hmm, I'm wondering if we can be a little more intelligent about how we dedupe these error messages instead of only debouncing every 5 seconds.

First of all, we should only be deduping if the error messages themselves are the same error message (this should be inside response.rawResponse, which is passed to the ShardFailureOpenModalButton).

Second, we really should only be deduping on each "load" or "refresh" of the data. Perhaps we can reuse sessionId inside ISearchOptions to accomplish this (or generate a random uuid if search sessions are disabled). We will have to pass the options object back to handleResponse inside SearchSource (and wherever else handleResponse is used) since it isn't currently passed to this utility.

[mattkime]

@lukasolson Sounds good. I think I've gotten ahead of things with some poor assumptions. I'd like to slow down and make sure this is done correctly.

I could use more info on the potential types of shard errors. I'm familiar with the scripting case but nothing else.

I'm also wondering if titling toasts with 'x of y shards failed' is useful. It might be useful to surface more specific info, such as 'scripting error'.

---

I have a similar PR for the data view layer. I think it avoids the specifics we're discussing here - #131779

[lukasolson]

I'm a little confused as to the debouncing here, but the deduping looks good. Just looked at the code so far, haven't actually tested.

[lukasolson]

Does this mean that regardless of the deduping, we'll still only show a max of one warning every 10 seconds?

[mattkime]

The debounce is applied for a given debounceKey. Different debounceKeys within 10s would result in multiple warnings. The same debounceKey within 10s will result in only one warning, past 10s an additional warning is shown.

[lukasolson]

This is a much needed improvement, thank you! Played around with it locally with different types of warnings/errors and things seemed to dedupe correctly while still showing errors of different types.

My only suggestion would be to increase the 10s debounce to something like 30s or 1m. I've already run into the situation multiple times where that 10s wasn't enough. Maybe we could also consider adding it to the config, but not totally necessary for this PR.

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: d182486

Metrics [docs]

Page load bundle

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

id	before	after	diff
data	437.1KB	437.4KB	+239.0B

History

• 💔 Build #80859 failed 0fd3182
• 💔 Build #80850 failed 9b3032b
• 💚 Build #80592 succeeded 9c5833e
• 💚 Build #80571 succeeded 830b932
• 💔 Build #80568 failed c581b6a
• 💔 Build #79995 failed bb912be

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