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

ref: Patched functions decorator for integrations #2454

Merged

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Oct 18, 2023

This PR introduces two new decorators in sentry_sdk.utils that we can use in our integrations to automate the checks for whether the integration is still enabled. Since these decorators use the new scopes API, adopting these decorators may simplify the change to the new Scopes API.

For example, we could replace the below sample, which uses the old Hub API:

async def _sentry_app(*args, **kwargs):
     # type: (*Any, **Any) -> Any
     hub = Hub.current
     integration = hub.get_integration(FastApiIntegration)
     if integration is None:
         return await old_app(*args, **kwargs)

    ... # rest of function body

with the code below, using these decorators:

@ensure_integration_enabled_async(FastApiIntegration, old_app)
async def _sentry_app(*args, **kwargs):
    # type: (*Any, **Any) -> Any
    ... # rest of function body

The above example is taken from #2836; the full code is available at that PR.

@antonpirker
Copy link
Member

I really like the idea, this can make our integration's code way cleaner and easier to read!

Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

Looks already very good. Left some comments.

sentry_sdk/utils.py Outdated Show resolved Hide resolved
sentry_sdk/utils_py3.py Outdated Show resolved Hide resolved
sentry_sdk/utils.py Outdated Show resolved Hide resolved
@antonpirker
Copy link
Member

Some tests are still failing. I guess the GQL you can fix easily. For the Starlette tests we need to check if the behavior changed now because of the wraps (I guess) if so, we would need to wait for a major release

@szokeasaurusrex
Copy link
Member Author

szokeasaurusrex commented Oct 20, 2023

Some tests are still failing. I guess the GQL you can fix easily. For the Starlette tests we need to check if the behavior changed now because of the wraps (I guess) if so, we would need to wait for a major release

If adding wraps is a breaking change, we could also remove wraps from this PR, and add it back in when we have a new major.

Update: see comment below

@szokeasaurusrex szokeasaurusrex self-assigned this Oct 20, 2023
@szokeasaurusrex szokeasaurusrex changed the base branch from master to sentry-sdk-2.0 March 15, 2024 10:56
@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review March 15, 2024 12:27
@szokeasaurusrex
Copy link
Member Author

@antonpirker The GQL tests that are failing are themselves appear to be broken; I don't think that the fact that they are failing indicates that this is a breaking change.

The reason the tests fail is because they are doing some strange monkey patching to verify that the method we monkeypatch is still being called by the integration. I don't think we do tests like this anywhere else; looks like I added them six months ago when I was less familiar with how to best test integrations.

I think we should just delete the failing tests since they provide little value, but clearly are failing even when they should not. I will open a new PR to make this change.

szokeasaurusrex added a commit that referenced this pull request Mar 18, 2024
These two tests are failing in #2454, blocking that PR from being merged. The tests appear to be broken, and since they appear to be unnecessary (we don't have similar tests for other integrations), we should delete them. See #2454 for a more detailed explanation.
@szokeasaurusrex szokeasaurusrex changed the title Patched functions decorator for integrations ref: Patched functions decorator for integrations Mar 18, 2024
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

Looks good! Great work.

Left some typo fix

sentry_sdk/utils.py Outdated Show resolved Hide resolved
@szokeasaurusrex szokeasaurusrex merged commit 7a2c153 into sentry-sdk-2.0 Mar 19, 2024
111 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/sentry_patched_decorator branch March 19, 2024 08:54
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.

2 participants