Skip to content

Add package to provide gevent compatibiliy (#628) #636

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

Merged
merged 1 commit into from May 3, 2019
Merged

Add package to provide gevent compatibiliy (#628) #636

merged 1 commit into from May 3, 2019

Conversation

aberres
Copy link
Contributor

@aberres aberres commented Apr 30, 2019

No description provided.

@aberres aberres requested review from c24t, reyang, songy23 and a team as code owners April 30, 2019 11:53
@aberres aberres changed the title Document how to use with gevent (#628) (DO NOT MERGE YET) Document how to use with gevent (#628) Apr 30, 2019
@aberres aberres changed the title (DO NOT MERGE YET) Document how to use with gevent (#628) Document how to use with gevent (#628) Apr 30, 2019
@reyang
Copy link
Contributor

reyang commented Apr 30, 2019

Thanks @aberres!

Having this in the core library README is a bit weird (as one can imagine, eventually we'll end up with workarounds for all kinds of 3rd party stuff in the main README, which could confuse/scare the audiences), probably having an extension opencensus-ext-gevent makes more sense? @aberres @c24t

What's in my mind:

  1. Having opencensus-ext-gevent package.
  2. Specify the gevent version in setup.py (so that in the future if gevent supports async, people won't install the wrong package and flip the context back to thread-local-storage by mistake).
  3. In the __init__.py, flip the context.
  4. Mention in the package README how to use it properly (e.g. needs to be imported before any other OpenCensus components).

@aberres aberres changed the title Document how to use with gevent (#628) [wip] Document how to use with gevent (#628) Apr 30, 2019
@aberres
Copy link
Contributor Author

aberres commented Apr 30, 2019

@reyang fair enough.

I just pushed a commit containing a new package which makes things "just work" when it is installed.
After gevent did its patching matching a setuptools entry point is called. See http://www.gevent.org/api/gevent.monkey.html#plugins for details.

I did not polish this yet, I want to check first of this is more to your liking.

Copy link
Contributor

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, I've left some minor suggestions.

@reyang
Copy link
Contributor

reyang commented Apr 30, 2019

I just pushed a commit containing a new package which makes things "just work" when it is installed.
After gevent did its patching matching a setuptools entry point is called. See http://www.gevent.org/api/gevent.monkey.html#plugins for details.

This approach is amazing!

I did not polish this yet, I want to check first of this is more to your liking.

This is great, thanks for the contribution @aberres!

Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @reyang said, we need a test that the patch works as expected before we can merge this. It would also be useful to check the entry point, i.e. load the patched gevent first.

Copy link
Contributor

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will merge once CI is clean.

@aberres aberres changed the title [wip] Document how to use with gevent (#628) Add package to provide gevent compatibiliy (#628) May 3, 2019

logging.warning("OpenCensus patched for gevent compatibility")
else:
logging.warning(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@reyang reyang merged commit 9179476 into census-instrumentation:master May 3, 2019
@reyang
Copy link
Contributor

reyang commented May 3, 2019

Thanks @aberres for the great work!

@allenjohnashton
Copy link

How long until this package hits pypi?

@reyang
Copy link
Contributor

reyang commented Jun 1, 2019

@allenjohnashton sorry for the delay, here you go https://pypi.org/project/opencensus-ext-gevent/.

In general we try to have at least a release per month.

@allenjohnashton
Copy link

Thanks!

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.

6 participants