[KP] use new ES client in SO service #72289

mshustov · 2020-07-17T15:02:02Z

Summary

Refactors SO service to use https://github.com/elastic/elasticsearch-js under the hood.
It should not affect public API since we return only the response body from SO API methods.
However, this change might affect plugins reading response status field from SO error bubbled to the Solutions code since ES error doesn't provide status field anymore (statusCode is still provided).
Adjusts several plugins to check SO errors with SavedObjectsErrorHelpers. Plugins must use it because we are going to stop wrapping errors in the Boom object Do not wrap SO errors in Boom #72441

Comments on client integration problems @delvedor:

lack of response types. I had to added DeleteDocumentResponse, GetResponse and SearchResponse manually from the legacy client interface.
update, bulk methods interface incompatible. before: refresh: boolean | 'wait_for'. after: refresh: 'true' | 'false' | 'wait_for'

kibana/src/core/server/saved_objects/service/lib/repository.ts

Lines 910 to 911 in e9ae3e9

// @ts-expect-error refresh?: 'true' | 'false' | 'wait_for' in Client

refresh,

It's possible to cast to String type, but it's rather clumsy in TS.

function toRefresh(refresh?: MutatingOperationRefreshSetting) {
  if (refresh === undefined) return;
  if (refresh === true) return 'true';
  if (refresh === false) return 'false';
  return refresh;
}

updateByQuery. accepts refresh?: boolean; is it expected? shouldn't it have a compatible interface with refresh: 'true' | 'false' | 'wait_for'?
there are no _seq_no, _primary_term fields in ES document returned from update, get methods https://www.elastic.co/guide/en/elasticsearch/reference/master/optimistic-concurrency-control.html
Several times I had runtime exceptions because all request params extends Generic type containing ignore?: number | number[];, however, it had been failing argument validation until I moved ignore: [404] to the transport options.

// no TS error. runtime exception
await client.mget({body: ..., ignore: [404] });
// no TS error. no runtime exception
await client.mget({body: ... }, { ignore: [404] });

TransportRequestPromise usage makes wrapping in an async function incompatible

async function logError(fn: () => Promise<T>): Promise<T> {
  try{
    return await fn();
  } catch(e){
    console.log(e);
    throw e;
  }
}

all camelCase request params renamed to snake_case. It's not mentioned in https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/breaking-changes.html

Checklist

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

… a string

delvedor · 2020-07-20T09:39:50Z

lack of response types. I had to added DeleteDocumentResponse, GetResponse and SearchResponse manually from the legacy client interface.

We are aware of this, unfortunately, the rest-api-spec is not complete in that regard. We are working for improving the situation, I'll keep you posted.

update, bulk methods interface incompatible. before: refresh: boolean | 'wait_for'. after: refresh: 'true' | 'false' | 'wait_for'

We are using the rest-api-spec for generating the types, and as you can see they are listed as strings. I agree that it should be boolean | 'wait_for, as the serialization is an implementation detail. I'll update the code generation to address this.

updateByQuery. accepts refresh?: boolean; is it expected? shouldn't it have a compatible interface with refresh: 'true' | 'false' | 'wait_for'?

This is how it's defined in the rest-api-spec, I'll ping the ES team for understanding if it's correct or is missing an option.

there are no _seq_no, _primary_term fields in ES document returned from update, get methods https://www.elastic.co/guide/en/elasticsearch/reference/master/optimistic-concurrency-control.html

The client is not doing anything with the Elasticsearch response, so if a field is missing, it means that ES is not sending it out.

Several times I had runtime exceptions because all request params extends Generic type containing ignore?: number | number[];, however, it had been failing argument validation until I moved ignore: [404] to the transport options.

This is a bug in the types, I'll fix it :)

TransportRequestPromise usage makes wrapping in an async function incompatible

This is "correct", as TransportRequestPromise contains an abort method as well.

import { ApiResponse } from './'
import { TransportRequestPromise } from './lib/Transport'

async function logError (fn: () => TransportRequestPromise<ApiResponse>): Promise<ApiResponse> {
  try{
    return await fn()
  } catch(e){
    console.log(e)
    throw e
  }
}

all camelCase request params renamed to snake_case. It's not mentioned in https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/breaking-changes.html

This is a fun one :P
The JavaScript code allows both (and I regret this decision), while the TypeScript one allows snake:case only. The reason is because if TypeScript would allows both styles, it would not be possible defining which parameters are required and which not. I've decided to force people to use snake_case for two reasons, the first is because inside the body you are forced to use snake_case anyhow, the second is because transforming every parameter from camelCase to snake_case at runtime is expensive, and it's better to just use snake_case.

elasticmachine · 2020-07-21T12:35:23Z

Pinging @elastic/kibana-platform (Team:Platform)

elasticmachine · 2020-07-21T12:35:25Z

Pinging @elastic/apm-ui (Team:apm)

elasticmachine · 2020-07-21T12:35:25Z

Pinging @elastic/ingest-management (Team:Ingest Management)

sorenlouv

APM changes lgtm

pgayvallet

Some nits and comments, overall LGTM

src/core/server/elasticsearch/client/mocks.ts

src/core/server/elasticsearch/client/configure_client.ts

pgayvallet · 2020-07-21T19:36:55Z

src/core/server/index.ts

@@ -109,6 +109,7 @@ export {
  LegacyAPICaller,
  FakeRequest,
  ScopeableRequest,
+  ElasticsearchClient,


NIT: Why exposing it now? Should we wait until we officially exposes the API to plugins?

I used it in the integration tests.

kibana/test/api_integration/apis/saved_objects/migrations.ts

Line 55 in b0b9137

const esClient = getService('es');

I don't want to expose raw Client there. Also, exposing a type doesn't hurt us a lot.

src/core/server/saved_objects/service/lib/repository_es_client.ts

src/core/server/saved_objects/migrations/core/elastic_index.test.ts

src/core/server/saved_objects/migrations/core/index_migrator.test.ts

src/core/server/saved_objects/service/lib/repository.ts

pgayvallet · 2020-07-22T08:07:02Z

src/core/server/saved_objects/service/lib/repository.ts

-    const deleteDocNotFound = deleteResponse.result === 'not_found';
-    const deleteIndexNotFound =
-      deleteResponse.error && deleteResponse.error.type === 'index_not_found_exception';
+    const deleteDocNotFound = body.result === 'not_found';


It was already like that, but wouldn't check on statusCode be better?

I didn't want to take a risk to change this code without talking to an author. From the source code, it seems that there could be different reasons for Not found response https://github.com/elastic/elasticsearch/blob/b7062391452dbe82c552fa2bb2d5a31f64667b1d/client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java

src/core/server/saved_objects/service/lib/repository.test.js

cjcenizal

Upgrade Assistant changes LGTM, didn't test locally.

jen-huang

Ingest Manager changes LGTM

mshustov · 2020-07-22T20:05:48Z

x-pack/plugins/lists/server/services/exception_lists/create_endpoint_list.ts

@@ -56,7 +56,7 @@ export const createEndpointList = async ({
    );
    return transformSavedObjectToExceptionList({ savedObject });
  } catch (err) {
-    if (err.status === 409) {


TODO: to make sure it's listed in the migration guide

kibanamachine · 2020-07-24T10:25:12Z

💚 Build Succeeded

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

Build metrics

✅ unchanged

History

💔 Build #64151 failed efe1c5f
💚 Build #63243 succeeded 3b7990a
💚 Build #63222 succeeded 70af672
💚 Build #63208 succeeded f94d916
💔 Build #63189 failed 5535b0f

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

* adapt retryCallCluster for new ES client * review comments * retry on 408 ResponseError * remove legacy retry functions * use Migrator Es client in SO migration * update migration tests * improve ES typings and mocks * migrate decorate ES errors * add repository es client * use new es client in so repository * update repository tests * fix migrator integration tests * declare _seq_no & _primary_term on get response. _source expect to be a string * make _sourceIncludes and refresh compatible with the client * add test for repository_es_client * move ApiResponse to es client mocks * TEMP: handle wait_for as true for deleteByNamespace * add tests for migration_es_client * TEMP: skip test for deleteByNamespace refresh * pass ignore as transport option in mget * log both es client and response errors * fix update method test failures * update deleteByNamespace refresh settings es doesn't support 'refresh: wait_for' for `updateByQuery` endpoint * update repository tests. we do not allow customising wait_for * do not delegate retry logic to es client * fix type errors after master merged * fix repository tests * fix security solutions code SO doesn't throw Error with status code anymore. Always use SO error helpers * switch error conditions to use SO error helpers * cleanup * address comments about mocks * use isResponseError helper * address comments * fix type errors Co-authored-by: pgayvallet <[email protected]>

* adapt retryCallCluster for new ES client * review comments * retry on 408 ResponseError * remove legacy retry functions * use Migrator Es client in SO migration * update migration tests * improve ES typings and mocks * migrate decorate ES errors * add repository es client * use new es client in so repository * update repository tests * fix migrator integration tests * declare _seq_no & _primary_term on get response. _source expect to be a string * make _sourceIncludes and refresh compatible with the client * add test for repository_es_client * move ApiResponse to es client mocks * TEMP: handle wait_for as true for deleteByNamespace * add tests for migration_es_client * TEMP: skip test for deleteByNamespace refresh * pass ignore as transport option in mget * log both es client and response errors * fix update method test failures * update deleteByNamespace refresh settings es doesn't support 'refresh: wait_for' for `updateByQuery` endpoint * update repository tests. we do not allow customising wait_for * do not delegate retry logic to es client * fix type errors after master merged * fix repository tests * fix security solutions code SO doesn't throw Error with status code anymore. Always use SO error helpers * switch error conditions to use SO error helpers * cleanup * address comments about mocks * use isResponseError helper * address comments * fix type errors Co-authored-by: pgayvallet <[email protected]> Co-authored-by: pgayvallet <[email protected]>

* master: (111 commits) Remove flaky note from gauge tests (elastic#73240) Convert functional vega tests to ts and unskip tests (elastic#72238) [Graph] Unskip graph tests (elastic#72291) Add default Elasticsearch credentials to docs (elastic#72617) [APM] Read body from indicesStats in upload-telemetry-data (elastic#72732) The directory in the command was missing the /generated directory and would cause all definitions to be regenerated in the wrong place. (elastic#72766) [KP] use new ES client in SO service (elastic#72289) [Security Solution][Exceptions] Prevents value list entries from co-existing with non value list entries (elastic#72995) Return EUI CSS to Shareable Runtime (elastic#72990) Removed useless karma test (elastic#73190) [INGEST_MANAGER] Make package config name blank for endpoint on Package Config create (elastic#73082) [Ingest Manager] Support DEGRADED state in fleet agent event (elastic#73104) [Security Solution][Detections] Change detections breadcrumb title (elastic#73059) [ML] Fixing unnecessary deleting job polling (elastic#73087) [ML] Fixing recognizer wizard create job button (elastic#73025) [Composable template] Preview composite template (elastic#72598) [Uptime] Use manual intervals for ping histogram (elastic#72928) [Security Solution][Endpoint] Task/policy save modal text change, remove duplicate policy details text (elastic#73130) [Maps] fix tile layer attibution text and attribution link validation errors (elastic#73160) skip ingest pipeline api tests ...

pgayvallet and others added 16 commits July 13, 2020 11:49

adapt retryCallCluster for new ES client

70ea001

review comments

fbc2d7b

retry on 408 ResponseError

68c1bd3

Merge branch 'master' into pr/71412

5d8bbba

Merge branch 'master' into pr/71412

91e7b07

Merge branch 'master' into pr/71412

c15ab31

Merge branch 'master' into pr/71412

4f8d844

Merge branch 'master' into pr/71412

6245661

remove legacy retry functions

aabc76a

use Migrator Es client in SO migration

ede22ea

update migration tests

b5adb68

improve ES typings and mocks

951cd1f

migrate decorate ES errors

a8aeb4f

add repository es client

7392dc8

use new es client in so repository

e9ae3e9

update repository tests

a076507

mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Jul 17, 2020

mshustov added 4 commits July 20, 2020 08:58

fix migrator integration tests

b0b9137

Merge branch 'master' into pr/71412

a9b0159

declare _seq_no & _primary_term on get response. _source expect to be…

ed83d51

… a string

make _sourceIncludes and refresh compatible with the client

e036810

mshustov added 5 commits July 20, 2020 11:42

add test for repository_es_client

cf5fbfb

move ApiResponse to es client mocks

1753d56

TEMP: handle wait_for as true for deleteByNamespace

c6cc1b5

add tests for migration_es_client

0f29058

Merge branch 'master' into pr/71412

238edc7

mshustov requested a review from a team July 21, 2020 12:35

mshustov requested review from a team as code owners July 21, 2020 12:35

botelastic bot added Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team labels Jul 21, 2020

epixa mentioned this pull request Jul 21, 2020

Use new elasticsearch-js client in Elasticsearch service #35508

Closed

14 tasks

sorenlouv approved these changes Jul 21, 2020

View reviewed changes

pgayvallet approved these changes Jul 22, 2020

View reviewed changes

cjcenizal approved these changes Jul 22, 2020

View reviewed changes

jen-huang approved these changes Jul 22, 2020

View reviewed changes

mshustov commented Jul 22, 2020

View reviewed changes

jonathan-buttner approved these changes Jul 22, 2020

View reviewed changes

mshustov added 5 commits July 24, 2020 08:07

Merge branch 'master' into pr/71412

c2865fb

address comments about mocks

25fd77d

use isResponseError helper

e5e4fa0

address comments

efe1c5f

fix type errors

edd2d35

mshustov merged commit 2a82ff9 into elastic:master Jul 25, 2020

mshustov deleted the pr/71412 branch July 25, 2020 10:00

mshustov mentioned this pull request Jul 25, 2020

[7.x] [KP] use new ES client in SO service (#72289) #73219

Merged

mshustov added the backported label Jul 25, 2020

mshustov mentioned this pull request Jul 28, 2020

[KP] fix doc generation for platform code #73407

Merged

jportner mentioned this pull request Nov 15, 2021

Documents saved with es-archiver are missing source fields #118639

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KP] use new ES client in SO service #72289

[KP] use new ES client in SO service #72289

mshustov commented Jul 17, 2020 •

edited

Loading

delvedor commented Jul 20, 2020

elasticmachine commented Jul 21, 2020

elasticmachine commented Jul 21, 2020

elasticmachine commented Jul 21, 2020

sorenlouv left a comment

pgayvallet left a comment

pgayvallet Jul 21, 2020

mshustov Jul 24, 2020

pgayvallet Jul 22, 2020

mshustov Jul 25, 2020

cjcenizal left a comment

jen-huang left a comment

mshustov Jul 22, 2020

kibanamachine commented Jul 24, 2020

	// @ts-expect-error refresh?: 'true' \| 'false' \| 'wait_for' in Client
	refresh,

[KP] use new ES client in SO service #72289

[KP] use new ES client in SO service #72289

Conversation

mshustov commented Jul 17, 2020 • edited Loading

Summary

Checklist

For maintainers

delvedor commented Jul 20, 2020

elasticmachine commented Jul 21, 2020

elasticmachine commented Jul 21, 2020

elasticmachine commented Jul 21, 2020

sorenlouv left a comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

pgayvallet Jul 21, 2020

Choose a reason for hiding this comment

mshustov Jul 24, 2020

Choose a reason for hiding this comment

pgayvallet Jul 22, 2020

Choose a reason for hiding this comment

mshustov Jul 25, 2020

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

jen-huang left a comment

Choose a reason for hiding this comment

mshustov Jul 22, 2020

Choose a reason for hiding this comment

kibanamachine commented Jul 24, 2020

💚 Build Succeeded

Build metrics

History

mshustov commented Jul 17, 2020 •

edited

Loading