aserafin / grape_logging

Request logging for Grape!
MIT License
147 stars 76 forks source link

Update how grape logging middleware should be placed #74

Closed luong-komorebi closed 3 years ago

luong-komorebi commented 3 years ago

At my company we use grape logging to perform rich logging of our grape API endpoints.

However, there were casual cases where I can see in our monitoring system, there is no response with status 500, but a bunch of these are being printed in the log.

That leads me to finding out what caused this and it seems to be the case (not sure if niche) that grape_logging is being required after Grape's default Error middleware.

Brief description in call stack: (notice the number)

image image

This leads to the case where an app has an error with a status other than 500, this https://github.com/ruby-grape/grape/blob/87cb0052b874b40824faeae2cfbd73c28da1d69b/lib/grape/middleware/error.rb#L63 line runs , and it runs after this line https://github.com/aserafin/grape_logging/blob/8afa893a698d2d9c17e72dadcae77c2f3551cde2/lib/grape_logging/middleware/request_logger.rb#L63 so the status that this middleware is logging is always 500 for all cases

A simple fix would be enhancing the doc, while more advanced one should be automatically checking the required stack to see if our logging middleware comes before error middleware.

luong-komorebi commented 3 years ago

@aserafin could you help review this? I believe this is a high impact document for new users of the library, without which would lead to confusion and issues raised

aserafin commented 3 years ago

@luong-komorebi that makes sense - thank you for the PR 🙇