[Search] Unify search plugin step 3 — Rollup search strategy #98122

Dosant · 2021-04-23T12:11:07Z

Summary

Partially addresses #92802

As part of unifying the search plugin, we agree that we want to move rollup search into a separate search strategy.

Before this PR:

step 1 done in [Search] Unify search plugin step 1 #95811
step 2 done in [Search] Unify search plugin step 2 — Search Interceptor #97845

This PR:

Split rollups out of async search strategy

Still TODO In separate pr:

Move rest of search/session to data (blocked by security and taskManager, at this point teams have no plans to moving them)
Update old docs / example plugins / READMEs with information about the new structure

Checklist

Delete any items that are not applicable to this PR.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-04-23T17:12:50Z

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

lizozom

Overall look good.
Added a couple of comments.

src/plugins/data/server/search/strategies/rollup_search/rollup_search_strategy.ts

src/plugins/data/server/search/strategies/rollup_search/rollup_search_strategy.test.ts

src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts

x-pack/test/api_integration/apis/search/search.ts

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

Dosant · 2021-04-28T10:01:07Z

@elasticmachine merge upstream

# Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.searchinterceptor.getserializableoptions.md # src/plugins/data/public/public.api.md

lizozom

If you try viewing a rollup index in discover, it now uses the wrong strategy and fails.
This happens because of the changes introduced by #91517.

See src/plugins/discover/public/application/helpers/update_search_source.ts:57.

Dosant · 2021-05-12T17:49:47Z

@lizozom, could take another closer look, please 🙏 I compared behavior with master and it seems to me it is the same.
Could it be a feature in master is broken atm?

If you try viewing a rollup index in discover, it now uses the wrong strategy and fails.
What is the error you see?

Dosant · 2021-05-17T10:30:50Z

@elasticmachine merge upstream

# Conflicts: # src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts

kertal · 2021-05-25T14:36:39Z

qq: when I'm using a rollup index pattern shouldn't strategy change in the request?

Dosant · 2021-05-26T10:30:25Z

qq: when I'm using a rollup index pattern shouldn't strategy change in the request?

@kertal, that is the question I wanted to ask you, and also would like @lizozom to double-check (#98122 (review)) 🙏
I am comparing current discover behavior with the main branch and it looks like in the main branch discover isn't sending indexType: 'rollup' inside ese strategy request which is equal to not using 'rollup' search strategy in this branch.

I have very limited knowledge of how that part should work in Discover, but from what I see it look like either this pr works as expected or we have a bug in the main branch.

kertal · 2021-05-26T10:59:46Z

qq: when I'm using a rollup index pattern shouldn't strategy change in the request?

@kertal, that is the question I wanted to ask you, and also would like @lizozom to double-check (#98122 (review)) 🙏
I am comparing current discover behavior with the main branch and it looks like in the main branch discover isn't sending indexType: 'rollup' inside ese strategy request which is equal to not using 'rollup' search strategy in this branch.

I have very limited knowledge of how that part should work in Discover, but from what I see it look like either this pr works as expected or we have a bug in the main branch.

Also not sure about this (tested rollups with this PR and it seemed to work), and you know #96766 is around the corner, which changes tons of stuff here. So the question is, what should be sent, and ideally after #96766 we could adapt it.

lizozom · 2021-05-30T16:11:44Z

@Dosant @kertal #96766 doesn't change anything about setPreferredSearchStrategyId, so I don't think it should matter.
I think as long as there are no regressions when querying rollup indices in discover, we should be ok with this.

lizozom

I don't see #96766 changing the behavior of setPreferredSearchStrategyId, so I don't think these two PRs are related in this sense.
I now understand why discover fetches documnets with the default strategy, regardless of the index type - because it only needs the raw documents, without any aggs.
LGTM!

lizozom · 2021-05-30T16:35:00Z

@elasticmachine merge upstream

kibanamachine · 2021-05-30T18:21:23Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`data`	570	572	+2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`data`	3223	3225	+2

Page load bundle

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

id	before	after	diff
`data`	817.7KB	818.3KB	+555.0B

Unknown metric groups

API count

id	before	after	diff
`data`	3768	3770	+2

History

💚 Build #127069 succeeded 2d7fee4
💚 Build #126190 succeeded b176150
💚 Build #124963 succeeded 88827fe
💚 Build #124754 succeeded 63e2165
💚 Build #122917 succeeded 32bb6fa

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

ppisljar · 2022-02-10T07:46:48Z

@Dosant should this be closed ?

Dosant · 2022-02-10T10:59:00Z

Let's revisit this later

rollup search strategy

e2aaa42

Dosant added Feature:Search Querying infrastructure in Kibana refactoring technical debt Improvement of the software architecture and operational architecture v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes v7.14.0 labels Apr 23, 2021

Dosant changed the title ~~[Search] Unify search plugin step 2 — Rollup search strategy~~ [Search] Unify search plugin step 3 — Rollup search strategy Apr 23, 2021

Dosant mentioned this pull request Apr 23, 2021

[Search] Unify data.search plugin #92802

Closed

10 tasks

fix rollup in timeseries

2b08df0

Dosant marked this pull request as ready for review April 23, 2021 17:12

Dosant requested a review from a team April 23, 2021 17:12

Dosant requested a review from a team as a code owner April 23, 2021 17:12

Dosant requested review from lizozom and lukasolson April 23, 2021 17:13

lizozom reviewed Apr 25, 2021

View reviewed changes

kibanamachine and others added 6 commits April 28, 2021 06:01

Merge branch 'master' into unify-search-3

32bb6fa

Merge branch 'master' of github.com:elastic/kibana into unify-search-3

439d5af

# Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.searchinterceptor.getserializableoptions.md # src/plugins/data/public/public.api.md

Merge branch 'master' of github.com:elastic/kibana into unify-search-3

56ba84d

review improvements

63e2165

add a functional test calling rollup search with non-rollup index

48a1745

Merge branch 'master' of github.com:elastic/kibana into unify-search-3

88827fe

Dosant requested a review from lizozom May 10, 2021 07:51

lizozom suggested changes May 10, 2021

View reviewed changes

Dosant marked this pull request as draft May 10, 2021 14:54

Merge branch 'master' into unify-search-3

b176150

Dosant requested a review from lizozom May 18, 2021 09:21

Merge branch 'master' of github.com:elastic/kibana into unify-search-3

2d7fee4

# Conflicts: # src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts

Dosant requested a review from kertal May 25, 2021 10:39

Dosant marked this pull request as ready for review May 25, 2021 13:22

lizozom approved these changes May 30, 2021

View reviewed changes

Merge branch 'master' into unify-search-3

ef1a861

spalger added v7.15.0 and removed v7.14.0 labels Jun 30, 2021

mistic added v7.16.0 and removed v7.15.0 labels Aug 18, 2021

timroes requested review from a team and removed request for a team August 31, 2021 15:05

Dosant mentioned this pull request Nov 11, 2021

Combine es and ese search strategies #118165

Closed

spalger added v7.17.0 and removed v7.16.0 labels Dec 7, 2021

Dosant closed this Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search] Unify search plugin step 3 — Rollup search strategy #98122

[Search] Unify search plugin step 3 — Rollup search strategy #98122

Dosant commented Apr 23, 2021 •

edited

Loading

elasticmachine commented Apr 23, 2021

lizozom left a comment

Dosant commented Apr 28, 2021

lizozom left a comment

Dosant commented May 12, 2021

Dosant commented May 17, 2021

kertal commented May 25, 2021

Dosant commented May 26, 2021

kertal commented May 26, 2021

lizozom commented May 30, 2021

lizozom left a comment

lizozom commented May 30, 2021

kibanamachine commented May 30, 2021

API count

ppisljar commented Feb 10, 2022

Dosant commented Feb 10, 2022

[Search] Unify search plugin step 3 — Rollup search strategy #98122

[Search] Unify search plugin step 3 — Rollup search strategy #98122

Conversation

Dosant commented Apr 23, 2021 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Apr 23, 2021

lizozom left a comment

Choose a reason for hiding this comment

Dosant commented Apr 28, 2021

lizozom left a comment

Choose a reason for hiding this comment

Dosant commented May 12, 2021

Dosant commented May 17, 2021

kertal commented May 25, 2021

Dosant commented May 26, 2021

kertal commented May 26, 2021

lizozom commented May 30, 2021

lizozom left a comment

Choose a reason for hiding this comment

lizozom commented May 30, 2021

kibanamachine commented May 30, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Page load bundle

API count

History

ppisljar commented Feb 10, 2022

Dosant commented Feb 10, 2022

Dosant commented Apr 23, 2021 •

edited

Loading