honeybadger-io / honeybadger-python

Send Python and Django errors to Honeybadger.
https://www.honeybadger.io/
MIT License
15 stars 25 forks source link

Honeybadger FastAPI plugin. #78

Closed stefanondisponibile closed 3 years ago

stefanondisponibile commented 3 years ago

Hello! I would love to add FastAPI support to the Honeybadger package, and I'd love to discuss some issues before diving deeper into the code.

First off, I'm already using Honeybadger with this framework (also in production): the solution I adopted is having a middleware handing the exceptions and notifying them when needed.

This approach would have the advantage of the possibility of implementing a single ASGI middleware for all the ASGI-based frameworks, but the disadvantage of not having an easy way for consuming the requests bodies in the middleware (and therefore sending them to Honeybadger). This is a known issue. I've been having a look around and Sentry has adopted the middleware solution, accepting the drawbacks of not parsing the body.

However, I understand that the request body may include useful piece of information from a debugging point of view. An alternative would be implementing and setting a custom APIRoute, which is the neat workaround offered by FastAPI to give access to the body of the request, but it's coming with its own drawbacks aswell:

  1. It would work just for FastAPI; (not a big issue)
  2. What if a FastAPI's user wants to set his own custom APIRoute? (that's quite of an issue)
  3. Users would need to set the custom APIRoute on their app and on each of their routers, or we'd have to elaborate some funny way of doing it under their backs, which doesn't sound too much of a good idea to me.

So, if that was me, I would keep the middleware solution and maybe offer the custom APIRoute as a "know-what-you-re-doing" alternative, but I would love an opinion on this approach.

Thank you!

joshuap commented 3 years ago

@stefanondisponibile thanks for the great issue writeup! I like your suggestion of going with the middleware approach by default and letting people use a custom APIRoute if they know what they're doing; this actually works well for me because I like to have people opt-in to sending the request body anyway, since it can potentially contain sensitive information.

Do you want to take a shot at implementing the ASGI middleware?

stefanondisponibile commented 3 years ago

sounds good @joshuap! Thank you for your feedback and yep, I'll be working on it and issuing a PR soon. 👍

joshuap commented 3 years ago

Sounds great! (assigning just so we can track who's working on what)

stefanondisponibile commented 3 years ago

hello @joshuap, here I am for a quick update. I've implemented:

I think I still need to review everything (and a couple of fixes here and there), but that's basically it.

Then I'll have to write a couple of tests: I'm a bit unsure about that, because with ASGI we are talking about any potential framerork/library. Moreover, it's easier testing FastAPI with pytest. Do you have any advice on that?

joshuap commented 3 years ago

@stefanondisponibile I'm honestly not sure about the testing setup tbh (I'm not super experienced w/ Python). I'd say keep the test as simple as possible (i.e. maybe test that the API conforms to what any ASGI-based framework will expect?) We don't necessarily need a full integration test, although that can be useful. I'd rather not change testing frameworks if possible (or if we do, I'd want to upgrade all tests to use the new framework). Our current tests are here.

cc @Kelvin4664 - let me know if you have anything to add

Kelvin4664 commented 3 years ago

I am not sure how much similarity ASGI frameworks share, I believe we can limit our tests to Fastapi for a start, than try to support a myriad of ASGI frameworks. Also I agree we don't need a full integration test. It is both tricky and monotonous. Testing the individual components in isolation should suffice.

stefanondisponibile commented 3 years ago

Thanks! I'm working in that direction 👍