[Search Sessions] fix saved object can be created even if courier:batchSearches is enabled#105407
Conversation
| if (!options?.strategy && syncSearchByDefault) { | ||
| options.strategy = ES_SEARCH_STRATEGY; | ||
| // `ES_SEARCH_STRATEGY` doesn't support search sessions, hence remove sessionId | ||
| options.sessionId = undefined; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Won't this disable client side caching of responses as well 🤔 ?
There was a problem hiding this comment.
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
|
Pinging @elastic/kibana-app-services (Team:AppServices) |
lizozom
left a comment
There was a problem hiding this comment.
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.
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
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)
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…chSearches is enabled (elastic#105407)
💚 Backport successful
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
Summary
This pr fixes #104311: the issue is when
courier:batchSearchesenabled (which essentially switches all searches that go through thesearchSourcefromasynctosyncsearch), 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:batchSearchesis applied on SearchSource level and, for example, tsvb doesn't use it.In this case, only async searches will use
sessionIdand 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:
this is incorrect usage because
essearch 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
Risk Matrix
Fix is very contained. Change is applied only when
courier:batchSearchesis used.For maintainers