[Search] Add a new advanced setting searchTimeout #75728

lizozom · 2020-08-23T15:41:42Z

Summary

Depends on #75943
Partially resolves #75321
Related to #75385

Remove defaultVars from the legacy core_plugins/elasticsearch code and stop using esShardTimeout anyewhere on the client.
Introduce a new SEARCH_TIMEOUT uiSetting. This setting controls the search interceptor timing in xpack, while the OSS timeout behavior is now controlled by the server.
Remove unnecessary settings from the FE (and fetch them from uiSettings on the server)
Update the default search settings in getDefaultSearchParams
Fix a small bug: delete wasn't using the search strategy name properly.
Simplify the construction of arguments for async search.

Dev Docs

This PR changes the behavior of how we timeout search requests:

The Kibana server uses the new ES client. The client already uses all timeout configurations like requestTimeout, shardTimeout and maxRetries and since the client can't override those settings anyway, in OSS, we are removing the code governing the ES timeout on the client. Instead, this PR adds handling for a timeout error response.
A nice side effect is being able to remove injectDefaultVars from the legacy core plugin.
With Basic+ licenses, users can control the maximum time for a search session (for example a single re-load of a dashboard), per space. This PR introduces a new Advanced Setting that can be set to a positive value, or to 0, hence allowing queries to run without a timeout, as long as a user stays on screen.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-08-23T15:42:33Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

lukeelmers · 2020-08-24T15:29:29Z

x-pack/plugins/data_enhanced/server/ui_settings.ts

+      value: 600000,
+      description: i18n.translate('kbn.advancedSettings.searchTimeoutDesc', {
+        defaultMessage:
+          'Change the maximum timeout for a search session or set to 0 to disable the timeout and allow queries to run to completion.',


I don't know "search timeout" is the right way to name this... it sounds very generic for a concept that is in my mind quite specific to async search & search sessions. (I also wouldn't expect an end user to understand what "search session" means)

That said, I'm struggling to think of better ideas. Maybe something like "grouped searches timeout" or "batched searches timeout"? IDK, but wanted to ask in case others have ideas.

How do we explain search sessions to a user? How do we make it clear that this isn't just a generic timeout for all searches?

I think asyncSearchTimeout would be more explicit. It's not actually the timeout per session, it's the timeout per async search (even though a single async search may consist of multiple requests).

@lukasolson we actually talked about making it the session timeout, didn't we?

lizozom · 2020-08-26T12:35:52Z

@elasticmachine merge upstream

src/plugins/data/public/search/search_interceptor.ts

lizozom · 2020-08-27T07:32:11Z

@elasticmachine merge upstream

lukasolson

We'll need to add docs for this setting to the asciidocs too: https://github.com/elastic/kibana/blob/630d2d5fad9e08df4c8898efbd47f1a5f09575bf/docs/management/advanced-options.asciidoc

Here's some notes from my testing so far:

At some point during development of async search, the rest_total_hits_as_int parameter wasn't working (for _async_search), so we used track_total_hits instead, and shimmed the response so it looked the same as when we send rest_total_hits_as_int. Well, it looks like now async search can handle this parameter. Regardless, I think the temporary solution should be to always send the track_total_hits parameter and shim the response instead of sending rest_total_hits_as_int, since that parameter is deprecated and being removed in 8.0. (See [Kibana App] Adapt to new hits.total format in Elasticsearch #26356.) This can be tackled in a separate PR.
max_concurrent_shard_requests is only currently sent in _msearch. It could (and should) be sent in all of the scenarios we discussed (_search, _msearch, _rollup_search, and _async_search). I think this wasn't properly handled prior to this PR either.

I have a little bit more testing to do tomorrow but that's my feedback on what I've tested thus far.

lizozom · 2020-09-01T12:17:53Z

@lukasolson I added maxConcurrentShardRequests to all types of search. I also used trackTotalHits instead of restTotalHitsAsInt and added a call to shimTotalHits in both search and msearch route handlers.

lukasolson

Looks like at some point we lost the snake casing of parameters... I'm getting this error:

request [/aw-watcher-*/_async_search] contains unrecognized parameters: [batchedReduceSize] -> did you mean [batched_reduce_size]?, [ignoreThrottled] -> did you mean [ignore_throttled]?, [ignoreUnavailable] -> did you mean [ignore_unavailable]?, [keepAlive], [trackTotalHits], [waitForCompletionTimeout]

lizozom · 2020-09-02T14:01:20Z

@lukasolson "lost" is a nice way to say "attempted to intentionally remove".
Just reverted this change.

lizozom · 2020-09-03T12:20:36Z

@elasticmachine merge upstream
@elasticmachine merge upstream

lukasolson · 2020-09-05T21:02:19Z

@elasticmachine merge upstream

lizozom · 2020-09-06T07:23:46Z

@elasticmachine merge upstream

…-timeout-setting

lizozom · 2020-09-06T13:18:26Z

@elasticmachine merge upstream

lizozom · 2020-09-07T06:38:33Z

@elasticmachine merge upstream

…-timeout-setting

…bana into search/search-timeout-setting

…-timeout-setting

majagrubic

kibana-app code LGTM!

kibanamachine · 2020-09-08T14:47:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d5c6042

Build metrics

page load bundle size

id	value	diff	baseline
data	1.4MB	-1.6KB	1.4MB
dataEnhanced	178.8KB	+645.0B	178.1KB
visTypeVega	665.4KB	-154.0B	665.5KB
total		-1.1KB

distributable file count

id	value	diff	baseline
default	45456	+2	45454
oss	27224	+2	27222
total		+4

History

💔 Build #73087 failed 61d1f76
💔 Build #73049 failed 4bdbcc6
💔 Build #73080 failed 4405680
💔 Build #73032 failed b9c7dfc
💚 Build #72841 succeeded b9306c0

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

* Add new x-pack advanced setting searchTimeout and use it in the EnhancedSearchInterceptor * docs * Rely on server timeout in OSS (?) Use UI setting in xpack. * Rename function * doc * Remove esShard from client * cleanup request parameters from FE * doc * doc * Align request parameters on server, Remove leftover parameters from client Shim responses for search and msearch routes * docs Stop using toSnakeCase Updates jest tests * add management docs * docs * Remove import * Break circular dep + fix msearch test * Remove deleted type * Fix jest * Bring toSnakeCase back * docs * fix jest * Fix merge * Fix types * Allow timeout to be undefined * Fix jest test * Upldate docs * Fix msearch jest * docs * Fix rollup search merge * docs Co-authored-by: Lukas Olson <[email protected]> Co-authored-by: Elastic Machine <[email protected]> # Conflicts: # src/legacy/core_plugins/elasticsearch/index.js

* master: (68 commits) a11y tests on spaces home page including feature control (elastic#76515) [ML] Transforms list: persist pagination through refresh interval (elastic#76786) [ML] Replace all use of date_histogram interval with fixed_interval (elastic#76876) [Timelion] Update timelion deprecation links (elastic#77008) [Security Solution] Refactor Network Details to use Search Strategy (elastic#76928) Upgrade elastic charts to 21.1.2 (elastic#76939) [Alerting][Connectors] Refactor Jira: Generic Implementation (phase one) (elastic#73778) [Snapshot & Restore] fix pre existing policy with no existing repository (elastic#76861) Update saved object management UI text (elastic#76826) [Form lib] Add validations prop to UseArray and expose "moveItem" handler (elastic#76949) [Logs UI] Use fields api in log stream (elastic#76919) [UI Metrics] Support multi-colon keys (elastic#76913) [APM] Script for creating functional test archive (elastic#76926) [ENDPOINT] First version of the trusted apps list. (elastic#76304) Correct field for rum page url (elastic#76916) [Security Solution] Fix redirect properly old SIEM App routes (elastic#76868) Bump http-proxy from 1.17.0 to 1.18.1 (elastic#76924) [RUM Dashboard] Visitor breakdown usability (elastic#76834) [Search] Add a new advanced setting searchTimeout (elastic#75728) [DOCS] Adds timelion deprecation to new visualize docs structure (elastic#76959) ...

…arch strategy (#91068) ## Summary Moves `track_total_hits` from body messages of our queries into the params section of our queries. Several of our `track_total_hits: false` were not taking effect and instead were being set to `track_total_hits: true` when being executed within the Kibana search strategy vs. previously when they were regular Elasticsearch queries and always took effect. When teams port over their searches to the search strategies provided by Kibana, they are required to move any and all `track_total_hits` from their `body` sections of their code into the `params` part of their code. The reason for this is that the search strategy maintains a backwards compatibility with earlier versions of searches before Elasticsearch introduced the `track_total_hits`. However, the code does not detect if you put the `track_total_hits` in your body, it only checks the params section and forces it to `true` if it is not found in the params section. If the search strategy does not see a `track_total_hits` within the params section of the query, it will force add one and that one will override any within the body of the query. For example, if you had a `track_total_hits` in your body and not in the params section, then search strategy would execute the query like so: ```ts GET someindex-*/_search?track_total_hits=true { // some query here "track_total_hits": false } ``` The forced parameter of `?track_total_hits=true` overrides the `track_total_hits: false` within the body of your query regardless of what the `track_total_hits` is set to and you always get the true. This bug has existed since 7.10.0 when we ported over queries to search strategy. You can see the code which sets this parameter if you do not here for master, 7.11, 7.10: https://github.com/elastic/kibana/blob/master/src/plugins/data/server/search/es_search/request_utils.ts#L31 https://github.com/elastic/kibana/blob/7.11/src/plugins/data/server/search/es_search/request_utils.ts#L31 https://github.com/elastic/kibana/blob/7.10/src/plugins/data/server/search/es_search/get_default_search_params.ts#L42 Comments about the behavior from 7.10: #75728 (review) When running this code you can open dev tools and inspect the data and now notice when the total hits does not get set vs. before when it was getting set: before fix where total shows up for queries with `track_total_hits` in the body: <img width="1370" alt="event_view_before" src="https://user-images.githubusercontent.com/1151048/107594265-bfc92e80-6bce-11eb-8526-8a9aa24e7b3a.png"> after fix where total no longer shows up for queries with `track_total_hits` moved to the params section: <img width="1309" alt="event_view_after" src="https://user-images.githubusercontent.com/1151048/107594274-c5bf0f80-6bce-11eb-9d8e-698ed430c953.png"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…arch strategy (#91068) (#91076) ## Summary Moves `track_total_hits` from body messages of our queries into the params section of our queries. Several of our `track_total_hits: false` were not taking effect and instead were being set to `track_total_hits: true` when being executed within the Kibana search strategy vs. previously when they were regular Elasticsearch queries and always took effect. When teams port over their searches to the search strategies provided by Kibana, they are required to move any and all `track_total_hits` from their `body` sections of their code into the `params` part of their code. The reason for this is that the search strategy maintains a backwards compatibility with earlier versions of searches before Elasticsearch introduced the `track_total_hits`. However, the code does not detect if you put the `track_total_hits` in your body, it only checks the params section and forces it to `true` if it is not found in the params section. If the search strategy does not see a `track_total_hits` within the params section of the query, it will force add one and that one will override any within the body of the query. For example, if you had a `track_total_hits` in your body and not in the params section, then search strategy would execute the query like so: ```ts GET someindex-*/_search?track_total_hits=true { // some query here "track_total_hits": false } ``` The forced parameter of `?track_total_hits=true` overrides the `track_total_hits: false` within the body of your query regardless of what the `track_total_hits` is set to and you always get the true. This bug has existed since 7.10.0 when we ported over queries to search strategy. You can see the code which sets this parameter if you do not here for master, 7.11, 7.10: https://github.com/elastic/kibana/blob/master/src/plugins/data/server/search/es_search/request_utils.ts#L31 https://github.com/elastic/kibana/blob/7.11/src/plugins/data/server/search/es_search/request_utils.ts#L31 https://github.com/elastic/kibana/blob/7.10/src/plugins/data/server/search/es_search/get_default_search_params.ts#L42 Comments about the behavior from 7.10: #75728 (review) When running this code you can open dev tools and inspect the data and now notice when the total hits does not get set vs. before when it was getting set: before fix where total shows up for queries with `track_total_hits` in the body: <img width="1370" alt="event_view_before" src="https://user-images.githubusercontent.com/1151048/107594265-bfc92e80-6bce-11eb-8526-8a9aa24e7b3a.png"> after fix where total no longer shows up for queries with `track_total_hits` moved to the params section: <img width="1309" alt="event_view_after" src="https://user-images.githubusercontent.com/1151048/107594274-c5bf0f80-6bce-11eb-9d8e-698ed430c953.png"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

lizozom added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Aug 23, 2020

lizozom requested a review from a team as a code owner August 23, 2020 15:41

lizozom changed the title ~~Add new x-pack advanced setting searchTimeout and use it in the Enhan…~~ Add new x-pack advanced setting searchTimeout Aug 23, 2020

lizozom changed the title ~~Add new x-pack advanced setting searchTimeout~~ Add new Basic+ advanced setting searchTimeout Aug 23, 2020

lizozom self-assigned this Aug 23, 2020

lizozom requested a review from lukasolson August 23, 2020 15:42

lizozom added Feature:Search Querying infrastructure in Kibana v7.10.0 v8.0.0 Team:AppArch labels Aug 23, 2020

lukeelmers reviewed Aug 24, 2020

View reviewed changes

lizozom marked this pull request as draft August 26, 2020 10:53

lizozom commented Aug 26, 2020

View reviewed changes

src/plugins/data/public/search/search_interceptor.ts Outdated Show resolved Hide resolved

lizozom changed the title ~~Add new Basic+ advanced setting searchTimeout~~ [Search] Add a new advanced setting searchTimeout Aug 26, 2020

This was referenced Aug 27, 2020

[Search] Remove client side shard timeout #75326

Closed

[Meta] Search Sessions Roadmap #61738

Closed

lukasolson requested changes Sep 1, 2020

View reviewed changes

lizozom requested review from lukasolson and lukeelmers September 1, 2020 14:12

lukasolson requested changes Sep 1, 2020

View reviewed changes

lizozom mentioned this pull request Sep 2, 2020

Add a hard time limit for the entire search request elastic/elasticsearch#30897

Closed

lizozom requested a review from lukasolson September 2, 2020 14:10

Fix types

313bbd7

Liza K added 5 commits September 6, 2020 10:48

Merge branch 'master' of github.com:elastic/kibana into search/search…

2dea1e3

…-timeout-setting

Allow timeout to be undefined

6111ac7

Fix jest test

b1b466c

Upldate docs

5a7c9e0

Fix msearch jest

882c4fd

elasticmachine and others added 9 commits September 7, 2020 00:38

Merge branch 'master' into search/search-timeout-setting

b9306c0

Merge branch 'master' of github.com:elastic/kibana into search/search…

bec86a5

…-timeout-setting

docs

887b3ea

Merge branch 'search/search-timeout-setting' of github.com:lizozom/ki…

b9c7dfc

…bana into search/search-timeout-setting

Merge branch 'master' of github.com:elastic/kibana into search/search…

4bdbcc6

…-timeout-setting

Merge branch 'master' of github.com:elastic/kibana into search/search…

f6d4b5b

…-timeout-setting

Fix rollup search merge

4405680

Merge branch 'master' of github.com:elastic/kibana into search/search…

61d1f76

…-timeout-setting

docs

d5c6042

majagrubic approved these changes Sep 8, 2020

View reviewed changes

lizozom merged commit 52c12ea into elastic:master Sep 9, 2020

lizozom mentioned this pull request Sep 9, 2020

[7.x] [Search] Add a new advanced setting searchTimeout (#75728) #77011

Merged

FrankHassanabad mentioned this pull request Feb 11, 2021

[Security Solutions] Fixes performance issue of track_total_hits not having an effect when used with search strategy #91068

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search] Add a new advanced setting searchTimeout #75728

[Search] Add a new advanced setting searchTimeout #75728

lizozom commented Aug 23, 2020 •

edited

Loading

elasticmachine commented Aug 23, 2020

lukeelmers Aug 24, 2020

lukasolson Aug 24, 2020

lizozom Aug 26, 2020

lizozom commented Aug 26, 2020

lizozom commented Aug 27, 2020

lukasolson left a comment

lizozom commented Sep 1, 2020

lukasolson left a comment

lizozom commented Sep 2, 2020

lizozom commented Sep 3, 2020

lukasolson commented Sep 5, 2020

lizozom commented Sep 6, 2020

lizozom commented Sep 6, 2020

lizozom commented Sep 7, 2020

majagrubic left a comment

kibanamachine commented Sep 8, 2020

[Search] Add a new advanced setting searchTimeout #75728

[Search] Add a new advanced setting searchTimeout #75728

Conversation

lizozom commented Aug 23, 2020 • edited Loading

Summary

Dev Docs

Checklist

For maintainers

elasticmachine commented Aug 23, 2020

lukeelmers Aug 24, 2020

Choose a reason for hiding this comment

lukasolson Aug 24, 2020

Choose a reason for hiding this comment

lizozom Aug 26, 2020

Choose a reason for hiding this comment

lizozom commented Aug 26, 2020

lizozom commented Aug 27, 2020

lukasolson left a comment

Choose a reason for hiding this comment

lizozom commented Sep 1, 2020

lukasolson left a comment

Choose a reason for hiding this comment

lizozom commented Sep 2, 2020

lizozom commented Sep 3, 2020

lukasolson commented Sep 5, 2020

lizozom commented Sep 6, 2020

lizozom commented Sep 6, 2020

lizozom commented Sep 7, 2020

majagrubic left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 8, 2020

💚 Build Succeeded

Build metrics

page load bundle size

distributable file count

History

lizozom commented Aug 23, 2020 •

edited

Loading