[celery] Patch via post-import hooks #534
Conversation
ddtrace/monkey.py
Outdated
| @@ -79,7 +79,22 @@ def patch(raise_errors=True, **patch_modules): | |||
| modules = [m for (m, should_patch) in patch_modules.items() if should_patch] | |||
| count = 0 | |||
| for module in modules: | |||
| patched = patch_module(module, raise_errors=raise_errors) | |||
| # TODO: this is a temporary hack until we shift to using | |||
There was a problem hiding this comment.
This patching method is hacked into our existing mechanism. I tried to work around this but there really isn't much that can be done until we work in a completely new patching mechanism to install the hooks.
For now I think it's fine that this remain a hack for the celery release. Once it is validated with the release we can consider refactoring our patching mechanism.
ddtrace/monkey.py
Outdated
|
|
||
| # manually add celery to patched modules and increment count | ||
| _PATCHED_MODULES.add(module) | ||
| count += 1 |
There was a problem hiding this comment.
I think you should add a patched = True/False here if you want to avoid errors on line 98
ddtrace/monkey.py
Outdated
| # manually add celery to patched modules and increment count | ||
| _PATCHED_MODULES.add(module) | ||
| count += 1 | ||
| patched = True |
There was a problem hiding this comment.
reading the code I think count is incremented twice :)
There was a problem hiding this comment.
ugh, yes, good catch! we really need to change how we patch so that this dirty hack can go away very soon...
|
|
||
|
|
||
| class CeleryPatchTest(unittest.TestCase): | ||
| def test_patch_after_import(self): |
There was a problem hiding this comment.
Thanks for testing this code-path. It ensures the current behavior is preserved without any breaking change.
| if patched: | ||
| count += 1 | ||
|
|
||
| # TODO: this is a temporary hack until we shift to using |
There was a problem hiding this comment.
Our next step is removing this hack and migrate all the instrumentation to this hook system.
| @@ -130,7 +130,6 @@ deps = | |||
| celery41: celery>=4.1,<4.2 | |||
| celery42: celery>=4.2,<4.3 | |||
| ddtracerun: redis | |||
| ddtracerun: celery | |||
There was a problem hiding this comment.
Good to remove because it's already running in our test suite.
Patching Celery via Post-import Hooks
Background
Patching
celeryour usual way does not work forceleryascelerycontains a reference tosys.argv. This is problematic assys.argvis not defined insitecustomize.py.Overview
This PR introduces a fix for this by taking a completely different approach to patching by using post-import hooks as defined here: https://www.python.org/dev/peps/pep-0369.
Caveats
For simplicity and proof of concept this approach is meant only to patch
celeryvia post-import hooks. It is a bit hacky in that we check explicitly forceleryand add in the normal patching side-effects.Take-aways
It turns out that patching this way is extremely easy. We should strongly consider moving to this strategy for patching all of our modules!