[ILM] Add esErrorHandler for the new es js client #80302

yuliacech · 2020-10-13T10:05:32Z

Summary

Fixes #79678
#77906 updated elasticsearch js client from legacy to the new version and added a new way of handling es errors.
This PR extracts this error handler into es_ui_shared. I believe all our plugins use the same error processing if the request fails. The isEsError can be deleted after it will have been replaced with the new function. I haven't renamed the legacy function since it will cause a lot of files to be changed.

elasticmachine · 2020-10-13T16:21:45Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

cjcenizal · 2020-10-13T21:02:07Z

src/plugins/es_ui_shared/__packages_do_not_import__/errors/es_error_handler.ts

+  error: ApiError;
+  response: KibanaResponseFactory;
+}
+export const esErrorHandler = ({ error, response }: EsErrorHandlerParams): IKibanaResponse => {


Nit: can we rename this with a verb? I find that code that uses verb-based method names and noun-based object names is a bit easier to read. For example handleEsError would be a little more self-descriptive when reading the consuming code:

} catch (error) { return handleEsError({ error, response }); }

Great suggestion, @cjcenizal, I agree! Using a verb-based method name here reads almost like a natural language :D

yuliacech · 2020-10-14T15:49:02Z

@elasticmachine merge upstream

alisonelizabeth

Great work @yuliacech! Thanks for creating this util. I did not test locally.

I left two suggestions, nothing blocking, but might be nice to address before merging.

It also might be helpful to add a comment to handle_es_error and is_es_error indicating one is intended for the new es client, and the other is for the legacy client and will eventually be removed.

alisonelizabeth · 2020-10-15T00:58:18Z

src/plugins/es_ui_shared/__packages_do_not_import__/errors/handle_es_error.ts

+  // error.name is slightly better in terms of performance, since all errors now have name property
+  if (error.name === 'ResponseError') {
+    return response.customError({
+      // we can ignore typescript error, since error is a ResponseError


Can we update EsErrorHandlerParams to use ResponseError instead of ApiError, and remove the @ts-ignore?

import { ResponseError } from '@elastic/elasticsearch/lib/errors'; interface EsErrorHandlerParams { error: ResponseError; response: KibanaResponseFactory; }

I think here it needs to be ApiResponse since es client can return different types or errors (TimeoutError, ConfigurationError etc), but they all will have a name property.

Ah, right. I read the comment and forgot it was scoped within the if block 😅.

What about something like this?

const { statusCode, body } = error as ResponseError; return response.customError({ statusCode, body: { message: body.error?.reason }, });

thank you @alisonelizabeth , I totally forgot type casting exists 🤦🏻‍♀️

alisonelizabeth · 2020-10-15T01:01:35Z

x-pack/plugins/index_lifecycle_management/server/routes/api/index/register_add_policy_route.ts

@@ -9,6 +9,7 @@ import { ElasticsearchClient } from 'kibana/server';

 import { RouteDependencies } from '../../../types';
 import { addBasePath } from '../../../services';
+import { handleEsError } from '../../../shared_imports';


nit: Instead of importing handleEsError everywhere, we could do something similar to what we were doing previously with isEsError and only import it in the main server/plugin.ts file as a parameter to registerApiRoutes.

https://github.com/elastic/kibana/blob/7.10/x-pack/plugins/index_lifecycle_management/server/plugin.ts#L102

yuliacech · 2020-10-15T09:32:23Z

@elasticmachine merge upstream

kibanamachine · 2020-10-16T12:42:52Z

💚 Build Succeeded

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

Metrics [docs]

distributable file count

id	before	after	diff
`default`	48495	48497	+2
`oss`	29211	29212	+1

History

💛 Build #81861 was flaky 268d165
💚 Build #81850 succeeded 92d703e
💛 Build #81815 was flaky 82c838e
💚 Build #81672 succeeded b3e7a65
💔 Build #81625 failed 38b72c1

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

* [ILM] Add esErrorHandler for the new es js client * [ILM] Fix function call params * Rename function * Rename function * Rename function * [ILM] Change import of handleEsError to lib dependency * [ILM] Add comments about legacy and new es js client * [ILM] Add type casting for ResponseError Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* [ILM] Add esErrorHandler for the new es js client * [ILM] Fix function call params * Rename function * Rename function * Rename function * [ILM] Change import of handleEsError to lib dependency * [ILM] Add comments about legacy and new es js client * [ILM] Add type casting for ResponseError Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: (51 commits) [Discover] Unskip flaky test (elastic#80670) Fix security solution template label (elastic#80754) [Ingest]: ignore 404, check if there are transforms in results. (elastic#80721) Moving loader to logo in header, add a slight 250ms pause (elastic#78879) [Security Solution][Cases] Fix bug with case connectors (elastic#80642) Update known-plugins.asciidoc (elastic#75388) [Lens] Add median operation (elastic#79453) Fix navigateToApp logic when navigating to the current app. (elastic#80809) [Visualizations] Fix bad color mapping with multiple split series (elastic#80801) [ILM] Add esErrorHandler for the new es js client (elastic#80302) Fix codeowners (elastic#80826) skip flaky suite (elastic#79463) [Timelion] Remove kui usage (elastic#80287) [Ingest Manager] add skipIfNoDockerRegistry to package_install_complete test (elastic#80779) [Alerting UI] Disable "Save" button for Alerts with broken Connectors (elastic#80579) Allow the default space to be accessed via `/s/default` (elastic#77109) Add script to identify plugin dependencies for TS project references migration (elastic#80463) [Search] Client side session service (elastic#76889) feat: 🎸 add separator for different context menu groups (elastic#80498) Lazy load reporting (elastic#80492) ...

[ILM] Add esErrorHandler for the new es js client

a6ef0df

yuliacech added release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.11.0 v8.0.0 chore labels Oct 13, 2020

[ILM] Fix function call params

de5dc91

yuliacech marked this pull request as ready for review October 13, 2020 16:21

yuliacech requested a review from a team as a code owner October 13, 2020 16:21

yuliacech requested a review from alisonelizabeth October 13, 2020 16:21

cjcenizal reviewed Oct 13, 2020

View reviewed changes

yuliacech added 3 commits October 14, 2020 16:14

Rename function

f7923a8

Rename function

bedc8ab

Rename function

38b72c1

Merge branch 'master' into es_error_handler

b3e7a65

alisonelizabeth approved these changes Oct 15, 2020

View reviewed changes

Merge branch 'master' into es_error_handler

82c838e

walterra mentioned this pull request Oct 15, 2020

[ML] DF Analytics results: object fields missing in table #65768

Closed

yuliacech added 3 commits October 15, 2020 14:31

[ILM] Change import of handleEsError to lib dependency

92d703e

[ILM] Add comments about legacy and new es js client

268d165

[ILM] Add type casting for ResponseError

4885988

yuliacech merged commit 322094a into elastic:master Oct 16, 2020

yuliacech mentioned this pull request Oct 16, 2020

[7.x] [ILM] Add esErrorHandler for the new es js client (#80302) #80830

Merged

cjcenizal added the Feature:ILM label Oct 27, 2020

cjcenizal mentioned this pull request Dec 4, 2020

Stack Management elasticsearch-js client migration #73973

Closed

17 tasks

yuliacech deleted the es_error_handler branch February 24, 2021 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ILM] Add esErrorHandler for the new es js client #80302

[ILM] Add esErrorHandler for the new es js client #80302

yuliacech commented Oct 13, 2020 •

edited

Loading

elasticmachine commented Oct 13, 2020

cjcenizal Oct 13, 2020

yuliacech Oct 14, 2020

yuliacech commented Oct 14, 2020

alisonelizabeth left a comment

alisonelizabeth Oct 15, 2020

yuliacech Oct 15, 2020

alisonelizabeth Oct 15, 2020

yuliacech Oct 16, 2020

alisonelizabeth Oct 15, 2020

yuliacech commented Oct 15, 2020

kibanamachine commented Oct 16, 2020

[ILM] Add esErrorHandler for the new es js client #80302

[ILM] Add esErrorHandler for the new es js client #80302

Conversation

yuliacech commented Oct 13, 2020 • edited Loading

Summary

elasticmachine commented Oct 13, 2020

cjcenizal Oct 13, 2020

Choose a reason for hiding this comment

yuliacech Oct 14, 2020

Choose a reason for hiding this comment

yuliacech commented Oct 14, 2020

alisonelizabeth left a comment

Choose a reason for hiding this comment

alisonelizabeth Oct 15, 2020

Choose a reason for hiding this comment

yuliacech Oct 15, 2020

Choose a reason for hiding this comment

alisonelizabeth Oct 15, 2020

Choose a reason for hiding this comment

yuliacech Oct 16, 2020

Choose a reason for hiding this comment

alisonelizabeth Oct 15, 2020

Choose a reason for hiding this comment

yuliacech commented Oct 15, 2020

kibanamachine commented Oct 16, 2020

💚 Build Succeeded

Metrics [docs]

distributable file count

History

yuliacech commented Oct 13, 2020 •

edited

Loading