Sanic 21.3.2 breaks sentry integration

ajay · March 23, 2021, 10:24am

I have upgraded to the latest version of Sanic, i.e, 21.3.2 and it broke my sentry integration.

Sentry version:
sentry-sdk==1.0.0
aiocontextvars==0.2.2

This is the error traceback that I get:

Traceback (most recent call last):
  File "/Users/ajay/.pyenv/versions/torpedo_boilerplate/lib/python3.7/site-packages/sanic/app.py", line 680, in handle_request
    request.path, request.method, request.headers.get("host")
TypeError: sentry_router_get() takes 2 positional arguments but 4 were given
ERROR:sanic.error:sentry_router_get() takes 2 positional arguments but 4 were given
Traceback (most recent call last):
  File "/Users/ajay/.pyenv/versions/torpedo_boilerplate/lib/python3.7/site-packages/sanic/app.py", line 680, in handle_request
    request.path, request.method, request.headers.get("host")
TypeError: sentry_router_get() takes 2 positional arguments but 4 were given

please suggest what can be done to resolve this.

ahopkins · March 23, 2021, 10:29am

I am not familiar with how the plugin works. Off the top of my head, perhaps they are passing in a custom router. If that is the case, then the plugin author needs to update the plugin.

ahopkins · March 23, 2021, 10:39am

Looks like they are monkey patching the router.

github.com

getsentry/sentry-python/blob/4a376428a5b28ca9b2871c3c39896fccf437ab2d/sentry_sdk/integrations/sanic.py#L101


            response = await response
        return response
Sanic.handle_request = sentry_handle_request
old_router_get = Router.get
def sentry_router_get(self, request):
    # type: (Any, Request) -> Any
    rv = old_router_get(self, request)
    hub = Hub.current
    if hub.get_integration(SanicIntegration) is not None:
        with capture_internal_exceptions():
            with hub.configure_scope() as scope:
                scope.transaction = rv[0].__name__
    return rv
Router.get = sentry_router_get
old_error_handler_lookup = ErrorHandler.lookup

This line here needs to be updated to pass: path, method, host like this:

github.com

sanic-org/sanic/blob/93a0246c0350b6dcaa89588497622f30541f1c87/sanic/app.py#L679


:param stream_callback: Coroutine that handles streaming a
    StreamingHTTPResponse if produced by the handler.
:return: Nothing
"""
# Define `response` var here to remove warnings about
# allocation before assignment below.
response = None
try:
    # Fetch handler from router
    route, handler, kwargs = self.router.get(
        request.path, request.method, request.headers.get("host")
    )
    request._match_info = kwargs
    request.route = route
    if (
        request.stream.request_body  # type: ignore
        and not route.ctx.ignore_body
    ):

ajay · March 23, 2021, 10:41am

Here what they have on the official sentry documentation for Sanic (Sanic | Sentry Documentation)

The Sanic integration adds support for the Sanic Web Framework. We support the following versions:

0.8

18.12

19.12

Any version of the form x.12 (LTS versions).

We do not support the latest version of Sanic. Versions between LTS releases have introduced breaking changes in the past without prior notice, so we cannot support them as they are too fast of a moving target

looks like Sanic is literally too fast moving for them

ahopkins · March 23, 2021, 11:07am

They have complained in the past about it. But, our documentation is generally pretty clear about when/how breaking changes will occurr: Policies | Sanic Framework

We usually do not break something unless there is 2 release cycles of deprecation notices, except if there being a good reason. In this particular instance, it was a noticeable erformance issue to no longer support router.get(request).

There is a major flaw in the sentry plugin. Sanic does (and has for a long time) allowed you to create a new router at startup:

class CustomRouter(Router):
    ....

app = Sanic(..., router=CustomRouter())

If old_router_get used the actual instantiated router, then it would be simple to monkeypatch their monkeypatch.

But, because of how it is written it would be hard to do without completely overwriting setup_once.

This should in theory be a simple PR that I can help you walk thru if you are interested.

ajay · March 23, 2021, 1:10pm

only passing the below class to the app instance worked:

class CustomRouter(Router):
    pass

app = Sanic('my-service', router=CustomRouter())

I would love to contribute, please suggest how can I do that

ashleysommer · March 24, 2021, 5:37am

Hi Ajay,
Thanks for your offer to contribute. Are you talking about submitting a fix to Sentry-plugin code, or to the Sanic Project itself?
If you’d like to help with Sanic, what did you have in mind?
If you are good at finding bugs and making fixes, please feel free to clone the Sanic source code and start submitting Pull-Requests on Github. We review every pull-request that is submitted.
Or even better, if you’re good at writing - we always need thorough documentation writers.

ajay · March 24, 2021, 8:21am

Hi @ashleysommer,

I meant it for both the Sentry plugin and the Sanic framework.
I will review the Sanic code this weekend and see where I would be able to contribute. Thanks for the explanation.

ahopkins · March 24, 2021, 1:57pm

Nice. We try to keep the issues in github labeled well to help people find something appropriate to work on. Feel free to ping me if you have questions or want some ideas of good first issues.