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

Refactor Sanic integration for v21.9 support #1212

Merged
merged 9 commits into from Nov 16, 2021

Conversation

ahopkins
Copy link
Contributor

@ahopkins ahopkins commented Oct 2, 2021

This PR allows for Sanic v21.9 style error handlers to operate and provide full access to handling Blueprint specific error handlers.


The ErrorHandler.lookup now requires two positional arguments:

Source

See also: sanic-org/sanic#2259


UPDATE:
Fixes #1240
Fixes sanic-org/sanic#2250

@ahopkins ahopkins changed the title Allow additional arguments to ErrorHandler.lookup Refactor Sanic integration for v21.9 support Nov 4, 2021
@ahopkins
Copy link
Contributor Author

ahopkins commented Nov 4, 2021

@AbhiPrasad So, I think this is generally a better approach. It will avoid touching handle_request which is really not a safe method to be monkeypatching.

Instead, we monkeypatch the _startup method to inject some code into the request cycle using signals. You do not need to familiarize yourself too much with them other than to know it is a method to inject arbitrary code into the request/response cycle at predetermined points.

Locally all of the tests pass, except for test_concurrency. I think before I dig too deep into this to figure out why, it might be helpful if I understood some of this stuff:

    weak_request = weakref.ref(request)
    request.ctx._sentry_hub = Hub(hub)
    request.ctx._sentry_hub.__enter__()

    with request.ctx._sentry_hub.configure_scope() as scope:
        scope.clear_breadcrumbs()
        scope.add_event_processor(_make_request_processor(weak_request))
        with capture_internal_exceptions():
            with hub.configure_scope() as scope:

Also, what is the scope and the assert in this handler trying to achieve?

    @app.route("/context-check/<i>")
    async def context_check(request, i):
        with configure_scope() as scope:
            scope.set_tag("i", i)

        await asyncio.sleep(random.random())

        with configure_scope() as scope:
            assert scope._tags["i"] == i

        return response.text("ok")

Any insight you could share?


UPDATE: Nevermind... I figured it out. I just pushed another commit now where I believe all tests 🤞 should pass (they do locally 😆 )

I ran it using 21.9, 21.6, 21.3, 20.12 ✔️

@ahopkins ahopkins marked this pull request as ready for review November 4, 2021 23:31
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

We have tried to focus on LTS versions as documented in https://docs.sentry.io/platforms/python/guides/sanic/.
We did merge in contributions to support recent non-LTS versions, but that's coming with a maintenance cost.

I'm happy we take this in to unblock users, but would also like to strongly consider dropping support for the non-LTS releases once 21.12 is out. Would that be a fair ask?


class SanicIntegration(Integration):
identifier = "sanic"

@staticmethod
def setup_once():
# type: () -> None

global version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For maintainability and testability, I'd avoid global state and make this an instance attribute of SanicIntegration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you okay with it being a class attribute?

sentry_sdk/integrations/sanic.py Outdated Show resolved Hide resolved
@ahopkins
Copy link
Contributor Author

ahopkins commented Nov 11, 2021

I'm happy we take this in to unblock users, but would also like to strongly consider dropping support for the non-LTS releases once 21.12 is out. Would that be a fair ask?

That is a contract between you and your user base. I'm happy to make this PR to help out and/or be a resource as needed.

@AbhiPrasad AbhiPrasad merged commit 5d357d0 into getsentry:master Nov 16, 2021
@AbhiPrasad
Copy link
Member

Thank you for your contribution @ahopkins!

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.

Name 'Hub' is not defined when using with Sanic 21.9.0 Breaks Sentry Integration
4 participants