This commit: https://github.com/elastic/kibana/commit/27e5406d7a493c53a862ba16ae064a61c13819cf#diff-230614a2ac854622b8f772e73eea3048R25
upgraded Hapi in master, and converted some of the APM server files. In each defaultErrorHandler function for each of the server/routes/* files, we no longer pass in a reply() method, but instead should return or throw the Boom error. We should:
a) return the Boom error from the transactions.ts default error handler
b) change the others where we throw the error to return instead (or throw in all cases, either one is fine but let's be consistent)
Pinging @elastic/apm-ui
While fixing this, I think we should make a minor change to how we handle http errors. Most of the http layer (req, resp) is handled in files in routes/*. I think that lib/* should be completely http agnostic. Eg. get_transaction should return a transaction (if found) or undefined if not. The rest layer can then determine how to communicate this to the clients.
Eg. This should be moved to routes/transactions.ts
https://github.com/elastic/kibana/blob/fb7dbaeb6a515431d670405a87fa15adc91eb25f/x-pack/plugins/apm/server/lib/transactions/get_transaction.ts#L63-L69
In general I like the idea of serving better http errors to the clients like done in transactions.ts
That makes sense and is probably good enough for our current use-case. In my experience the model layer will eventually need to provide more information and context about certain expected error cases so that the HTTP response can provide that context or info in the error response, or centrally log the error, etc. A more general purpose ResponseError type is useful there so you can dictate various messages (logged vs returned to the client), etc, but we could also abstract that so it's still HTTP-agnostic. Probably something to improve on down the road though. :)