[ILM] Add esErrorHandler for the new es js client#80302
[ILM] Add esErrorHandler for the new es js client#80302yuliacech merged 10 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
| error: ApiError; | ||
| response: KibanaResponseFactory; | ||
| } | ||
| export const esErrorHandler = ({ error, response }: EsErrorHandlerParams): IKibanaResponse => { |
There was a problem hiding this comment.
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 });
}There was a problem hiding this comment.
Great suggestion, @cjcenizal, I agree! Using a verb-based method name here reads almost like a natural language :D
|
@elasticmachine merge upstream |
alisonelizabeth
left a comment
There was a problem hiding this comment.
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.
| // 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 |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 },
});
There was a problem hiding this comment.
thank you @alisonelizabeth , I totally forgot type casting exists 🤦🏻♀️
|
|
||
| import { RouteDependencies } from '../../../types'; | ||
| import { addBasePath } from '../../../services'; | ||
| import { handleEsError } from '../../../shared_imports'; |
There was a problem hiding this comment.
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.
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]distributable file count
History
To update your PR or re-run it, just comment with: |
* [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) ...
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. TheisEsErrorcan 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.