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

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

Merged
merged 22 commits into from
Sep 3, 2020

Conversation

lukeelmers
Copy link
Member

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:

  1. 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.
  2. The legacy "default search strategy" is updated to call the new internal endpoint instead of using the legacy ES client.
  3. The __LEGACY APIs are removed from the client-side search service.
  4. 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.
  5. 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

For maintainers

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.

@lukeelmers 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 lukeelmers self-assigned this Aug 26, 2020
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.

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.

let resolved = false;

const loadingCount$ = new BehaviorSubject(0);
http.addLoadingCountSource(loadingCount$);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moved loadingCount$ to search service


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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can handle it separately from this PR 👍

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.

LGTM, minor comments below


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

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@lukeelmers
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

🎉

@lizozom
Copy link
Contributor

lizozom commented Aug 31, 2020

@elasticmachine merge upstream

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.

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
Copy link
Member Author

@elasticmachine merge upstream

@lukeelmers
Copy link
Member Author

@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.

@lukeelmers
Copy link
Member Author

lukeelmers commented Sep 3, 2020

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
Copy link
Member Author

@elasticmachine merge upstream

@lizozom
Copy link
Contributor

lizozom commented Sep 3, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

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

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.

LGTM!

@lukeelmers lukeelmers merged commit d494f1e into elastic:master Sep 3, 2020
@lukeelmers lukeelmers deleted the fix/remove-es-client branch September 3, 2020 17:24
lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Sep 3, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 3, 2020
* 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)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 4, 2020
…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)
  ...
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 release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants