sentry-python
sentry-python copied to clipboard
Patch eventlet under Sentry SDK
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
Sorry for stepping in without invitation, I wish they removed "contextvars don't work with eventlet" warning from docs.
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
.
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 🥀
@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.
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
Any updates on this? We would love to have this functionality in so we can move our deployment to eventlets!
@antonpirker any plans to get this reviewed + tested?
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!
In the mean time @sl0thentr0py could you also please have a look? Thanks!
Any updates on this ?
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!
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!
Hey @guiscaranse I need to be annoying again. Could you merge master again into this branch? Then the tests should be all green. Thanks!
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. :)
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 🥀
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.
Closing, since these changes have been merged in #2464