Kibana: [APM] Hapi upgrade is broken for transaction endpoint

Created on 30 Oct 2018  路  3Comments  路  Source: elastic/kibana

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)

apm bug v6.6.0

All 3 comments

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. :)

Was this page helpful?
0 / 5 - 0 ratings

Related issues

hvisage picture hvisage  路  170Comments

Alex-Ikanow picture Alex-Ikanow  路  364Comments

seti123 picture seti123  路  100Comments

doubret picture doubret  路  105Comments

tbragin picture tbragin  路  81Comments