rollbar / pyrollbar

Error tracking and logging from Python to Rollbar
https://docs.rollbar.com/docs/python/
MIT License
213 stars 133 forks source link

Move requests package to optional dependency #417

Open phillipuniverse opened 1 year ago

phillipuniverse commented 1 year ago

I have a fully asynchronous FastAPI application that I use with Rollbar for error tracking. I want to avoid other developers accidentally utilizing the requests package to make external API calls as it will introduce blocking code within a fully asynchronous service.

However, requests is non-optional https://github.com/rollbar/pyrollbar/blob/55d08a65c626539d5a74a0439facc992514247e4/setup.py#L82-L87 and will always come in as a transitive dependency with Rollbar.

Ideally, adding a dependency on pyrollbar would not transitively bring in the requests package.

danielmorell commented 1 year ago

Thank you @phillipuniverse for bringing this up. That makes a lot of sense!

We currently import requests at the package level and use it as the default HTTP sender for a few handlers. We will need to try to import it if the handler uses it and throw an exception if it does not exist.

Because of that, this is a breaking change. I am adding this to our v1.0.0 milestone. We were talking internally about getting to v1.0.0 yesterday, so hopefully this is not too far in the future.

Is this update something you would be willing to contribute to?

phillipuniverse commented 1 year ago

@danielmorell yes, I can work on a contribution for this.

Do y'all have a candidate timeline for when you are looking at a 1.0 release?

danielmorell commented 1 year ago

Thank you @phillipuniverse. I greatly appreciate that!

I am aiming at release Q1 2023. But I will need to talk to product before anything concrete is determined.

homm commented 1 year ago

Please also add optional dependencies for other handlers like async. Currently there is a note in documentation:

The following examples use the async handler that requires the HTTPX package to be installed.

This is not the way how dependencies should be listed.

phillipuniverse commented 1 year ago

@danielmorell candidate PR at #422.

This is not the way how dependencies should be listed.

@homm indeed, it should be an extras_require, right? I'll wait for further feedback but could definitely roll that into my changes in #422.

homm commented 1 year ago

@phillipuniverse Right. I think it's a good idea to define one key in extras_require for every handler (even if the actual list is empty) so the one could specify rollbar[thread]>=0.17.0 and not worry about extra requirements even for future versions.

phillipuniverse commented 1 year ago

I decided to go in a slightly different direction here that didn't rely on removing the requests package from pyrollbar. I used flake8-tidy-imports, specifically from within Ruff to ban the requests api in my async projects. Example from my pyproject.toml:

    [tool.ruff.flake8-tidy-imports.banned-api]
    "requests".msg = "Use httpx instead"

This achieves my purposes of trying to prevent developers from using blocking code accidentally.