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

[ES] Upgrade client to v8.0 #113950

Merged
merged 210 commits into from
Oct 26, 2021
Merged

[ES] Upgrade client to v8.0 #113950

merged 210 commits into from
Oct 26, 2021

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Oct 5, 2021

Summary

Migrates Kibana to next major version of the @elastic/elasticsearch client
closes #108387

The full list of changes is here
Some most noticeable changes:

The returned value of API calls is the body and not the HTTP related keys.

It's the biggest source of changes in the current PR. From v8.0, the Client will return body by default. If you need statusCode and header, you need to provide meta: true flag to TransportRequestOptions

// from
const response = await client.search({
  index: 'test',
  body: {
    query: {
      match_all: {}
    }
  }
})
console.log(response) // { body: SearchResponse, statusCode: number, headers: object, warnings: array }

// to
const response = await client.search({
  index: 'test',
  query: {
    match_all: {}
  }
})
console.log(response) // SearchResponse

// with a bit of TypeScript and JavaScript magic...
const response = await client.search({
  index: 'test',
  query: {
    match_all: {}
  }
}, {
  meta: true
})
console.log(response) // { body: SearchResponse, statusCode: number, headers: object, warnings: array }

Since this change affected literally every method call in the entire repository, we decided that the service provided from Core would default to this meta: true flag of of the box.
This creates a difference in the import type { Client } from '@elastic/elasticsearch' and import type { ElasticsearchClient } from 'src.core.server client interfaces, so I've created #116095 to resolve these differences.

Your plugin code doesn't need to set this flag manually.
The code in your functional tests can set the flag. it depends on whether you need meta-information about the request results.

Types for the mode with meta: true should be imported from @elastic/elasticsearch/lib/api/typesWithBodyKey.

Drop support for old camelCased keys

Some interfaces have been removed in #115528, some in the current PR.

Remove the current abort API and use the new Abort API

Code in data and APM plugins is refactored.

body key in request is derecated.

Codeowner should inline body keys instead. We will define an explicit deadline to remove the body key from requests.

Some request keys have been renamed

Use type definitions as the reference.

--- putLifecycle({ policy })
+++ putLifecycle({ name })

--- putSettings({ settings: {...}})
+++ putSettings({...})

Product check performed on every response

Notes for teams

  • @elastic/apm-ui I didn't manage to fix createEsClient inferred type, I kindly ask you to fix it in a follow-up
  • @elastic/logs-metrics-ui I had to add @ts-ignore since type check fails locally and on CI unpredictably

Skipped tests

  • x-pack/test/search_sessions_integration/tests/apps/discover/async_search.ts
    // SKIPPED - see https://github.com/elastic/kibana/pull/113950
    // `expect(await toasts.getToastCount()).to.be(0)` fails because of a `Your search session is still running` toast
    // rest of the test does pass
    it.skip('relative timerange works', async () => {

Performance

DemoJourney test shows "global" response time reduced by 30%
The performance will be improved when we switch to the new http client (undici). The HTTP client migration will be done in a separate task #116087

Known problems with Kibana

List of known client problems
  • create API is broken. body is rewritten by any parameter https://github.com/elastic/elasticsearch-js/blob/main/src/api/api/create.ts#L56-L59 probably another API is broken too
  • type check fails
    • 'node-abort-controller' doesn't contain typings
    • undici uses buffer API introduced in nodejs v15.
  • Not exported from root level: estypes, ApiKeyAuth, BasicAuth, NodeOptions, RequestBody, and others. The long export path is problematic as well
  • doesn't export @elastic/transport typings, I had to add @elastic/transport to the Kibana deps
  • Client is not compatible with KibanaClient ('dataFrameTransformDeprecated' is missing in type 'Client')
  • statusCode, body and headers are optional in DiagnosticResult
  • Client uses Symbol to declare API. It breaks mocks in Kibana
  • requires AbortController. Use signal: AbortSignal instead. See examples https://www.npmjs.com/package/node-abort-controller https://www.npmjs.com/package/node-fetch#request-cancellation-with-abortsignal
  • Broken types due to { [key:string]: never }: MappingFieldMapping, IndicesUpdateAliasesIndicesUpdateAliasBulk, IlmAction, Transform, AggregationsBucketsPath, SpecUtilsAdditionalProperties, SpecUtilsAdditionalProperty, SpecUtilsOverloadOf, SearchAggregationProfileDebug, AggregationsPercentageScoreHeuristic, QueryDslRankFeatureFunction, QueryDslRankFeatureFunctionLinear, WatcherAlwaysCondition, WatcherNeverCondition
  • SecurityClusterPrivilege doesn’t list all the possible privileges. Please use string instead.
  • Context should falls back to unknown by deafult. Otherwise we have to explicitly declare type My = TransportResult<ResponseType, unknown> instead of type My = TransportResult<ResponseType>. It affects DiagnosticResult as well.
  • SearchHitsMetadata.total should it be SearchTotalHits? AFAIK number is deprecated
  • don’t use enum for events. It's better to use a method overload instead of importing enum. client.diagnostic.on(events.RESPONS --> client.diagnostic.on('response')
  • error stack doesn't show the precise place where a method was called
17:24:10     │ proc [kibana] TypeError: Cannot use 'in' operator to search for 'readable' in true
17:24:10     │ proc [kibana]     at ensureString (/dev/shm/workspace/parallel/4/kibana/src/core/server/elasticsearch/client/configure_client.ts:80:7)
17:24:10     │ proc [kibana]     at getRequestDebugMeta (/dev/shm/workspace/parallel/4/kibana/src/core/server/elasticsearch/client/configure_client.ts:126:28)
17:24:10     │ proc [kibana]     at getResponseMessage (/dev/shm/workspace/parallel/4/kibana/src/core/server/elasticsearch/client/configure_client.ts:106:21)
17:24:10     │ proc [kibana]     at Diagnostic.<anonymous> (/dev/shm/workspace/parallel/4/kibana/src/core/server/elasticsearch/client/configure_client.ts:143:27)
17:24:10     │ proc [kibana]     at Diagnostic.emit (events.js:400:28)
17:24:10     │ proc [kibana]     at KibanaTransport.request (/dev/shm/workspace/parallel/4/kibana/node_modules/@elastic/transport/src/Transport.ts:538:31)
17:24:10     │ proc [kibana]     at processTicksAndRejections (internal/process/task_queues.js:95:5)
17:24:10     │ proc [kibana]     at Client.CreateApi [as create] (/dev/shm/workspace/parallel/4/kibana/node_modules/@elastic/elasticsearch/src/api/api/create.ts:64:10
  • undici agent config doesn't support keepAlive and maxSockets options
  • RequestBody and others aren't exported from @elastic/transport/lib/types
  • BulkResponseItemBase.error: ErrorCause | string;, MgetHit.error: ErrorCause | string;, ShardFailure.reason: ErrorCause | string; and lots of others break a lot of code since it doesn't expect string
  • Transport, HttpConnection and other classes aren't re-exported. I had to export from @elastic/transport directly
  • ilm.putLifecycle({ policy, body }) fails with ES error ResponseError: x_content_parse_exception: [x_content_parse_exception] Reason: [1:11] [put_lifecycle_request] policy doesn't support values of type: VALUE_STRING but ilm.putLifecycle({ body }) fails with ES client error TypeError: Cannot read property 'toString' of undefined at Ilm.putLifecycle (/@elastic/elasticsearch/src/api/api/ilm.ts:245:67)
  • case sensitive file names break bundle builds (Client.js, Helpers.js, etc.)
  • updateTransform (and probably others) ignore body https://github.com/elastic/elasticsearch-js/blob/main/src/api/api/transform.ts#L228
  • scroll({body: { scroll_id:.. } }) fails intil move body to params: scroll({ scroll_id:.. })
  • doesn't provide deprecated indices.freeze method
  • attach comptible-with=8 to accept header, but not content-type
  • ml.startDatafeed request do not recognize start
  • transport with undici client throws ConnectionError: path must be an absolute URL or start with a slash
  • sort param is always sent in the body. The fieldName:sortOrder syntax can no longer be used (e.g sort: 'create_at:desc')
  • APM instrumentation is broken: support v8.x of @elastic/elasticsearch apm-agent-nodejs#2355, and was forcefully disabled on Kibana until this is addressed:
    • packages/kbn-apm-config-loader/src/init_apm.ts
    • packages/kbn-apm-config-loader/src/init_apm.test.ts
    • x-pack/test/performance/tests/reporting_dashboard.ts
  • client never stops until the queue of incoming requests is empty
  • client mutates a request object passed as an argument to a client method

Risk Matrix

Risk Probability Severity Mitigation/Notes
Incompatible changes affect Kibana runtime behavior Low Medium CI is green, the problem might happen in an area with low test coverage
Client changes cause performance degradation Low Medium Conduct performance testing to make sure the new client doesn't have significant overhead

Plugin API changes

Elasticsearch service provides the new version of the elasticsearch-js library. You can find the full list of changes, including breaking ones, here.

@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. v8.0.0 backport:skip This commit does not require backporting labels Oct 5, 2021
Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

LGTM,

  • Looked through the security_solution code, saw most was repeating patterns for the upgrade.
  • Took two notes of an areas that looked more involved, dug a bit deeper but those areas look ok to me 👍

Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

Asset management LGTM

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

infra plugin changes LGTM. We'll do some cleanup separately. Thank you!

Copy link
Contributor

@matschaffer matschaffer left a comment

Choose a reason for hiding this comment

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

Approving for @elastic/metrics-ui and @elastic/stack-monitoring-ui

@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #6 / apis SecuritySolution Endpoints Tls Test with Packetbeat Tls Overview Test Ensure data is returned for FlowTarget.Destination

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 177 180 +3
core 1023 1022 -1
total +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.6MB 3.6MB +94.0B

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 847 894 +47
Unknown metric groups

API count

id before after diff
@kbn/test 200 203 +3
core 2305 2304 -1
total +2

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting buildkite-ci release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:APM All issues that need APM UI Team support Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ES client version upgrade to v8.0