Skip to content

ref: Patched functions decorator for integrations#2454

Merged
szokeasaurusrex merged 28 commits into
sentry-sdk-2.0from
szokeasaurusrex/sentry_patched_decorator
Mar 19, 2024
Merged

ref: Patched functions decorator for integrations#2454
szokeasaurusrex merged 28 commits into
sentry-sdk-2.0from
szokeasaurusrex/sentry_patched_decorator

Conversation

@szokeasaurusrex

@szokeasaurusrex szokeasaurusrex commented Oct 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

antonpirker commented Oct 20, 2023

Copy link
Copy Markdown
Contributor

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

antonpirker
antonpirker reviewed Oct 20, 2023
View reviewed changes

@antonpirker antonpirker left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread sentry_sdk/utils.py Outdated
Comment thread sentry_sdk/utils_py3.py Outdated
Comment thread sentry_sdk/utils.py Outdated
@antonpirker

antonpirker commented Oct 20, 2023

Copy link
Copy Markdown
Contributor

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

szokeasaurusrex commented Oct 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

szokeasaurusrex commented Mar 18, 2024

Copy link
Copy Markdown
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 szokeasaurusrex mentioned this pull request Mar 18, 2024
Merged
szokeasaurusrex added a commit that referenced this pull request Mar 18, 2024
@szokeasaurusrex
test(gql): Remove problematic tests (#2835)
cee64e0 
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 commented Mar 18, 2024
View reviewed changes
Comment thread sentry_sdk/utils.py
@szokeasaurusrex szokeasaurusrex mentioned this pull request Mar 18, 2024
Merged
@szokeasaurusrex szokeasaurusrex changed the title Patched functions decorator for integrations ref: Patched functions decorator for integrations Mar 18, 2024
antonpirker
antonpirker approved these changes Mar 18, 2024
View reviewed changes

@antonpirker antonpirker left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread sentry_sdk/utils.py Outdated
@antonpirker antonpirker mentioned this pull request Mar 19, 2024
Closed
@szokeasaurusrex
szokeasaurusrex merged commit 7a2c153 into sentry-sdk-2.0 Mar 19, 2024
@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

Reviewers

@sentrivana sentrivana Awaiting requested review from sentrivana
1 more reviewer
@antonpirker antonpirker antonpirker approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@szokeasaurusrex szokeasaurusrex

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@szokeasaurusrex @antonpirker