[data.search] Set default expiration to 1m if search sessions are disabled by lukasolson · Pull Request #105329 · elastic/kibana

lukasolson · 2021-07-12T20:56:19Z

Summary

Prior to this PR, when search sessions were disabled, we would still send 7d (by default) as the expiration for async search requests, meaning many of these requests were kept alive longer than necessary.

This PR sends the default expiration of 1m when search sessions are disabled so the async search requests are cleaned up properly.

Checklist

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

…abled

elasticmachine · 2021-07-12T20:56:21Z

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

src/plugins/data/server/search/strategies/ese_search/request_utils.ts

Dosant

Code lgtm, but while I was testing this out I realized that this basically means that you can't sit and wait while you dashboard is loading for more than 1 minute anymore if search sessions are disabled.

Before this pr, you still could sit and wait until the slow dashboard loads.

What happens now after one minute is we have an unclear user error:

And in console:

resource_not_found_exception: [resource_not_found_exception] Reason: FjRuR2VFMlBwVHJhbk9RMDlSUGFsV2ccTW03YlpwbFBRMjZTNVhxXzgwVTFoUToxMjMyOA==

So with that in mind, I am not sure now if we want this change, especially considering that we DELETE searches when we are navigating away (doesn't work in 100% of cases, but better then if we'd not delete them at all)

Curious what y'all think about this

lukasolson · 2021-07-14T21:59:30Z

Hmm, it looks like that behavior is happening because we are still sending sessionId in the options, even when search sessions is disabled. I thought we had fixed that at some point. We have a couple options here: We can either stop sending sessionId when it's disabled or we can pass the config into the getDefaultAsyncGetParams and check there instead of relying on whether sessionId is present or not. What do you think?

Dosant · 2021-07-15T09:44:27Z

it looks like that behavior is happening because we are still sending sessionId in the options, even when search sessions is disabled. I thought we had fixed that at some point.

Right, we didn't change it. We added server-side checks instead.
Originally we still sent sessionId to the server, because the code that controls it is in src/ and disabled config is in x-pack/ and it was tricky to get it. That is solvable, but now we also have a client-side caching mechanism that relies on sessionId. We still want caching to work. So client-side approach could now only be to use sessionId everywhere, but then to strip it out just before the request is sent. I don't think we want this to be that way.

I think the question is what is expected behaviour from a user perspective:
Should a user be able to wait for the long-running dashboard to finish loading when the search session is disabled? I think so? But then the question what is a time limit? If none, then I assume we can leave everything as is?

Chatted with @lizozom, now I understand part about getDefaultAsyncGetParams, I think we should do this:

we can pass the config into the getDefaultAsyncGetParams and check there instead of relying on whether sessionId is present or not

👍

Dosant · 2021-07-20T10:09:09Z

@elasticmachine merge upstream

lizozom

Added some changes but other than that from my side LGTM

kibanamachine · 2021-07-20T18:36:34Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #139704 succeeded d75bdff
💔 Build #139686 failed b92b836
💔 Build #139676 failed aadbe2e
💔 Build #138329 failed 9766368
💔 Build #137992 failed f567031

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

cc @lukasolson

Dosant

@lizozom, nice changes 👍 a lot cleaner

…abled (elastic#105329) Co-authored-by: Anton Dosov <anton.dosov@elastic.co> Co-authored-by: Liza K <liza.katz@elastic.co>

kibanamachine · 2021-07-21T09:13:46Z

💚 Backport successful

Status	Branch	Result
✅	7.14
✅	7.x

The backport PRs will be merged automatically after passing CI.

…abled (#105329) (#106366) Co-authored-by: Anton Dosov <anton.dosov@elastic.co> Co-authored-by: Liza K <liza.katz@elastic.co> Co-authored-by: Lukas Olson <olson.lukas@gmail.com> Co-authored-by: Anton Dosov <anton.dosov@elastic.co> Co-authored-by: Liza K <liza.katz@elastic.co>

…abled (#105329) (#106367) Co-authored-by: Anton Dosov <anton.dosov@elastic.co> Co-authored-by: Liza K <liza.katz@elastic.co> Co-authored-by: Lukas Olson <olson.lukas@gmail.com> Co-authored-by: Anton Dosov <anton.dosov@elastic.co> Co-authored-by: Liza K <liza.katz@elastic.co>

…y-show-migrate-to-authzd-users * 'master' of github.com:elastic/kibana: (48 commits) [Canvas] Expression shape (elastic#103219) [FTR] Skips Vega tests [Sample data] Use Lens in ecommerce data (elastic#106039) [APM] Backends inventory & overview page routes (elastic#106223) [TSVB] Add more functional tests for Gauge and TopN (elastic#105361) Add toggle to enable/disable rule install from SOs (elastic#106189) Improve unit test coverage of FS API calls (elastic#106242) Remove recursive plugin status in meta field (elastic#106286) [Ingest pipelines] add community id processor (elastic#103863) [XY axis] Fixes the values inside bar charts (elastic#106198) [data.search] Set default expiration to 1m if search sessions are disabled (elastic#105329) set the doc title when navigating to reporting and unset when navigating away (elastic#106253) [Lens] Display legend inside chart (elastic#105571) [RAC] [TGrid] Migrate the TGrid's rendering to `EuiDataGrid` (elastic#106199) [Security Solutions] Removes the elastic legacy client from lists and security_solution plugins (elastic#106130) [Enterprise Search] Require security plugin in 8.0 (elastic#106307) [DOCS] Updates screenshots in Dev Tools docs (elastic#105859) [DOCS] Updates text and screenshots in tags doc (elastic#105853) [Alerting] Allow rule types to extract/inject saved object references on rule CRU (elastic#101896) Jest and Storybook fixes (elastic#104991) ... # Conflicts: # x-pack/plugins/reporting/public/plugin.ts

[data.search] Set default expiration to 1m if search sessions are dis…

f567031

…abled

lukasolson requested a review from lizozom July 12, 2021 20:56

lukasolson requested a review from a team as a code owner July 12, 2021 20:56

lukasolson self-assigned this Jul 12, 2021

Dosant reviewed Jul 13, 2021

View reviewed changes

src/plugins/data/server/search/strategies/ese_search/request_utils.ts Outdated Show resolved Hide resolved

Use keep_on_completion conditionally

9766368

lukasolson requested a review from Dosant July 13, 2021 23:30

Dosant reviewed Jul 14, 2021

View reviewed changes

kibanamachine and others added 4 commits July 20, 2021 06:09

Merge branch 'master' into search-default-expiration

aadbe2e

fix get

b92b836

fix eql strategy util usage

d75bdff

adjust request params

a5bea2a

lizozom approved these changes Jul 20, 2021

View reviewed changes

Dosant approved these changes Jul 21, 2021

View reviewed changes

Dosant added the v7.15.0 label Jul 21, 2021

Dosant merged commit a75ba8f into elastic:master Jul 21, 2021

kibanamachine mentioned this pull request Jul 21, 2021

[7.14] [data.search] Set default expiration to 1m if search sessions are disabled (#105329) #106366

Merged

kibanamachine mentioned this pull request Jul 21, 2021

[7.x] [data.search] Set default expiration to 1m if search sessions are disabled (#105329) #106367

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[data.search] Set default expiration to 1m if search sessions are disabled#105329

[data.search] Set default expiration to 1m if search sessions are disabled#105329
Dosant merged 6 commits intoelastic:masterfrom
lukasolson:search-default-expiration

lukasolson commented Jul 12, 2021

Uh oh!

elasticmachine commented Jul 12, 2021

Uh oh!

Uh oh!

Dosant left a comment •

edited

Loading

Uh oh!

lukasolson commented Jul 14, 2021

Uh oh!

Dosant commented Jul 15, 2021 •

edited

Loading

Uh oh!

Dosant commented Jul 20, 2021

Uh oh!

lizozom left a comment

Uh oh!

kibanamachine commented Jul 20, 2021

Uh oh!

Dosant left a comment

Uh oh!

kibanamachine commented Jul 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

lukasolson commented Jul 12, 2021

Summary

Checklist

Uh oh!

elasticmachine commented Jul 12, 2021

Uh oh!

Uh oh!

Dosant left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasolson commented Jul 14, 2021

Uh oh!

Dosant commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dosant commented Jul 20, 2021

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 20, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

Dosant left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 21, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Dosant left a comment •

edited

Loading

Dosant commented Jul 15, 2021 •

edited

Loading