[Ingest Manager] Improve server-side error handling #67278
Conversation
botelastic
bot
added
the
Team:Fleet
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
skh
added
Ingest Management:beta1
release_note:skip
| statusCode: getHTTPResponseCode(e), | ||
| body: { message: e.getMessage() }, | ||
| }); | ||
| } | ||
| if (e.isBoom) { | ||
| logger.error(e.output.payload.message); |
There was a problem hiding this comment.
This will be removed once we stop using Boom elsewhere. This handler is currently in an in-between state where it should capture both Boom and IngestManagerError errors
| this.message = message; | ||
| } | ||
|
|
||
| public getType = (): IngestManagerErrorType => { |
There was a problem hiding this comment.
What do you think instead of relaying on type having all of our errors extending IngestManagerError
class RegistryError extends IngestManagerError {
}
There was a problem hiding this comment.
See #66688 (comment)
We can do that. We don't use that pattern (inheritance + instanceof) at the moment in ingest manager code, and I didn't want to introduce it when it isn't needed.
There was a problem hiding this comment.
I think it's a really good pattern for Error (inheritance + instanceof) and we can keep the getStatusCode method so this stay an internal to this error module, curious to have other thoughts here, maybe @jfsiii?
But it's really personal preference for me.
x-pack/plugins/ingest_manager/server/services/epm/registry/requests.ts
Outdated
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* Use IngestManagerError instead of Boom errors. * Extend Error and simplify. * Add registry url to error messages. Co-authored-by: Elastic Machine <[email protected]>
* Use IngestManagerError instead of Boom errors. * Extend Error and simplify. * Add registry url to error messages. Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
Minimal example implementation of #66688
How to test this
Mostly, read the linked issue, and read the changes in this PR. Do you agree with the approach?
To trigger the error case handled in this PR:
yarn es snapshotdoes that).xpack.ingestManager.epm.registryUrlto a non-existing registry/api/ingest_manager/setup[ingestManager])