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.

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

[Search] Unify search plugin step 1 #95811

Merged
merged 26 commits into from
Apr 21, 2021
Merged

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Mar 30, 2021

Summary

Partially addresses #92802

In this pr:

  • Remove the defaultStrategy override
  • Move async search strategy to data
  • Move EQL search strategy to data
  • Move rest of common/search/session data (Moving whole search/session is blocked by security and taskManager)

Still TODO In separate pr:

  • Unify search interceptor (Wait for caching PR to avoid merges)
  • Deprecate TimeoutErrorMode.UPGRADE (Wait for caching PR to avoid merges)
  • Split rollups out of async search strategy - (Will pick up next)
  • Move rest of search/session to data (blocked by security and taskManager)

Checklist

For maintainers

Dosant added 20 commits March 30, 2021 17:57
# Conflicts:
#	src/plugins/data/server/search/search_service.ts
# Conflicts:
#	docs/development/plugins/data/public/kibana-plugin-plugins-data-public.searchbar.md
#	src/plugins/data/README.mdx
#	src/plugins/data/server/search/search_service.ts
#	x-pack/plugins/data_enhanced/config.ts
#	x-pack/plugins/data_enhanced/server/plugin.ts
#	x-pack/plugins/data_enhanced/server/search/session/check_running_sessions.ts
# Conflicts:
#	src/plugins/data/common/search/types.ts
#	src/plugins/data/server/server.api.md
# Conflicts:
#	x-pack/plugins/data_enhanced/config.ts
#	x-pack/plugins/data_enhanced/server/search/session/check_running_sessions.test.ts
#	x-pack/plugins/data_enhanced/server/search/session/check_running_sessions.ts
This reverts commit 1ea7325.
@@ -30,3 +30,62 @@ export const configSchema = schema.object({
});

export type ConfigSchema = TypeOf<typeof configSchema>;

export const searchSessionsConfigSchema = schema.object({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to move the schema now to src/ because ese strategy uses defaultExpiration from it.
This schema is still registered by x-pack/ code

@@ -183,6 +184,11 @@ export class SearchInterceptor {
request: IKibanaSearchRequest,
options: ISearchOptions = {}
): Observable<IKibanaSearchResponse> {
options = {
strategy: ES_SEARCH_STRATEGY,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

search_interceptor will be simplified in follow-up pr. for now I had to specify non async search strategy here, otherwise it would hit async and won't handle partial results.

@@ -31,6 +32,7 @@ export interface IScopedSearchSessionsClient<T = unknown> {
cancel: (sessionId: string) => Promise<{}>;
delete: (sessionId: string) => Promise<{}>;
extend: (sessionId: string, expires: Date) => Promise<SavedObjectsUpdateResponse<T>>;
getConfig: () => SearchSessionsConfigSchema | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SearchSession still leave in x-pack, but ese strategy needs defaultExpiration from its config

test/api_integration/apis/search/bsearch.ts Outdated Show resolved Hide resolved
test/api_integration/apis/search/bsearch.ts Outdated Show resolved Hide resolved
@Dosant Dosant requested review from lizozom and lukasolson April 13, 2021 14:43
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.

Reviewed as draft, without testing.
Looks like a good first step

src/plugins/data/server/search/search_service.test.ts Outdated Show resolved Hide resolved
Dosant added 2 commits April 19, 2021 15:54
# Conflicts:
#	src/plugins/data/common/search/types.ts
#	x-pack/plugins/data_enhanced/public/search/search_interceptor.ts
@Dosant Dosant added Feature:Search Querying infrastructure in Kibana refactoring labels Apr 19, 2021
@Dosant Dosant added release_note:skip Skip the PR/issue when compiling release notes technical debt Improvement of the software architecture and operational architecture v7.14.0 v8.0.0 Team:AppServices labels Apr 19, 2021
@Dosant Dosant marked this pull request as ready for review April 19, 2021 16:25
@Dosant Dosant requested review from a team as code owners April 19, 2021 16:25
@elasticmachine
Copy link
Contributor

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

@Dosant Dosant requested a review from lizozom April 19, 2021 16:25
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.

LGTM

Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

Security solution are approved, I make sure to test it locally ;)

Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

Asset management LGTM

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Left a couple of minor nits, but code LGTM, and tested locally and things seem to be working great!

src/plugins/data/README.mdx Outdated Show resolved Hide resolved
src/plugins/data/server/search/search_service.test.ts Outdated Show resolved Hide resolved
@Dosant Dosant mentioned this pull request Apr 21, 2021
10 tasks
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 562 567 +5
dataEnhanced 104 99 -5
total -0

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dataEnhanced 159.0KB 159.2KB +232.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 790.1KB 792.9KB +2.9KB
dataEnhanced 35.1KB 33.1KB -2.1KB
total +819.0B
Unknown metric groups

API count

id before after diff
data 3454 3476 +22
dataEnhanced 52 16 -36
total -14

API count missing comments

id before after diff
data 2958 2979 +21
dataEnhanced 34 16 -18
total +3

History

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

@Dosant Dosant merged commit bd4f461 into elastic:master Apr 21, 2021
Dosant added a commit to Dosant/kibana that referenced this pull request Apr 21, 2021
Remove the defaultStrategy override
Move async search strategy to data
Move EQL search strategy to data
Move rest of common/search/session data (Moving whole search/session is blocked by security and taskManager)
Dosant added a commit that referenced this pull request Apr 21, 2021
Remove the defaultStrategy override
Move async search strategy to data
Move EQL search strategy to data
Move rest of common/search/session data (Moving whole search/session is blocked by security and taskManager)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Search Querying infrastructure in Kibana refactoring release_note:skip Skip the PR/issue when compiling release notes technical debt Improvement of the software architecture and operational architecture v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants