[Transform] Replace legacy elasticsearch client by darnautov · Pull Request #84932 · elastic/kibana

darnautov · 2020-12-03T18:17:30Z

Summary

Replaces legacy elasticsearch client for server-side routes in transform plugin.

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-12-03T18:17:33Z

Pinging @elastic/ml-ui (:ml)

…_audit_messages.ts

…client

jgowdyelastic · 2020-12-07T13:52:52Z

x-pack/plugins/transform/server/routes/api/privileges.ts

-        cluster,
-      } = await ctx.transform!.dataClient.callAsCurrentUser('transport.request', {
+        body: { has_all_requested: hasAllPrivileges, cluster },
+      } = await ctx.core.elasticsearch.client.asCurrentUser.transport.request({


I think this should use hasPrivileges from the security namespace.
ctx.core.elasticsearch.client.asCurrentUser.security.hasPrivileges

Updated in 4e44a09

jgowdyelastic · 2020-12-07T13:53:07Z

x-pack/plugins/transform/server/routes/api/privileges.ts

-      const { indices } = await ctx.transform!.dataClient.callAsCurrentUser('transport.request', {
+      const {
+        body: { indices },
+      } = await ctx.core.elasticsearch.client.asCurrentUser.transport.request({


same as above comment

Updated in 4e44a09

jgowdyelastic · 2020-12-07T13:58:59Z

x-pack/plugins/transform/server/routes/api/transforms.ts

    { path: addBasePath('transforms'), validate: false },
-    license.guardApiRoute(async (ctx, req, res) => {
-      const options = {};
+    license.guardApiRoute<TransformGetTransform, undefined, undefined>(async (ctx, req, res) => {


ctx.core.elasticsearch.client.asCurrentUser.transform could be extracted as transformClient and passed to the handler function inside guardApiRoute.
This is the pattern we use in ML and saves having to write ctx.core.elasticsearch.client.asCurrentUser.transform. each time.

I tried it first, but it didn't work. ctx.core.elasticsearch.client.asCurrentUser.transform relies on the context and uses transport under the hood.

From my tests this works correctly. guardApiRoute can extract ctx.core.elasticsearch.client.asCurrentUser.transform and pass it to the handler callback, and it works correctly when being called in the routes.

but this suggested change just a refactor for simplifying the code, and is not important for the functionality.

jgowdyelastic · 2020-12-07T14:01:26Z

x-pack/plugins/transform/server/routes/api/transforms.ts

 async function startTransforms(
  transformsInfo: StartTransformsRequestSchema,
-  callAsCurrentUser: LegacyAPICaller
+  esClient: ElasticsearchClient


if you adopt the change suggested above, only transformClient could be passed in to these functions.

…client

jgowdyelastic · 2020-12-08T12:57:46Z

x-pack/plugins/transform/server/routes/api/transforms.ts

+        const {
+          body,
+        } = await ctx.core.elasticsearch.client.asCurrentUser.transform.getTransformStats({
+          transform_id: transformId,


the previous version wouldn't send the transform ID if it was undefined, this now always sends one.
is this change intentional?

yes, this route has this validation schema

kibana/x-pack/plugins/transform/common/api_schemas/common.ts

Line 35 in dd18220

export const transformIdParamSchema = schema.object({

, hence this check is redundant

also covered by API integration tests and checked manually

jgowdyelastic

LGTM

peteharverson

Tested and the only issue I found was around error handling, where some of the error details previously shown are getting lost.

x-pack/plugins/transform/server/routes/api/error_utils.ts

…client

walterra

Great update, LGTM 👍

jgowdyelastic · 2020-12-09T09:43:17Z

x-pack/plugins/transform/server/routes/api/error_utils.ts

+  if (line !== undefined && col !== undefined) {
+    message +=
+      ', ' +
+      i18n.translate('xpack.transform.models.transformService.withColAndLineMessage', {


we don't format error messages on the server side.
This will be appended onto an error which will be in english.

Removed error translation in 307252b

jgowdyelastic · 2020-12-09T09:51:02Z

x-pack/plugins/transform/server/routes/api/error_utils.ts

+/**
+ * Returns an error message based on the root cause
+ */
+function extractErrorMessage({ type, reason, line, col }: EsError): string {


I'm not sure this is guaranteed to match all errors raised from elasticsearch.
Also, the root_cause may not have the most relevant information for the error.
In ML we pass the whole es error through to the client and display it when the user clicks the more info button.

We also have a extractErrorMessage function in the public code of transforms which we bring over from the ML plugin, maybe we can do the same on the server side (assuming there's also a server side function for the ML plugin)?

It's definitely worth improving error reporting, but in this PR I focused on preserving the initial error message shape.
I'll create an issue to provide more detailed errors in the transform UI.

peteharverson

Tested latest edits and LGTM

kibanamachine · 2020-12-09T11:43:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 307252b

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	46960	47719	+759

History

💚 Build #93013 succeeded 4099aa7
💚 Build #92647 succeeded fd5e988
💔 Build #92449 failed 4e44a09
💔 Build #92442 failed 605b016
💔 Build #92420 failed 7f1bbe9

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

* [Transform] replace legacy elasticsearch client * [Transform] delete custom legacy client definition, update transforms_audit_messages.ts * [Transform] fix start and stop transform endpoints * [Transform] fix privileges and stats endpoints * [Transform] fix forbidden * [Transform] revert continue statement, add a comment * [Transform] update privileges.ts using security namespace * [Transform] fix error wrappers * [Transform] add functional test for preview error validation * [Transform] extract error message from the root cause * [Transform] remove error translation

…k-field-to-hot-phase * 'master' of github.com:elastic/kibana: (429 commits) simplify popover open state logic (elastic#85379) [Logs UI][Metrics UI] Move actions to the kibana header (elastic#84648) [Search Source] Do not pick scripted fields if * provided (elastic#85133) [Search] Session SO polling (elastic#84225) [Transform] Replace legacy elasticsearch client (elastic#84932) [Uptime]Refactor header and action menu (elastic#83779) Fix agg select external link (elastic#85380) [ILM] Show forcemerge in hot when rollover is searchable snapshot is enabled (elastic#85292) clear using keyboard (elastic#85042) [GS] add tag and dashboard suggestion results (elastic#85144) [ML] API integration tests - skip GetAnomaliesTableData Add ECS field for event.code. (elastic#85109) [Functional][TSVB] Wait for markdown textarea to be cleaned (elastic#85128) skip flaky suite (elastic#62060) skip flaky suite (elastic#85098) Bump highlight.js to v9.18.5 (elastic#84296) Add `server.publicBaseUrl` config (elastic#85075) [Alerting & Actions ] More debug logging (elastic#85149) [Security Solution][Case] Manual attach alert to a case (elastic#82996) Loosen UUID regex to accept uuidv1 or uuidv4 (elastic#85338) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/index.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts # x-pack/plugins/index_lifecycle_management/server/routes/api/policies/register_create_route.ts

* [Transform] replace legacy elasticsearch client * [Transform] delete custom legacy client definition, update transforms_audit_messages.ts * [Transform] fix start and stop transform endpoints * [Transform] fix privileges and stats endpoints * [Transform] fix forbidden * [Transform] revert continue statement, add a comment * [Transform] update privileges.ts using security namespace * [Transform] fix error wrappers * [Transform] add functional test for preview error validation * [Transform] extract error message from the root cause * [Transform] remove error translation

[Transform] replace legacy elasticsearch client

13a130f

darnautov added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Transforms Transforms v7.11.0 labels Dec 3, 2020

darnautov requested review from peteharverson and walterra December 3, 2020 18:17

darnautov self-assigned this Dec 3, 2020

darnautov requested a review from a team as a code owner December 3, 2020 18:17

[Transform] delete custom legacy client definition, update transforms…

fcbc2c4

…_audit_messages.ts

peteharverson requested a review from jgowdyelastic December 4, 2020 09:50

darnautov added 4 commits December 4, 2020 13:54

[Transform] fix start and stop transform endpoints

4b6696c

Merge remote-tracking branch 'upstream/master' into transform-new-es-…

e7b9331

…client

[Transform] fix privileges and stats endpoints

c597ebd

Merge remote-tracking branch 'upstream/master' into transform-new-es-…

3df72cc

…client

jgowdyelastic reviewed Dec 7, 2020

View reviewed changes

darnautov added 5 commits December 7, 2020 15:15

Merge remote-tracking branch 'upstream/master' into transform-new-es-…

6505b30

…client

[Transform] fix forbidden

7f1bbe9

[Transform] revert continue statement, add a comment

605b016

[Transform] update privileges.ts using security namespace

4e44a09

[Transform] fix error wrappers

fd5e988

darnautov requested a review from jgowdyelastic December 8, 2020 12:45

jgowdyelastic reviewed Dec 8, 2020

View reviewed changes

darnautov requested a review from jgowdyelastic December 8, 2020 13:05

jgowdyelastic approved these changes Dec 8, 2020

View reviewed changes

peteharverson reviewed Dec 8, 2020

View reviewed changes

x-pack/plugins/transform/server/routes/api/error_utils.ts Outdated Show resolved Hide resolved

darnautov added 2 commits December 8, 2020 17:50

Merge remote-tracking branch 'upstream/master' into transform-new-es-…

e667fb9

…client

[Transform] add functional test for preview error validation

9b06f7e

[Transform] extract error message from the root cause

4099aa7

darnautov requested a review from peteharverson December 9, 2020 09:06

walterra approved these changes Dec 9, 2020

View reviewed changes

jgowdyelastic reviewed Dec 9, 2020

View reviewed changes

[Transform] remove error translation

307252b

peteharverson approved these changes Dec 9, 2020

View reviewed changes

darnautov merged commit 58fc711 into elastic:master Dec 9, 2020

darnautov deleted the transform-new-es-client branch December 9, 2020 11:55

darnautov mentioned this pull request Dec 9, 2020

[7.x] [Transform] Replace legacy elasticsearch client (#84932) #85396

Merged

peteharverson mentioned this pull request Dec 10, 2020

[Meta] Switch to the new Elasticsearch client #83910

Closed

74 tasks

Conversation

darnautov commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

elasticmachine commented Dec 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic left a comment

Choose a reason for hiding this comment

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walterra Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Dec 9, 2020

💚 Build Succeeded

Metrics [docs]

Distributable file count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

darnautov commented Dec 3, 2020 •

edited

Loading

jgowdyelastic Dec 8, 2020 •

edited

Loading

walterra Dec 9, 2020 •

edited

Loading