[Search Sessions] fix saved object can be created even if courier:batchSearches is enabled #105407

Dosant · 2021-07-13T10:16:38Z

Summary

This pr fixes #104311: the issue is when courier:batchSearches enabled (which essentially switches all searches that go through the searchSource from async to sync search), we still treat those as they are async from a search session perspective.

There are a lot of ways how to handle this, but since this fix is focused on courier:batchSearches, I suggest we contain the fix together with the option usage.

Considerations

Dashboard with different panels

With this option enabled you can have a dashboard that uses both async search and sync search, because courier:batchSearches is applied on SearchSource level and, for example, tsvb doesn't use it.

In this case, only async searches will use sessionId and only those will get into the session object.

Using search strategy that doesn't support search session

You can still use search service as follows:

search({strategy: 'es', sessionId: 'id'})

this is incorrect usage because es search strategy doesn't support sessions, but it will still create all the session overhead.

@lizozom, @lukasolson, Should I create an issue to improve this?

Checklist

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

Risk Matrix

Fix is very contained. Change is applied only when courier:batchSearches is used.

For maintainers

This was checked for breaking API changes and was labeled appropriately

Dosant · 2021-07-13T10:46:37Z

src/plugins/data/common/search/search_source/search_source.ts

@@ -287,6 +287,8 @@ export class SearchSource {
    // This still uses bfetch for batching.
    if (!options?.strategy && syncSearchByDefault) {
      options.strategy = ES_SEARCH_STRATEGY;
+      // `ES_SEARCH_STRATEGY` doesn't support search sessions, hence remove sessionId
+      options.sessionId = undefined;


There are a lot of ways how to handle this, but since this fix is focused on courier:batchSearches, I suggest we contain the fix together with the option usage.

Long term, I think we find a how-to avoid search session overhead (both on UI and server) when running a search strategy that doesn't support search sessions. (see pr description for more details)

Won't this disable client side caching of responses as well 🤔 ?

Yes @lizozom!

But otherwise not sure how to solve it also keeping the fix surface as small as possible:

We want cache to still work, so we need to pass sessionId,
but then we also don't want to trackSearch in client-side session service and ideally don't want to pass sessionId to the server.

I don't think we should put such conditional logic that depends on a specific strategy into client-side search interceptor, hence I suggest going with removing sessionId in searchSource level

elasticmachine · 2021-07-13T13:29:52Z

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

lizozom

I tested this change and if I go to discover, the search goes out using es strategy but I still see the indicator and I can still save the session.

lizozom

Nothing like a good old kbn clean

Would be nice to have a functional test for this as well :-)

Dosant · 2021-07-15T07:56:37Z

@elasticmachine merge upstream

…-enabled

kibanamachine · 2021-07-15T10:07:32Z

💚 Build Succeeded

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`	839.9KB	840.0KB	+25.0B

History

💚 Build #138106 succeeded 81b69c9

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

Dosant · 2021-07-15T10:56:49Z

Would be nice to have a functional test for this as well :-)

Discussed offline. Intentionally, adding only a unit test because the fix is very contained and only for a deprecated option we plan to remove soon

…chSearches is enabled (elastic#105407)

kibanamachine · 2021-07-15T11:00:22Z

💚 Backport successful

Status	Branch	Result
✅	7.14

This backport PR will be merged automatically after passing CI.

…chSearches is enabled (elastic#105407)

kibanamachine · 2021-07-15T11:36:16Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…-png-pdf-report-type * 'master' of github.com:elastic/kibana: (75 commits) [Search Sessions] Don’t try to delete errored searches (elastic#105434) [Search Sessions] fix saved object can be created even if courier:batchSearches is enabled (elastic#105407) [Remote Clusters] Fixed remote clusters details flyout for long strings (elastic#105592) [ML] Functional tests - re-activate a11y tests (elastic#105198) [APM] Typed client-side routing (elastic#104274) [Canvas] Expression error (elastic#103048) [ML] Fixing job wizard with missing description (elastic#105574) [Security Solution][Alerts] - Add alerts subfeature UI (elastic#105505) Upgrade EUI to v35.0.0 (elastic#105127) [Reporting] Clean up types for internal APIs needed for UI (elastic#105508) skip flaky suite (elastic#105087) [Workplace Search] Fix Chrome issues with GitHub sources (elastic#105680) [Fleet] Add containerized fleet server instructions to Fleet README (elastic#105669) [ML] Add api integration test for analytics map endpoint (elastic#105531) Fixes cypress flake across two tests (elastic#105645) [Logs&Metrics UI] add owner properties to plugin manifest (elastic#105580) chore(NA): introduce preset for jest-integration tests on @kbn/test (elastic#105144) [Enterprise Search] Added Thumbnails to Search UI (elastic#104199) Translate App Search credentials list (elastic#105619) [APM] APM agent config created prior to Fleet migration is not injected into integration policy (elastic#105504) ... # Conflicts: # x-pack/plugins/reporting/common/types.ts # x-pack/plugins/reporting/public/management/report_listing.test.tsx

…chSearches is enabled (#105407) (#105733) Co-authored-by: Anton Dosov <anton.dosov@elastic.co>

…chSearches is enabled (#105407) (#105738) Co-authored-by: Anton Dosov <anton.dosov@elastic.co>

Dosant added 2 commits July 13, 2021 11:53

disable search sessions when sync search

cd945cb

better test name

81b69c9

Dosant commented Jul 13, 2021

View reviewed changes

Dosant marked this pull request as ready for review July 13, 2021 13:29

Dosant requested a review from a team as a code owner July 13, 2021 13:29

Dosant requested review from lizozom and lukasolson July 13, 2021 13:29

Dosant added bug Fixes for quality problems that affect the customer experience Feature:Search Querying infrastructure in Kibana v7.14.0 Team:AppServices labels Jul 13, 2021

Dosant added release_note:fix impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels Jul 13, 2021

lizozom suggested changes Jul 14, 2021

View reviewed changes

lizozom approved these changes Jul 14, 2021

View reviewed changes

Merge branch 'master' into dev/disable-search-session-when-old-search…

b1f09e7

…-enabled

Dosant added the auto-backport Deprecated - use backport:version if exact versions are needed label Jul 15, 2021

Dosant merged commit 4b4a405 into elastic:master Jul 15, 2021

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 15, 2021

[Search Sessions] fix saved object can be created even if courier:bat…

327296b

…chSearches is enabled (elastic#105407)

kibanamachine mentioned this pull request Jul 15, 2021

[7.14] [Search Sessions] fix saved object can be created even if courier:batchSearches is enabled (#105407) #105733

Merged

Dosant added v7.15.0 v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed and removed auto-backport Deprecated - use backport:version if exact versions are needed labels Jul 15, 2021

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 15, 2021

[Search Sessions] fix saved object can be created even if courier:bat…

6db4e68

…chSearches is enabled (elastic#105407)

kibanamachine mentioned this pull request Jul 15, 2021

[7.x] [Search Sessions] fix saved object can be created even if courier:batchSearches is enabled (#105407) #105738

Merged

kibanamachine added a commit that referenced this pull request Jul 15, 2021

[Search Sessions] fix saved object can be created even if courier:bat…

0e44fc8

…chSearches is enabled (#105407) (#105733) Co-authored-by: Anton Dosov <anton.dosov@elastic.co>

kibanamachine added a commit that referenced this pull request Jul 15, 2021

[Search Sessions] fix saved object can be created even if courier:bat…

9629611

…chSearches is enabled (#105407) (#105738) Co-authored-by: Anton Dosov <anton.dosov@elastic.co>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search Sessions] fix saved object can be created even if courier:batchSearches is enabled #105407

[Search Sessions] fix saved object can be created even if courier:batchSearches is enabled #105407

Dosant commented Jul 13, 2021 •

edited

Loading

Dosant Jul 13, 2021

lizozom Jul 13, 2021 •

edited

Loading

Dosant Jul 14, 2021

elasticmachine commented Jul 13, 2021

lizozom left a comment

lizozom left a comment •

edited

Loading

Dosant commented Jul 15, 2021

kibanamachine commented Jul 15, 2021

Dosant commented Jul 15, 2021

kibanamachine commented Jul 15, 2021

kibanamachine commented Jul 15, 2021

[Search Sessions] fix saved object can be created even if courier:batchSearches is enabled #105407

[Search Sessions] fix saved object can be created even if courier:batchSearches is enabled #105407

Conversation

Dosant commented Jul 13, 2021 • edited Loading

Summary

Considerations

Dashboard with different panels

Using search strategy that doesn't support search session

Checklist

Risk Matrix

For maintainers

Dosant Jul 13, 2021

Choose a reason for hiding this comment

lizozom Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Dosant Jul 14, 2021

Choose a reason for hiding this comment

elasticmachine commented Jul 13, 2021

lizozom left a comment

Choose a reason for hiding this comment

lizozom left a comment • edited Loading

Choose a reason for hiding this comment

Dosant commented Jul 15, 2021

kibanamachine commented Jul 15, 2021

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

Dosant commented Jul 15, 2021

kibanamachine commented Jul 15, 2021

💚 Backport successful

kibanamachine commented Jul 15, 2021

💚 Backport successful

Dosant commented Jul 13, 2021 •

edited

Loading

lizozom Jul 13, 2021 •

edited

Loading

lizozom left a comment •

edited

Loading