-
Notifications
You must be signed in to change notification settings - Fork 394
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
[core] Add import hook module #769
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor tweaks, but otherwise good to go.
- fix test - add dedup check and log
@brettlangdon this should be good to look at again when you get the chance! 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor suggestions, but otherwise this is good to go.
ddtrace/utils/hook.py
Outdated
|
||
|
||
@synchronized(_post_import_hooks_lock) | ||
def deregister_post_import_hook(modulename, matcher): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remind me, why this approach vs:
def hook():
pass
register_post_import_hook('module_name', hook)
deregister_post_import_hook('module_name', hook)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in my initial implementation I had the idea of attaching an 'enabled' property on the hooks that would be checked. The matcher
would allow us to easily filter out the disabled hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, as long as it makes sense with the plan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm getting myself back up to speed to see if we still want to go in this direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opted to just accept the hook. If I recall correctly, our plan was to have a layer on top of this that would be meant for enabling and disabling hooks which can interface with this general api.
Co-Authored-By: Kyle-Verhoog <kyle.verhoog@datadoghq.com>
Co-Authored-By: Kyle-Verhoog <kyle.verhoog@datadoghq.com>
This PR introduces a modified version of
wrapt
's import hook mechanism that is better suited to our needs. The fundamentals stay the same the notable differences from thewrapt
system are:deregister_post_import_hook
is introduced to allow the removal of hooksTests are also added for this new functionality.