[data.search.SearchSource] Remove legacy ES client APIs. by lukeelmers · Pull Request #75943 · elastic/kibana

lukeelmers · 2020-08-26T05:14:56Z

Part of #55139
Part of #65069
Relates to #75326

Summary

This PR removes the __LEGACY APIs from the client-side search service, thereby also removing our last dependency on elasticsearch-browser. A few things were needed to make this happen:

A new /internal/_msearch endpoint has been created to replace the need for /elasticsearch/_msearch, which is still used a few places (including Index Patterns UI -- there's a separate task to track removal of that usage). This basically proxies requests directly to ES, however the request format is slightly different in that it accepts an array of header / body objects to make it easier to inject some config onto each request on the server.
The legacy "default search strategy" is updated to call the new internal endpoint instead of using the legacy ES client.
The __LEGACY APIs are removed from the client-side search service.
Some msearch-specific search params which were always set on the client are now set on the server instead, to keep the client implementation simple.
Lastly, any traces of elasticsearch-browser I could find have been removed from the repo.

This also moves forward the efforts to remove esShardTimeout from the client which were started in #75326, however at the moment it only removes usages from the high-level search service.

Testing

You must enable the courier:batchSearches advanced setting (disabled by default) to test this. Check places using SearchSource, such as visualize, lens, maps, discover, dashboards.

Checklist

Documentation was added for features that require explanation or tutorials
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

Dev Docs

The __LEGACY APIs have been removed from the data plugin's client-side search service. Specifically data.search.__LEGACY.esClient is no longer exposed and the legacy elasticsearch-browser package has been removed from the repo. If you rely on this client in your plugin, we recommend migrating to the new elasticsearch-js client.

…rnal route.

lizozom

Tested these changes with search and msearch and seems to be ok.
Couldn't manage to create a rollup, to make sure everything works there as well.
I think its worth testing the rollup + msearch combination at least once.

src/plugins/data/public/search/legacy/default_search_strategy.ts

lizozom · 2020-08-26T09:32:33Z

src/plugins/data/public/search/legacy/default_search_strategy.ts

+  let resolved = false;
+
+  const loadingCount$ = new BehaviorSubject(0);
+  http.addLoadingCountSource(loadingCount$);


I don't think you want to add a new BehaviorSubject for every msearch. Especially as there's no way to un-register them.

This should be managed at a higher level, passing in the loadingCount$ to be used.

Moved loadingCount$ to search service

src/plugins/data/server/search/routes/msearch.ts

lizozom · 2020-08-26T09:37:30Z

src/plugins/data/server/search/routes/msearch.ts

+
+      // get shardTimeout
+      const config = await deps.globalConfig$.pipe(first()).toPromise();
+      const { timeout } = getDefaultSearchParams(config);


The correct timeout to use for his request is requestTimeout, rather than shardTimeout.
This was an implementation fault in the old implementation.

Hmm, I think timeout (which is the shard timeout) is the correct parameter here. It's the actual parameter being sent to ES. The requestTimeout should be automatically handled by the ES client.

I guess we can handle it separately from this PR 👍

lukasolson

LGTM, minor comments below

src/plugins/data/public/search/legacy/default_search_strategy.ts

lukasolson · 2020-08-26T17:59:54Z

src/plugins/data/server/search/routes/msearch.ts

+
+      // get shardTimeout
+      const config = await deps.globalConfig$.pipe(first()).toPromise();
+      const { timeout } = getDefaultSearchParams(config);


Hmm, I think timeout (which is the shard timeout) is the correct parameter here. It's the actual parameter being sent to ES. The requestTimeout should be automatically handled by the ES client.

src/plugins/data/server/search/routes/msearch.ts

lukeelmers · 2020-08-26T22:26:17Z

@elasticmachine merge upstream

lukeelmers · 2020-08-27T16:44:30Z

Couldn't manage to create a rollup, to make sure everything works there as well.
I think its worth testing the rollup + msearch combination at least once.

Thanks for the suggestion -- just tested this with rollups and I think there may be some issues -- Discover seems to work, however I'm not getting consistent aggregations in visualizations when toggling courier:batchSearches on and off.

Let's hold on merging and I can look into those next week.

spalger

🎉

lizozom · 2020-08-31T08:32:20Z

@elasticmachine merge upstream

lukasolson

On a second look, I'm noticing that preference is considered to be required in the _msearch route, but in reality you can set courier:setRequestPreference to "none" and then it's expected that no preference is set. Currently, this breaks the route. You get this error:

[request body.searches.0.header.preference]: expected value of type [number] but got [undefined]

Also, it can be set to a string, not just a number.

lukeelmers · 2020-09-01T16:28:48Z

@elasticmachine merge upstream

lukeelmers · 2020-09-01T22:23:05Z

@lukasolson I've fixed the error you caught & added some integration tests for it.

As for rollups: I'm blocked trying to figure out what's wrong with creating rollup index patterns (currently broken on master). Until I can sort through that I won't be able to confirm that this PR works as expected with rollups. Will update here with any progress updates.

src/plugins/data/server/search/routes/msearch.ts

lukeelmers · 2020-09-03T05:02:47Z

As for rollups: I'm blocked trying to figure out what's wrong with creating rollup index patterns (currently broken on master). Until I can sort through that I won't be able to confirm that this PR works as expected with rollups. Will update here with any progress updates.

I have a PR up which fixes the index patterns issue with rollups: #76593

After applying the change to this PR and testing, I can confirm that the behavior I'm seeing with this PR is also present on master: Visualizations aren't showing the same results for rollup index patterns when courier:batchSearches is toggled on and off.

The "good" news is that I think it's safe to move forward with this PR once #76593 lands. The bad news is that we have another unrelated issue to sort out 🙁

lukeelmers · 2020-09-03T05:02:59Z

@elasticmachine merge upstream

lizozom · 2020-09-03T12:18:36Z

@elasticmachine merge upstream

kibanamachine · 2020-09-03T14:40:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: bcb52b9

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
data	542	-4	546

async chunks size

id	value	diff	baseline
enterpriseSearch	372.4KB	-34.0B	372.4KB

page load bundle size

id	value	diff	baseline
data	1.4MB	-4.3KB	1.4MB
upgradeAssistant	64.6KB	-34.0B	64.6KB
total		-4.3KB

oss distributable file count

id	value	diff	baseline
total	27287	-7	27294

distributable file count

id	value	diff	baseline
total	45446	-7	45453

History

💔 Build #72219 failed 24172f5
💚 Build #72136 succeeded 975cb86
💚 Build #72026 succeeded e607f5e
💔 Build #71743 failed e0d6809
💚 Build #71620 succeeded bb20f61

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

lukasolson

LGTM!

* master: (340 commits) [data.search.SearchSource] Remove legacy ES client APIs. (elastic#75943) [release notes] automatically retry on Github API 5xx errors (elastic#76447) [es_ui_shared] Fix eslint exhaustive deps rule (elastic#76392) [i18n] Integrate 7.9.1 Translations (elastic#76391) [APM] Update aggregations to support script sources (elastic#76429) [Security Solution] Refactor Network Top Countries to use Search Strategy (elastic#76244) Document security settings available on ESS (elastic#76513) [Ingest Manager] Add input revision to the config send to the agent (elastic#76327) [DOCS] Identifies cloud settings for Monitoring (elastic#76579) [DOCS] Identifies Cloud settings in Dev Tools (elastic#76583) [Ingest Manager] Better default value for fleet long polling timeout (elastic#76393) [data.indexPatterns] Fix broken rollup index pattern creation (elastic#76593) [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558) [Security Solution] add an excess validation instead of the exact match (elastic#76472) Introduce TS incremental builds & move src/test_utils to TS project (elastic#76082) fix bad merge (elastic#76629) [Newsfeed] Ensure the version format when calling the API (elastic#76381) remove server_extensions mixin (elastic#76606) Remove legacy applications and legacy mode (elastic#75987) [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749) ...

…6676)

@ts-expect-error

…rok/new-patterns-component-use-array * 'master' of github.com:elastic/kibana: (75 commits) Remove legacy ui-apps-mixin (elastic#76604) remove unused test_utils (elastic#76528) [ML] Functional tests - add UI permission tests (elastic#76368) [APM] @ts-error -> @ts-expect-error (elastic#76492) [APM] Avoid negative offset for error marker on timeline (elastic#76638) [Reporting] rename interfaces to align with task manager integration (elastic#76716) Revert back ESO migration for alerting, added try/catch logic to avoid failing Kibana on start (elastic#76220) Test reverting "Add plugin status API (elastic#75819)" (elastic#76707) [Security Solution][Detections] Removes ML Job Settings SIEM copy and fixes link to ML app for creating custom jobs (elastic#76595) [Maps] remove region/coordinate-maps visualizations from sample data (elastic#76399) [DOCS] Dashboard-first docs refresh (elastic#76194) Updated ServiceNow description in docs and actions management UI to contains correct info (elastic#76344) [DOCS] Identifies cloud settings in reporting (elastic#76691) [Security Solution] Refactor timeline details to use search strategy (elastic#75591) es-archiver: Drop invalid index settings, support --query flag (elastic#76522) [DOCS] Identifies graph settings available on cloud (elastic#76661) Add more info about a11y tests (elastic#76045) [data.search.SearchSource] Remove legacy ES client APIs. (elastic#75943) [release notes] automatically retry on Github API 5xx errors (elastic#76447) [es_ui_shared] Fix eslint exhaustive deps rule (elastic#76392) ...

lukeelmers added 7 commits August 25, 2020 22:31

Move search server routes into a directory.

356281a

Add internal/_msearch route.

84ad62f

Remove legacy search API, rewrite default search strategy to use inte…

9a8de03

…rnal route.

Remove legacy es_client code.

371a4df

Handle msearch options on server.

2475467

Remove elasticsearch-browser dependency.

775586a

Update generated docs.

d7df81e

lukeelmers added Feature:Search Querying infrastructure in Kibana release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v8.0.0 Team:AppArch v7.10.0 labels Aug 26, 2020

lukeelmers requested review from lizozom and lukasolson August 26, 2020 05:14

lukeelmers self-assigned this Aug 26, 2020

lizozom reviewed Aug 26, 2020

View reviewed changes

lizozom mentioned this pull request Aug 26, 2020

[Search] Add a new advanced setting searchTimeout #75728

Merged

7 tasks

lukasolson approved these changes Aug 26, 2020

View reviewed changes

elasticmachine and others added 8 commits August 26, 2020 16:26

Merge branch 'master' into fix/remove-es-client

791a92f

Code review fixes

260d23c

code review

9540e4c

doc

6de4205

loading count

5049b35

simplify code review and fix jest tets

002c13c

Merge branch 'master' of github.com:elastic/kibana into pr/75943

3c07a71

type check

1dded0a

lizozom mentioned this pull request Aug 27, 2020

[Search] Remove client side shard timeout #75326

Closed

7 tasks

lizozom marked this pull request as ready for review August 27, 2020 14:02

lizozom requested a review from a team as a code owner August 27, 2020 14:02

spalger approved these changes Aug 27, 2020

View reviewed changes

Merge branch 'master' into fix/remove-es-client

8dd8f95

lukasolson requested changes Sep 1, 2020

View reviewed changes

elasticmachine and others added 2 commits September 1, 2020 10:28

Merge branch 'master' into fix/remove-es-client

bb20f61

Ensure preference is optional and can be provided as string or number.

e0d6809

lukasolson reviewed Sep 2, 2020

View reviewed changes

src/plugins/data/server/search/routes/msearch.ts Show resolved Hide resolved

Fix TS failures.

e607f5e

lukeelmers mentioned this pull request Sep 3, 2020

[data.indexPatterns] Fix broken rollup index pattern creation #76593

Merged

elasticmachine and others added 2 commits September 2, 2020 23:03

Merge branch 'master' into fix/remove-es-client

975cb86

Merge branch 'master' of github.com:elastic/kibana into pr/75943

24172f5

Merge branch 'master' into fix/remove-es-client

bcb52b9

lukasolson approved these changes Sep 3, 2020

View reviewed changes

lukeelmers merged commit d494f1e into elastic:master Sep 3, 2020

lukeelmers deleted the fix/remove-es-client branch September 3, 2020 17:24

lukeelmers mentioned this pull request Sep 3, 2020

[7.x] [data.search.SearchSource] Remove legacy ES client APIs. (#75943) #76676

Merged

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Sep 3, 2020

[data.search.SearchSource] Remove legacy ES client APIs. (elastic#75943)

bddf706

lukeelmers mentioned this pull request Sep 3, 2020

[Search] Remove legacy ES client #55139

Closed

lukeelmers added a commit that referenced this pull request Sep 3, 2020

[data.search.SearchSource] Remove legacy ES client APIs. (#75943) (#7…

e934fb8

…6676)

pgayvallet mentioned this pull request Sep 4, 2020

Replace usage of /elasticsearch/_msearch endpoint in Index Pattern Management UI #73993

Closed

lukeelmers mentioned this pull request Sep 15, 2020

[index_pattern_management]: Replace calls to /elasticsearch/_msearch with internal route. #77564

Merged

Conversation

lukeelmers commented Aug 26, 2020

Summary

Testing

Checklist

For maintainers

Dev Docs

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lizozom Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

lizozom Aug 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lizozom Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

lukasolson Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

lizozom Aug 27, 2020

Choose a reason for hiding this comment

Uh oh!

lukasolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lukasolson Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukeelmers commented Aug 26, 2020

Uh oh!

lukeelmers commented Aug 27, 2020

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

lizozom commented Aug 31, 2020

Uh oh!

lukasolson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukeelmers commented Sep 1, 2020

Uh oh!

lukeelmers commented Sep 1, 2020

Uh oh!

Uh oh!

lukeelmers commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukeelmers commented Sep 3, 2020

Uh oh!

lizozom commented Sep 3, 2020

Uh oh!

kibanamachine commented Sep 3, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

oss distributable file count

distributable file count

History

Uh oh!

lukasolson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

lukasolson left a comment •

edited

Loading

lukeelmers commented Sep 3, 2020 •

edited

Loading