-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[ILM] Add esErrorHandler for the new es js client #80302
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion, @cjcenizal, I agree! Using a verb-based method name here reads almost like a natural language :D
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you @alisonelizabeth , I totally forgot type casting exists 🤦🏻♀️
@@ -9,6 +9,7 @@ import { ElasticsearchClient } from 'kibana/server'; | |||
|
|||
import { RouteDependencies } from '../../../types'; | |||
import { addBasePath } from '../../../services'; | |||
import { handleEsError } from '../../../shared_imports'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. TheisEsError
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.