sentry-python icon indicating copy to clipboard operation
sentry-python copied to clipboard

Patch eventlet under Sentry SDK

Open guiscaranse opened this issue 3 years ago • 9 comments

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 avatar Nov 18 '21 15:11 guiscaranse

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

temoto avatar Nov 19 '21 13:11 temoto

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.

guiscaranse avatar Nov 19 '21 14:11 guiscaranse

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 🥀

github-actions[bot] avatar Jan 03 '22 18:01 github-actions[bot]

@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.

temoto avatar Jan 03 '22 20:01 temoto

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 avatar Feb 21 '22 11:02 antonpirker

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

martinlisaturn avatar May 02 '22 18:05 martinlisaturn

@antonpirker any plans to get this reviewed + tested?

guiscaranse avatar May 10 '22 14:05 guiscaranse

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 avatar Jun 01 '22 08:06 antonpirker

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

antonpirker avatar Jun 01 '22 08:06 antonpirker

Any updates on this ?

ouss1919 avatar Dec 11 '22 12:12 ouss1919

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 avatar Jun 19 '23 06:06 antonpirker

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 avatar Jun 26 '23 07:06 antonpirker

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

antonpirker avatar Jul 10 '23 08:07 antonpirker

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. :)

sentrivana avatar Aug 16 '23 13:08 sentrivana

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 🥀

getsantry[bot] avatar Oct 18 '23 07:10 getsantry[bot]

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 avatar Oct 25 '23 09:10 szokeasaurusrex

Closing, since these changes have been merged in #2464

szokeasaurusrex avatar Oct 25 '23 13:10 szokeasaurusrex