Elasticsearch client: no longer default to using meta: true

[pgayvallet]

This is not a failed rebase

Summary

Fix #116095

• Remove usages of the KibanaClient interface in favor of the vanilla Client
• Stop forcing the meta: true option in our client's transport
• Adapt all the usages of the client in the whole codebase to handle the new signature and returns
• Adapt associated tests

What is this about?

The 8.0 version of @elastic/elasticsearch changed the behavior of the client to only return the body by default, unless the meta: true option is specified, in which case it returns a { body, statusCode, headers, warnings } struct as in did in 7.x. This was a very significant breaking change.

Which is why, when we migrated to this version of the client in #113950, we decided to force the old behavior by injecting meta: true to all ES client calls and using our own KibanaClient interface shim-ing the old signatures to reduce the impact of this library upgrade in the initial PR.

This PR is the direct follow up of #113950, and get rid of the KibanaClient interface, switch to using the behavior of a vanilla Client, and adapt the client usages across the whole codebase to properly use the changed API.

See #116095 for more context

New ElasticsearchClient mock

To ease the testing of the ES API, especially the behavior where the return of the client's API differs depending on the meta option, this PR adapts core's ES client mock to add additional mocking functions to ElasticsearchClientMock:

• mockResponse
• mockResponseOnce
• mockResponseImplementation
• mockResponseImplementationOnce

These methods handle the meta option internally, to either only return the body or the whole response depending on the provided meta option's value.

To codeowners

All usages of the client were, in theory, adapted in this PR.

Note that if some features are not properly covered by FTR tests, some usages may have been missed, as some practices can shallow typescript errors:

• Record<> based ES responses causing response.body to pass validation
• Usages of lodash get such as _.get(response, 'body.X.Y')
• @ts-ignore-error usage on the line of the API calls
• Custom interfaces on top of Client or ElasticsearchClient (e.g the APM client or ML client)
• Client calls from javascript (.js) files
• ...

If you are aware that some of your owned features are not properly test-covered, please focus on those during the review and double check that no client call was missed.

Regarding the test changes, I tried as much as possible to use the new client mock functions to make sure that the returns were correctly of the body or response shapes, but in some cases, such as manual mocks not using core's ES client mock, I couldn't, and had to just change the return values of the mock to return the body instead of the whole response. Improving the tests to properly use core's ES client mock is considered out of scope of this PR and should eventually be performed by the individual code owners as follow-ups, if they think it's necessary.

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

Risk Matrix

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Features relying on direct ES calls and not properly covered by functional tests may be broken	Medium	High	Typescript validation detects most of the signature mismatches, but some specific scenarios can lead to undetected 'failures' by the validation. This PR aims to be merged early in the 8.2 dev cycle to allow detecting such potential problems during the development phase of 8.2

[jgowdyelastic]

ML, Transforms and File Upload changes LGTM

[mshustov]

We need tests for the mocks now 😄

[pgayvallet]

Oh, that's right 😅 . will add them

[mshustov]

Do we really need to provide it in the RepositoryEsClient? Is it possible to migrate from transport to an ES method?

[pgayvallet]

We just need it because of the client API definition (unfortunately), we're not using it.

see #124488 (comment)

Removing it causes errors such as

 proc [tsc] src/core/server/saved_objects/service/lib/repository.ts:2210:49 - error TS2769: No overload matches this call.
 proc [tsc]   Overload 1 of 3, '(this: That, params: import("/kibana/node_modules/@elastic/elasticsearch/lib/api/types").GetRequest | import("/kibana/node_modules/@elastic/elasticsearch/lib/api/typesWithBodyKey").GetRequest, options?: TransportRequestOptionsWithOutMeta | undefined): Promise<...>', gave the following error.
 proc [tsc]     The 'this' context of type 'RepositoryEsClient' is not assignable to method's 'this' of type 'That'.
 proc [tsc]   Overload 2 of 3, '(this: That, params: import("/kibana/node_modules/@elastic/elasticsearch/lib/api/types").GetRequest | import("/kibana/node_modules/@elastic/elasticsearch/lib/api/typesWithBodyKey").GetRequest, options?: TransportRequestOptionsWithMeta | undefined): Promise<...>', gave the following error.
 proc [tsc]     The 'this' context of type 'RepositoryEsClient' is not assignable to method's 'this' of type 'That'.
 proc [tsc]   Overload 3 of 3, '(this: That, params: import("/kibana/node_modules/@elastic/elasticsearch/lib/api/types").GetRequest | import("/kibana/node_modules/@elastic/elasticsearch/lib/api/typesWithBodyKey").GetRequest, options?: TransportRequestOptions | undefined): Promise<...>', gave the following error.
 proc [tsc]     The 'this' context of type 'RepositoryEsClient' is not assignable to method's 'this' of type 'That'.
 proc [tsc]
 proc [tsc] 2210     const { body, statusCode, headers } = await this.client.get<SavedObjectsRawDocSource>

[yuliacech]

Thanks a lot, @pgayvallet!
Deployment Management changes LGTM 🚀
(Logged an unrelated search profiler issue here)

[joshdover]

Fleet changes LGTM. One nit suggestion.

Nice removal of a little boilerplate, and I think it's a good breaking change 🎉 . Wonder if we should rename the asResponse option we have on our public http client in Core to meta to match?

[FrankHassanabad]

Went through the changes owned by me. 👍 , nothing bad I could find. Thanks.

[nickpeihl]

file_upload and geo_containment changes lgtm!

[tsullivan]

Reporting changes LGTM

[weltenwort]

infra and monitoring changes LGTM, thank you!

[crob611]

Tested changes to Presentation Team and everything looks and works as expected ✅

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: b1bd734
• Storybooks Preview
• Webpack Bundle Analyzer

Metrics [docs]

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
@kbn/test	208	205	-3

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
kibana	942	302	-640

Unknown metric groups

API count

id	before	after	diff
@kbn/test	244	241	-3

ESLint disabled line counts

id	before	after	diff
actions	25	22	-3
apm	86	83	-3
infra	127	126	-1
monitoring	39	31	-8
securitySolution	446	442	-4
total			-19

Total ESLint disabled count

id	before	after	diff
actions	30	27	-3
apm	101	98	-3
infra	134	133	-1
monitoring	46	38	-8
securitySolution	513	509	-4
total			-19

History

• 💚 Build #23679 succeeded b255557
• 💚 Build #23529 succeeded ed48672
• 💔 Build #23504 failed f1323ec
• 💚 Build #23232 succeeded cd3023b
• 💚 Build #22823 succeeded 6aee533
• 💚 Build #22746 succeeded 1ff4d92

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

[pgayvallet]

A planned, merged on green CI despite having some missing reviews.