Skip to content

Execution context - use thread locals if gevent/eventlet has monkey patched it #453

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

Closed
wants to merge 1 commit into from

Conversation

ramshaw888
Copy link
Contributor

@ramshaw888 ramshaw888 commented Apr 9, 2019

  • Extract out logic to decide which backing to use for execution_context into a function init_execution_context
  • Check if gevent or eventlet has monkey patched _threading.local, if it has then use elasticapm.context.threadlocal as the backing.

Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

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

Great stuff! Would you mind adding a check for eventlet?

/edit: I pushed an import sort change to your branch, hope that's ok

return execution_context


def threading_local_monkey_patched():
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're at it, it might make sense to do the same check for eventlet

http://eventlet.net/doc/patching.html#eventlet.patcher.is_monkey_patched

@beniwohli
Copy link
Contributor

jenkins test this

@beniwohli beniwohli closed this in 5337c43 Apr 9, 2019
@beniwohli beniwohli reopened this Apr 9, 2019
@beniwohli
Copy link
Contributor

Oops, sorry for closing the PR, got ticket numbers mixed up in the commit message for 5337c43 🤦‍♂️

@beniwohli
Copy link
Contributor

@ramshaw888 The build currently fails because of a linting issue. We use black for code formatting (in addition to isort for import sorting and flake8 for general linting). You can run these linters locally before committing using pre-commit

$ pip install pre-commit # or brew install pre-commit if you are on mac and use homebrew
$ pre-commit install  # in your apm-agent-python directory

I'll make sure to add these steps to our contributing guidelines.

@ramshaw888 ramshaw888 force-pushed the monkey-thread-locals branch from 29bfaa4 to 6d9a7ff Compare April 9, 2019 21:55
@ramshaw888 ramshaw888 changed the title Execution context - use thread locals if gevent has monkey patched it Execution context - use thread locals if gevent/eventlet has monkey patched it Apr 9, 2019
@ramshaw888 ramshaw888 force-pushed the monkey-thread-locals branch from d6c486d to 2ebdf7c Compare April 9, 2019 23:09
ramshaw888 added a commit to ramshaw888/apm-agent-python that referenced this pull request Apr 9, 2019
…atched it elastic#453

* Extract out logic to decide which backing to use for execution_context
into a function init_execution_context
* Check if gevent or eventlet has monkey patched _threading.local, if it
has then use elasticapm.context.threadlocal as the backing.
…atched it elastic#453

* Extract out logic to decide which backing to use for execution_context
into a function init_execution_context
* Check if gevent or eventlet has monkey patched _threading.local, if it
has then use elasticapm.context.threadlocal as the backing.
@ramshaw888 ramshaw888 force-pushed the monkey-thread-locals branch from 2ebdf7c to 58dc200 Compare April 9, 2019 23:10
@ramshaw888
Copy link
Contributor Author

ramshaw888 commented Apr 9, 2019

I think the closing/reopening might have messed up the appveyor integration. Will close and re-open in a new PR.

@ramshaw888 ramshaw888 closed this Apr 9, 2019
beniwohli pushed a commit that referenced this pull request Apr 10, 2019
…atched it (#454)

* Execution context - use thread locals if gevent/eventlet has monkey patched it #453

* Extract out logic to decide which backing to use for execution_context
into a function init_execution_context
* Check if gevent or eventlet has monkey patched _threading.local, if it
has then use elasticapm.context.threadlocal as the backing.

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

Successfully merging this pull request may close these issues.

3 participants