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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Jul 21, 2021
Merged

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

merged 6 commits into from
Jul 21, 2021

Conversation

lukasolson
Copy link
Member

Summary

Resolves #104405.

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

@lukasolson lukasolson added review Feature:Search Querying infrastructure in Kibana v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes Project:AsyncSearch Background search, partial results, async search services. v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jul 12, 2021
@lukasolson lukasolson requested a review from lizozom July 12, 2021 20:56
@lukasolson lukasolson requested a review from a team as a code owner July 12, 2021 20:56
@lukasolson lukasolson self-assigned this Jul 12, 2021
@elasticmachine
Copy link
Contributor

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

@lukasolson lukasolson requested a review from Dosant July 13, 2021 23:30
Dosant
Dosant reviewed Jul 14, 2021
View reviewed changes
Copy link
Contributor

@Dosant Dosant left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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:

Screen Shot 2021-07-14 at 13 24 11

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
Copy link
Member Author

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
Copy link
Contributor

Dosant commented Jul 15, 2021 •
edited
Loading

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
Copy link
Contributor

Dosant commented Jul 20, 2021

@elasticmachine merge upstream

lizozom
lizozom approved these changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Added some changes but other than that from my side LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @lukasolson

Dosant
Dosant approved these changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

@lizozom, nice changes 👍 a lot cleaner

@Dosant Dosant added the v7.15.0 label Jul 21, 2021
@Dosant Dosant merged commit a75ba8f into elastic:master Jul 21, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 21, 2021
@lukasolson @Dosant @kibanamachine
[data.search] Set default expiration to 1m if search sessions are dis…
29ef40a 
…abled (elastic#105329)

Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
Co-authored-by: Liza K <liza.katz@elastic.co>
@kibanamachine kibanamachine mentioned this pull request Jul 21, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 21, 2021
@lukasolson @Dosant @kibanamachine
[data.search] Set default expiration to 1m if search sessions are dis…
0d96e5a 
…abled (elastic#105329)

Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
Co-authored-by: Liza K <liza.katz@elastic.co>
@kibanamachine kibanamachine mentioned this pull request Jul 21, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.14
✅ 7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jul 21, 2021
@kibanamachine @lukasolson @Dosant
[data.search] Set default expiration to 1m if search sessions are dis…
e898416 
…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>
kibanamachine added a commit that referenced this pull request Jul 21, 2021
@kibanamachine @lukasolson @Dosant
[data.search] Set default expiration to 1m if search sessions are dis…
f4276af 
…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>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 21, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into reporting/onl…
5160a33 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizozom lizozom lizozom approved these changes

@Dosant Dosant Dosant approved these changes

Assignees

@lukasolson lukasolson

Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Search Querying infrastructure in Kibana Project:AsyncSearch Background search, partial results, async search services. release_note:skip Skip the PR/issue when compiling release notes review v7.14.0 v7.15.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Search Sessions] Async searches have expiration date in a week when search sessions are disabled
5 participants
@lukasolson @elasticmachine @Dosant @kibanamachine @lizozom