Skip to content

Patch eventlet under Sentry SDK - #1258

Closed
guiscaranse wants to merge 5 commits into
getsentry:masterfrom
Saturn-Technologies:master
Closed

guiscaranse wants to merge 5 commits into
getsentry:masterfrom
Saturn-Technologies:master

Conversation

@guiscaranse

@guiscaranse guiscaranse commented Nov 18, 2021 •

Copy link
Copy Markdown
Contributor

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
Comment thread sentry_sdk/utils.py Resolved
Comment thread sentry_sdk/utils.py Resolved
@temoto

temoto commented Nov 19, 2021

Copy link
Copy Markdown
Contributor

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

@guiscaranse

guiscaranse commented Nov 19, 2021 •

Copy link
Copy Markdown
Contributor Author

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

github-actions Bot commented Jan 3, 2022

Copy link
Copy Markdown
Contributor

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

temoto commented Jan 3, 2022

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

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
Copy Markdown

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

@guiscaranse

Copy link
Copy Markdown
Contributor Author

@antonpirker any plans to get this reviewed + tested?

@antonpirker

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@ouss1919

Copy link
Copy Markdown

Any updates on this ?

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Collaborator

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

getsantry Bot commented Oct 18, 2023

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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