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

Patch eventlet under Sentry SDK #1258

Closed
wants to merge 5 commits into from

Conversation

guiscaranse
Copy link
Contributor

@guiscaranse guiscaranse commented Nov 18, 2021

This patches the eventlet library to allow it to run without issues under the Sentry SDK when using greenlet>=0.5. Eventlet doesn't pin the greenlet dependency and greenlet has been supporting contextvars for some time now (that's why the sdk runs nicely under Gevent for example). So, the eventlet library itself is not a good marker to whatever the contextvars are broken or not.

This should fix #1036

@guiscaranse guiscaranse marked this pull request as ready for review November 18, 2021 15:34
sentry_sdk/utils.py Outdated Show resolved Hide resolved
sentry_sdk/utils.py Outdated Show resolved Hide resolved
@temoto
Copy link
Contributor

temoto commented Nov 19, 2021

Sorry for stepping in without invitation, I wish they removed "contextvars don't work with eventlet" warning from docs.

@guiscaranse
Copy link
Contributor Author

guiscaranse commented Nov 19, 2021

Sorry for stepping in without invitation, I wish they removed "contextvars don't work with eventlet" warning from docs.

Everyone is welcome haha!
Yes, I think if this patch gets approved they should remove the warning since the default behavior right now is to flag the contextvars broken on the sdk and use locals.

@github-actions
Copy link

github-actions bot commented Jan 3, 2022

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@temoto
Copy link
Contributor

temoto commented Jan 3, 2022

@rhcarvalho @lobsterkatie @iker-barriocanal @AbhiPrasad would you please look at this? Awesome patch. The problem worked around with _is_contextvars_broken function has long been fixed in greenlet lib.

@antonpirker
Copy link
Member

Hey @guiscaranse and @temoto !
Thanks for the great work.
I think this is a important contribution to improve Sentry with async Python. I will try to review and test this

@antonpirker antonpirker self-assigned this Feb 21, 2022
@antonpirker antonpirker added the Integration: gevent/eventlet Green threads and the sorts. label Feb 21, 2022
@martinlisaturn
Copy link

Any updates on this? We would love to have this functionality in so we can move our deployment to eventlets!

@guiscaranse
Copy link
Contributor Author

@antonpirker any plans to get this reviewed + tested?

@antonpirker
Copy link
Member

Hey @guiscaranse
Sorry for taking soo long. I think it looks good.
Could you please rebase your branch on master again (or merge master into it)
So we have an up to date PR and can let the tests run properly? Would be amazing!

@antonpirker
Copy link
Member

In the mean time @sl0thentr0py could you also please have a look? Thanks!

@ouss1919
Copy link

Any updates on this ?

@antonpirker antonpirker enabled auto-merge (squash) June 19, 2023 06:28
@antonpirker
Copy link
Member

Hey @guiscaranse, @temoto and @ouss1919 !
Sorry again that this one fell through the cracks. Could one of you rebase again on master, so the CI checks ran run and then the changes will be merged automatically!

Thanks!

@antonpirker
Copy link
Member

As Github CI dropped Python 2.7 in the runners, you will have to merge the new master again into you branch (this fixes the Python 2.7 tests) @guiscaranse

And there is one code style error [1]. Try run black sentry_sdk tests on you local machine. Make sure it is black==23.3.0 so it is the same version as in CI. Thanks a lot!

@antonpirker
Copy link
Member

Hey @guiscaranse I need to be annoying again. Could you merge master again into this branch? Then the tests should be all green. Thanks!

@sentrivana
Copy link
Contributor

Hey @guiscaranse, would it be possible to set the PR as open to edits from maintainers? Then we could do the missing rebasing/linting changes ourselves without having to bother you all the time. :)

@getsantry
Copy link

getsantry bot commented Oct 18, 2023

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@szokeasaurusrex
Copy link
Member

We have opened a new PR (#2464), which contains the same commits as this PR. We are updating the new PR with the latest changes from master so that these changes can get merged.

@szokeasaurusrex szokeasaurusrex removed their assignment Oct 25, 2023
@szokeasaurusrex
Copy link
Member

Closing, since these changes have been merged in #2464

auto-merge was automatically disabled October 25, 2023 13:47

Pull request was closed

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

Successfully merging this pull request may close these issues.

Sentry with eventlet
9 participants