-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add new elasticsearch client #69905
Add new elasticsearch client #69905
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elastic/kibana-platform This is a POC for the introduction of the new ES client, just to be sure we are all on the same page regarding the implementation.
bulk< | ||
TResponse = Record<string, any>, | ||
TRequestBody extends RequestNDBody = Array<Record<string, any>>, | ||
TContext = unknown | ||
>( | ||
params?: RequestParams.Bulk<TRequestBody>, | ||
options?: TransportRequestOptions | ||
): TransportRequestPromise<ApiResponse<TResponse, TContext>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the successor of APICaller
(only added 3 methods here for the POC. I gonna have fun later copying/adapting the 2000 lines of signatures from node_modules/@elastic/elasticsearch/index.d.ts
)
Most important question is: Do we want to expose the options?: TransportRequestOptions
to our consumers, should we only expose a subset of the possible transport options, or should our facade simply not expose this second parameter as all.
As a reminder, options
is only to override transport related options:
export interface TransportRequestOptions {
ignore?: number[];
requestTimeout?: number | string;
maxRetries?: number;
asStream?: boolean;
headers?: Record<string, any>;
querystring?: Record<string, any>;
compression?: 'gzip';
id?: any;
context?: any;
warnings?: string[];
opaqueId?: string;
}
I don't think I have enough knowledge of our usages of the ES client to decide on this one. @rudolf maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only added 3 methods here for the POC. I gonna have fun later copying/adapting the 2000 lines of signatures from
IIRC ll the types are auto-generated. It's error-prone to update them manually every time we bump the library version. Can we just re-use the same typings?
Do we want to expose the options?: TransportRequestOptions
I don't see why we shouldn't. We already provide maxRetries
& requestTimeout
in legacy client. asStream
is not possible to implement without low level support at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC ll the types are auto-generated. It's error-prone to update them manually every time we bump the library version. Can we just re-use the same typings?
The generated types are a mess (take a look at https://github.com/elastic/elasticsearch-js/blob/master/index.d.ts)
I would love to avoid replicating what we did with APICaller
by having an exhaustive list on our side, unfortunately (at least imho) we can't (please, prove me wrong here). The strongest argument would be that they define multiple signature for every methods, and we only want one (and AFAIK you can't Pick
a single signature of a multi-sign method with TS). Else the ClientWrapper
implementation is going to be a nightmare. If we go in that direction, we should probably just expose a (concrete) preconfigured client instead (but there are some things we definitely don't want to open to consumers I think, such as close
, transport
and things like that).
I.E These are the signatures for asyncSearch.delete
. We only want the first one here
delete<TResponse = Record<string, any>, TContext = unknown>(params?: RequestParams.AsyncSearchDelete, options?: TransportRequestOptions): TransportRequestPromise<ApiResponse<TResponse, TContext>>
delete<TResponse = Record<string, any>, TContext = unknown>(callback: callbackFn<TResponse, TContext>): TransportRequestCallback
delete<TResponse = Record<string, any>, TContext = unknown>(params: RequestParams.AsyncSearchDelete, callback: callbackFn<TResponse, TContext>): TransportRequestCallback
delete<TResponse = Record<string, any>, TContext = unknown>(params: RequestParams.AsyncSearchDelete, options: TransportRequestOptions, callback: callbackFn<TResponse, TContext>): TransportRequestCallback
Second point, if we use the client's signatures instead of replicating them, we would never be able to introduce higher level options that are consumed by our wrapper (as that was done with CallAPIOptions
in the legacy client). I don't have any example of why we could want that, but using the lib's types directly would just close this door.
Other (minor) point, all the apis are available both in camel and snake case. It would be great to avoid such pollution, and that would also avoid having to grep for two things when searching for usages (this one could be resolved with a Pick
based type)
delete_autoscaling_policy<TResponse = Record<string, any>, TContext = unknown>(params?: RequestParams.AutoscalingDeleteAutoscalingPolicy, options?: TransportRequestOptions): TransportRequestPromise<ApiResponse<TResponse, TContext>>
delete_autoscaling_policy<TResponse = Record<string, any>, TContext = unknown>(callback: callbackFn<TResponse, TContext>): TransportRequestCallback
delete_autoscaling_policy<TResponse = Record<string, any>, TContext = unknown>(params: RequestParams.AutoscalingDeleteAutoscalingPolicy, callback: callbackFn<TResponse, TContext>): TransportRequestCallback
delete_autoscaling_policy<TResponse = Record<string, any>, TContext = unknown>(params: RequestParams.AutoscalingDeleteAutoscalingPolicy, options: TransportRequestOptions, callback: callbackFn<TResponse, TContext>): TransportRequestCallback
deleteAutoscalingPolicy<TResponse = Record<string, any>, TContext = unknown>(params?: RequestParams.AutoscalingDeleteAutoscalingPolicy, options?: TransportRequestOptions): TransportRequestPromise<ApiResponse<TResponse, TContext>>
deleteAutoscalingPolicy<TResponse = Record<string, any>, TContext = unknown>(callback: callbackFn<TResponse, TContext>): TransportRequestCallback
deleteAutoscalingPolicy<TResponse = Record<string, any>, TContext = unknown>(params: RequestParams.AutoscalingDeleteAutoscalingPolicy, callback: callbackFn<TResponse, TContext>): TransportRequestCallback
deleteAutoscalingPolicy<TResponse = Record<string, any>, TContext = unknown>(params: RequestParams.AutoscalingDeleteAutoscalingPolicy, options: TransportRequestOptions, callback: callbackFn<TResponse, TContext>): TransportRequestCallback
Third point, in my opinion again, in term of Dev experience, A Pick
based type is way worse than a 'plain' explicit interface when searching for s specific thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The strongest argument would be that they define multiple signature for every methods, and we only want one (and AFAIK you can't Pick a single signature of a multi-sign method with TS).
Other (minor) point, all the apis are available both in camel and snake case. It would be great to avoid such pollution, and that would also avoid having to grep for two things when searching for usages (this one could be resolved with a Pick based type
That's true, the client supports all possible use-cases which we don't want to. I'm still skeptical about manual work required on every update... That's not ideal, but we can adjust type generator script in elasticsearch-js
to run it for our use-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not ideal, but we can adjust type generator script in elasticsearch-js to run it for our use-case.
Automated generation could definitely be an option if we are afraid of manual maintenance when we bump the library.
So we would use (and maintain) an edited version of their script to generate our (currently named) ClientFacade
type, to only have the camelCase and promise-based version of the APIs? And we will regenerate the type using our script every time we bump the library?
I can give this a try if we want to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not ideal, but we can adjust type generator script in elasticsearch-js to run it for our use-case.
Just saw that the script
folder of @lastic/elasticsearch
is not shipped in the npm module (neither are the source), which mean we can't use the script without checkout-ing the whole module manually.
Maybe AST parsing of node_modules/@elastic/elasticsearch/index.d.ts
is a better option then? It would at least allow to generate our type directly from the kibana checkout/repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, after 3 5 hours trying both approaches of using AST (with ts
and ts-morph
) and hacking the library generation scripts, I kinda gave up.
- hacking their scripts does not seems a viable option. That requires a local
@elastic/elasticsearch-js
checkout, which also itself has to perform a checkout of@elastic/elastic
to build their generated API and documentation. Don't really see how we plug that easily into our repo - using TS AST is a pain, but the most problematic thing here is the overloaded signatures the
Client
API is defining. I did not found any way to properly extract a specific overload from the definition list. Also converting the concreteclass
definition to an interface is quite tedious, even usingts-morph
.
So, instead, I moved on using a (way less sexy but yet effective for our needs) plain regexp-based parsing of their .d.ts
file in 24ac36b. The script generates both the ClientFacade
API and its wrapper implementation.
I feel like this could do the trick, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, after 3 5 hours trying
😅 On the whole I'm okay even to have a manual process.
So, instead, I moved on using a (way less sexy but yet effective for our needs) plain regexp-based parsing of their .d.ts file
Ok, as long as it works. I didn't review the whole file. I thought that we could extend the script right in elasticsearch-js
repo to generate a separate file for Kibana.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to change the script in the @elastic/elasticsearch
repo to generate a separate type that only includes the Promise-based, camelCase API? Seems like it would be useful for other consumers of this npm package than just us. @delvedor wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshdover sorry, you were not included in the slack discussion between delvedor, restry and myself. A brief summary:
Delvedor did that for us (--kibana
flag - elastic/elasticsearch-js#1239). However:
- to avoid polluting the distributable, the script must be manually launched (the kibana version is not in the distributable), meaning that we need to have a local checkout of the library. If not blocker when developing locally, it could be for [discuss] new elasticsearch client version management #70431 depending on the chosen solution.
- it's still a type, not an interface. meaning that if we want to use methods such as
close
ONLY from within core, we still need to have an interface/facade OR expose a proxy of the Client to block access to the 'private' fields/methods - We still need to generate the mocked version of the client for our mocks. A
https://github.com/elastic/elasticsearch-js-mock
lib does exists, but it's more an integration test mock (allow to mock responses for specific endpoints) than a jest-based mock. The divergence from our other testing mocks made me go the generation way (prefer one way to do thing)
Overall, imho, these generation scripts 'just works (tm)' for our need, at least for now. As it's just an implementation detail (it shouldnt impact core' public API), I'd say we could probably go with it on the initial implementation, and eventually change the approach later.
asyncSearch: { | ||
delete<TResponse = Record<string, any>, TContext = unknown>( | ||
params?: RequestParams.AsyncSearchDelete, | ||
options?: TransportRequestOptions | ||
): TransportRequestPromise<ApiResponse<TResponse, TContext>>; | ||
get<TResponse = Record<string, any>, TContext = unknown>( | ||
params?: RequestParams.AsyncSearchGet, | ||
options?: TransportRequestOptions | ||
): TransportRequestPromise<ApiResponse<TResponse, TContext>>; | ||
submit< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another 'detail' regarding this typed/structured replacement of APICaller
is that it may be a little difficult to migrate the retryCall
methods used by the SO client
kibana/src/core/server/elasticsearch/retry_call_cluster.ts
Lines 89 to 107 in bf04235
export function retryCallCluster(apiCaller: APICaller) { | |
return (endpoint: string, clientParams: Record<string, any> = {}, options?: CallAPIOptions) => { | |
return defer(() => apiCaller(endpoint, clientParams, options)) | |
.pipe( | |
retryWhen((errors) => | |
errors.pipe( | |
concatMap((error, i) => | |
iif( | |
() => error instanceof legacyElasticsearch.errors.NoConnections, | |
timer(1000), | |
throwError(error) | |
) | |
) | |
) | |
) | |
) | |
.toPromise(); | |
}; | |
} |
Previously APICaller
was just a method, so wrapping it to retry was rather trivial. This this new typed interface, I'm unsure what would be the correct solution to achieve the same thing.
Maybe someone have an idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can change signature to accept a function:
export function retryCallCluster(fn: () => Promise<T>): Promise<T> {
return defer(fn())
.pipe(
retryWhen((errors) =>
errors.pipe(
concatMap((error, i) =>
iif(
() => error instanceof legacyElasticsearch.errors.NoConnections,
timer(1000),
throwError(error)
)
)
)
)
)
.toPromise();
};
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that's the 'easier' solution I see. However multiple endpoint/API calls are used in the SO client. wrapping the whole APICaller with that allowed to be sure every call was going to be wrapper with the retry logic. If we wrap each individual methods, we would need to adapt all calls in the SO repository that were based on this retry logic.
Not really seeing another option atm though.
package.json
Outdated
@@ -125,6 +125,7 @@ | |||
"@elastic/apm-rum": "^5.2.0", | |||
"@elastic/charts": "19.5.2", | |||
"@elastic/datemath": "5.0.3", | |||
"@elastic/elasticsearch": "^7.7.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client follows the stack versioning, meaning that using the client 7.x
in kibana master
will cause issues. You should use the client master
branch.
Currently, we are not publishing any 8.x
version on npm, but we could do it if it does help you.
Here you can find the compatibility table of the client.
If you want to install the master branch of the client:
npm install elastic/elasticsearch-js#master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the client released separately for every Stack release? Should it be another place to sync across the Stack when bumping a Kibana version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we do a release for every major.minor
of the stack, patches are released as soon as it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, this may be quite problematic AFAIK, as kibana master is targeting 8.0, but the current branch (7.9 atm for example) is targeting 7.X
That means that we would need to have different versions (with potential differences in APIs) between our kibana master branch and our next-release branch?
This feels like it could become a backport nightmare, doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@delvedor Can you elaborate on the typical changes between versions? If newer versions only change to support or remove new or deprecated ES functionality then this shouldn't cause any problems for us that aren't already caused by ES.
But if elasticsearch-js plans to make breaking changes to it's API signatures this adds an additional maintenance burden and we will have to migrate all code within the release timeframe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client follows semantic versioning, so there will never be a breaking change between minor or patch releases, but there might be between majors.
Minor releases are always additive, in a generic minor release you will find new ES endpoints and some additional features of the client, for example, in the last 2/3 minors, client helpers have been added.
If the client needs to do a breaking change, which can be dropping the support for a specific version of Node, remove/change a configuration option, or drop an API, that will happen in a major release.
The only parts of the client that could have a breaking change between minors are the helpers and the type definitions, which are still experimental (even if they are stable and not expected to change unless there is a very good reason).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client does prereleases as soon as there is a feature freeze, if you take a look at the published versions on npm you will see few rcs.
const { body: apiKeyBody } = await esClient.security.createApiKey({ | ||
const { body: apiKeyBody } = await esClient.security.createApiKey<typeof apiKey>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL: the elastic/elasticsearch-js
package was already used in dev. As I bumped the version, I had to fix a few usages.
76b6759
to
f956fb4
Compare
expect(scopedClient.child).toHaveBeenCalledWith({ | ||
headers: { | ||
foo: 'request', | ||
hello: 'dolly', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we could extract it in a separate test case: do not filter customHeaders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's covered in the includes the customHeaders from the config when creating the child client
test. Will rename for something more explicit
describe('Mocked client', () => { | ||
let client: ReturnType<typeof elasticsearchClientMock.createInternalClient>; | ||
|
||
const expectMocked = (fn: jest.MockedFunction<any> | undefined) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the first test for mock 😅
Array [ | ||
"200 | ||
GET /foo | ||
hello=dolly", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: could you add a test case that query string is properly encoded?
querystring: { city: 'Münich' },
* | ||
* @public | ||
*/ | ||
export type ElasticSearchClient = Omit< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for this capitalization? I don't believe we capitalize Elasticsearch that way elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to ElasticsearchClient
retest |
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
* add "@elastic/elasticsearch" to dependencies * first POC of new client * add logging * add generation script for client facade API and implementation * add back keepAlive * add exports from client * add new client mocks * add some doc * fix API usages * rename legacy client to legacy in service * rename currently unused config/client observable * wire new client to service & update mocks * fix mock type * export client types * add transport.request * more doc * migrate version_check to new client * fix default port logic * rename legacy client mocks * move legacy client mocks to legacy folder * start adding tests * add configure_client tests * add get_client_facade tests * bump client to 7.8 * add cluster_client tests * expose new client on internal contract only * revert using the new client for es version check * add service level test for new client * update generated API * Revert "rename legacy client mocks" This reverts commit e48f3ad * address some review comments * revert ts-expect-error from unowned files * move response mocks to mocks.ts * Remove generated facade, use ES Client directly * log queries even in case of error * nits * use direct properties instead of accessors * handle async closing of client * review nits * ElasticSearchClient -> ElasticsearchClient * add test for encoded querystring * adapt test file # Conflicts: # x-pack/test/upgrade_assistant_integration/upgrade_assistant/status.ts
* add "@elastic/elasticsearch" to dependencies * first POC of new client * add logging * add generation script for client facade API and implementation * add back keepAlive * add exports from client * add new client mocks * add some doc * fix API usages * rename legacy client to legacy in service * rename currently unused config/client observable * wire new client to service & update mocks * fix mock type * export client types * add transport.request * more doc * migrate version_check to new client * fix default port logic * rename legacy client mocks * move legacy client mocks to legacy folder * start adding tests * add configure_client tests * add get_client_facade tests * bump client to 7.8 * add cluster_client tests * expose new client on internal contract only * revert using the new client for es version check * add service level test for new client * update generated API * Revert "rename legacy client mocks" This reverts commit e48f3ad * address some review comments * revert ts-expect-error from unowned files * move response mocks to mocks.ts * Remove generated facade, use ES Client directly * log queries even in case of error * nits * use direct properties instead of accessors * handle async closing of client * review nits * ElasticSearchClient -> ElasticsearchClient * add test for encoded querystring * adapt test file # Conflicts: # x-pack/test/upgrade_assistant_integration/upgrade_assistant/status.ts
Summary
Part of #35508
Add a new ES client wrapper based on
@elastic/elasticsearch
.ClusterClient
and underlying classes.es
service internal contractNote that the new client is currently unused in any way (even within
core
), and is not exposed oncore
's public API. See #35508 for tasks order.Thanks to @delvedor for the help and support to our team on this one.
Checklist