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

[Elasticsearch] remove legacy es client #107619

Merged
merged 17 commits into from
Aug 18, 2021

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Aug 4, 2021

Summary

closes #106885
closes #108388

Remove the legacy ES client from Core API and the legacy elasticsearch client from Kibana dependencies.
It's been deprecated for a long time and we stated that it will be removed by v7.16.0

Plugin API changes

The legacy Elasticsearch client is no longer available in CoreSetup and RequestHandlerContext interfaces. Use the new client instead https://www.elastic.co/guide/en/kibana/current/elasticsearch-service.html

@mshustov mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. technical debt Improvement of the software architecture and operational architecture v8.0.0 v7.16.0 labels Aug 4, 2021
@mshustov
Copy link
Contributor Author

mshustov commented Aug 4, 2021

@mistic The integration test failures likely are caused by my changes, but it's hard to debug them due to cryptic error stack:

11:40:03   FAIL  packages/kbn-optimizer/src/integration_tests/basic_optimization.test.ts
11:40:03    ● Test suite failed to run
11:40:03  
11:40:03      /dev/shm/workspace/parallel/3/kibana/packages/kbn-optimizer/src/integration_tests/basic_optimization.test.ts: Error: No substitution given for "$0". If this is not meant to be a
11:40:03                  placeholder you may want to consider passing one of the following options to @babel/template:
11:40:03                  - { placeholderPattern: false, placeholderWhitelist: new Set(['$0'])}
11:40:03                  - { placeholderPattern: /^$0$/ }
11:40:03  
11:40:03                    placeholder you may want to consider passing one of the following options to @babel/template:
11:40:03                    - { placeholderPattern: false, placeholderWhitelist: new Set(['$0'])}
11:40:03                    - { placeholderPattern: /^$0$/ }
11:40:03            at undefined
11:40:03            at undefined
11:40:03            at undefined
11:40:03            at undefined
11:40:03            at undefined
11:40:03            at undefined
11:40:03            at undefined
11:40:03            at undefined
11:40:03            at undefined
11:40:03            at undefined

Would you mind taking a look, please?

@joshdover
Copy link
Contributor

Should we merge it after v7.15 FF?

Sounds like a good plan to me.

@mistic
Copy link
Member

mistic commented Aug 13, 2021

@elasticmachine merge upstream

Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

We can remove this now as the fix on Bazel was introduced

WORKSPACE.bazel Outdated Show resolved Hide resolved
import { TransportRequestPromise } from '@elastic/elasticsearch/lib/Transport';
import type { DeeplyMockedKeys } from '@kbn/utility-types/jest';
import { ElasticsearchClient } from './types';
import { ICustomClusterClient } from './cluster_client';
import { PRODUCT_RESPONSE_HEADER } from '../supported_server_response_check';

// use jest.requireActual() to prevent weird errors when people mock @elastic/elasticsearch
const { Client: UnmockedClient } = jest.requireActual('@elastic/elasticsearch');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix is done by @spalger. It seems to be a bug in mockify breaking runtime code, when it tries to mock an already mocked object. Related issue #108388

@@ -53,7 +52,7 @@ describe('geo_containment', () => {
it('should correctly transform expected results', async () => {
const transformedResults = transformResults(
// @ts-ignore
(sampleAggsJsonResponse.body as unknown) as SearchResponse<unknown>,
sampleAggsJsonResponse.body,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the line has // @ts-ignore directive, so the manual casting is unnecessary. and it's wrong anyway.

@mshustov mshustov marked this pull request as ready for review August 18, 2021 07:43
@mshustov mshustov requested a review from a team as a code owner August 18, 2021 07:43
@mshustov mshustov requested review from a team as code owners August 18, 2021 07:43
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@mshustov
Copy link
Contributor Author

@elastic/kibana-app-services this file contains references to elasticsearch packages but it hasn't been updated for 10 months. I guess you need to remove it completely https://github.com/elastic/kibana/blob/6684d9531d7aad63a04faac81507f1d3b1c70820/src/plugins/data/server/plugins_data_server.api.md

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

🥇

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

🙌

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

Alerting changes LGTM!

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

geo_containment changes LGMT

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

src/plugins/data/server/server.api.mdCO review, LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id before after diff
core 2444 2260 -184

API count missing comments

id before after diff
core 1166 1006 -160

API count with any type

id before after diff
core 149 27 -122

References to deprecated APIs

id before after diff
globalSearch 4 0 -4

History

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

@mshustov mshustov merged commit b53b2cd into elastic:master Aug 18, 2021
@mshustov mshustov deleted the issue-83910-remove-legacy-es-client branch August 18, 2021 17:18
mshustov added a commit to mshustov/kibana that referenced this pull request Aug 18, 2021
* remove legacy es client

* update docs

* uninstall elasticsearch package

* fix global_search tests

* ad-hoc fix to address bazel failure. authored by Tiago

* update docs

* remove elasticsearch import. errors are muted with @ts-ignore

* Update WORKSPACE.bazel

Co-authored-by: Tiago Costa <tiagoffcc@hotmail.com>

* update docs

* fix problem when dev mock already mocked client

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Tiago Costa <tiagoffcc@hotmail.com>
# Conflicts:
#	src/core/server/elasticsearch/legacy/scoped_cluster_client.test.ts
mshustov added a commit that referenced this pull request Aug 18, 2021
* remove legacy es client

* update docs

* uninstall elasticsearch package

* fix global_search tests

* ad-hoc fix to address bazel failure. authored by Tiago

* update docs

* remove elasticsearch import. errors are muted with @ts-ignore

* Update WORKSPACE.bazel

Co-authored-by: Tiago Costa <tiagoffcc@hotmail.com>

* update docs

* fix problem when dev mock already mocked client

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Tiago Costa <tiagoffcc@hotmail.com>
# Conflicts:
#	src/core/server/elasticsearch/legacy/scoped_cluster_client.test.ts
@stacey-gammon
Copy link
Contributor

Screen Shot 2021-08-24 at 10 25 45 AM

-122 any types! 🎉 🎉 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc technical debt Improvement of the software architecture and operational architecture v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jest Integration test failures produce a cryptic error stack trace Remove the legacy ES client from Core API