Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Fastapi contrib #369

Closed
wants to merge 2 commits into from
Closed

Add Fastapi contrib #369

wants to merge 2 commits into from

Conversation

arthurio
Copy link

@arthurio arthurio commented Mar 13, 2021

Description of the change

Add basic support for fastapi. Since accessing the request data (body and GET/POST) is done via asynchronous methods I'm not sure how to implement that. Looks like older version of python are still supported so it'll have to wait... At least this PR can be used as a guide for people seeking setting up rollbar with fastapi.

TODO

  • Support adding body and GET/POST to request data
  • Support adding person information

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers mentioned in a comment
  • Changes have been reviewed by at least one other engineer
  • Issue from task tracker has a link to this pull request

@bxsx
Copy link
Contributor

bxsx commented Mar 22, 2021

Hi @arthurio ! Thanks for your effort on this PR. Great job 👌

Great timing too as we are about to release pyrollbar with our own FastAPI integration. I'm afraid this PR won't be merged though but the good news is that the release should be ready in the coming weeks!

Stay tuned 😎✨

@arthurio
Copy link
Author

@bxsx Great to hear 👍🏻

@tedmiston
Copy link

@bxsx Do you have more info on when your FastAPI integration will be released? I had to switch FastAPI projects to Sentry in the meantime but would like to have them in Rollbar.

@bxsx
Copy link
Contributor

bxsx commented Apr 1, 2021

@tedmiston I believe it will be released soon after upcoming public holidays.

@tedmiston
Copy link

awesome. thank you!

@bxsx
Copy link
Contributor

bxsx commented Jun 22, 2021

@arthurio @tedmiston @NQTrg

We've released v0.16.0 with ASGI / Starlette / FastAPI support. You can update pyrollbar to the latest version (v0.16.1 as I write it) via pip and try it out: pip install -U rollbar

Documentation available at https://docs.rollbar.com/docs/python

@arthurio
Copy link
Author

@bxsx Will do! Thanks for the update and all the work that went in.

@bxsx
Copy link
Contributor

bxsx commented Jun 22, 2021

Thanks @arthurio !
Yeah, it took a lot of work to make it the way we want. Thanks for all your patience.

PS. We have a few extra integration features in our roadmap, so expect some improvements over time. In any case, please let us know if anything is missing!

@tedmiston
Copy link

Thank you! Looking forward to checking it out.

@arthurio
Copy link
Author

arthurio commented Jun 23, 2021

@bxsx I was able to set it up for fastapi but the fact that you catch errors at the router level means that expected errors (like validation or unauthorized requests, i.e. HTTPException) are caught and reported by rollbar. What I had before was something along the lines of:

    @app.exception_handler(Exception)
    async def handle_unexpected_exceptions(request: Request, exc: Exception):
        """Handle exceptions that were unexpected in endpoints.
        Send an error message to rollbar and let it blow up.
        """
        try:
            raise exc
        except Exception:
            rollbar.report_exc_info(request=request)

        return Response(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR)

I haven't found a straightforward way to do that with your implementation. Do you have any recommendations?

Edit: For now I'm going with the following, let me know if you can think of a better way:

import sys

import rollbar
from fastapi import Request, __version__, status
from fastapi.responses import Response
from rollbar.lib._async import report_exc_info

from . import settings


def configure(app):
    """Init rollbar module."""

    rollbar.init(
        settings.rollbar_token,
        environment=settings.environment,
        root=str(settings.src_dir),
        capture_ip=True,
        handler="async",
        include_request_body=True,
    )

    def _hook(request, data):
        data["framework"] = f"fastapi {__version__}"

    rollbar.BASE_DATA_HOOK = _hook

    @app.exception_handler(Exception)
    async def handle_unexpected_exceptions(request: Request, exc: Exception):
        """Handle exceptions that were unexpected in endpoints.

        Send an error message to rollbar and let it blow up.

        Note: To test locally, make sure to use DEBUG=0.
        """
        try:
            raise exc
        except Exception:
            await report_exc_info(sys.exc_info(), request=request)

        return Response(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR)

@bxsx
Copy link
Contributor

bxsx commented Jun 23, 2021

Thanks for taking the time for testing the new integration, @arthurio !

@bxsx I was able to set it up for fastapi but the fact that you catch errors at the router level means that expected errors (like validation or unauthorized requests, i.e. HTTPException) are caught and reported by rollbar.

You're right but this behavior is intentional for now. The rationale behind this was that we have similar approaches in some other integrations (like Django) and it looks like it can be beneficial in some cases for some of our customers. We may consider disabling the reporting of these errors by default (and provide configurable interface) if there are requests to do so. However, you can still ignore these errors.

I haven't found a straightforward way to do that with your implementation. Do you have any recommendations?

You can filter out errors that you don't want to report.

Here is an example:

import rollbar
from rollbar.contrib.fastapi import add_to as rollbar_add_to

def configure(app):
    """Init rollbar module."""

    rollbar.init(
        settings.rollbar_token,
        environment=settings.environment,
        root=str(settings.src_dir),
        capture_ip=True,
        handler="async",
        include_request_body=True,
    )

    def ignore_expected_errors(payload, **kw):
        EXPECTED_ERRORS = ('RequestValidationError', 'HTTPException')

        if payload['data']['body']['trace']['exception']['class'] in EXPECTED_ERRORS:
            return False
        return payload

    rollbar.events.add_payload_handler(ignore_expected_errors)
    rollbar_add_to(app)

Edit: For now I'm going with the following, let me know if you can think of a better way:

    @app.exception_handler(Exception)
    async def handle_unexpected_exceptions(request: Request, exc: Exception):
        """Handle exceptions that were unexpected in endpoints.

        Send an error message to rollbar and let it blow up.

        Note: To test locally, make sure to use DEBUG=0.
        """
        try:
            raise exc
        except Exception:
            await report_exc_info(sys.exc_info(), request=request)

        return Response(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR)

The problem with using the exception_handler decorator is that it's not possible to include the request body content in the payload - regardless of the rollbar.SETTINGS['include_request_body'] flag.

If that's OK for you (no request body in payload) you can be also interested in integrating via middleware. The middleware does not include the request body in the payload and does not report HTTPException or similar errors. So in this case you can integrate like this:

import rollbar
from rollbar.contrib.fastapi import ReporterMiddleware

def configure(app):
    """Init rollbar module."""

    rollbar.init(
        settings.rollbar_token,
        environment=settings.environment,
        root=str(settings.src_dir),
        capture_ip=True,
        handler="async",
        include_request_body=True, # this is ignored for ASGI middleware
    )

    app.add_middleware(ReporterMiddleware)

Let me know if you have further questions!

PS. Thanks for your feedback! I've added a note about additional errors in case of integrating via router and how to handle it.

@arthurio
Copy link
Author

@bxsx That's great, thanks for the suggestion, it works just as expected 👌🏻

@bxsx
Copy link
Contributor

bxsx commented Jun 25, 2021

Glad to hear!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants