Skip to content
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 module import hook install logic #754

Closed
wants to merge 8 commits into from

Conversation

Kyle-Verhoog
Copy link
Member

Overview

This PR adds the required logic necessary for installing module import hooks.

Now integrations can register their patch functions to be run when the required module is imported. This means that integrations can be certain of being the first to run after the code is imported.

What this looks like for integrations (with gevent as the example). In gevent/__init__.py:

def _patch(gevent):
    # install logic that will be run when module is imported

def patch():
    # install the module import hook to patch gevent on module import
    install_module_import_hook('gevent', _patch)

def unpatch():
    # uninstall logic...
    # remove import hook if it exists
    uninstall_module_import_hook('gevent')

Caveats

This approach uses wrapt's module import hooking system which deletes the hooks after they are run. This isn't desirable for our use-case even though it's highly unlikely that modules will be reimported or deleted. Another undesired effect of using wrapt is that we cannot uninstall module import hooks and have to import wrapt privates to do so. While it is overkill for this PR to introduce our own meta-path managing it is something we will have to move to long term.

@Kyle-Verhoog Kyle-Verhoog added this to the 0.18.0 milestone Dec 3, 2018
@brettlangdon
Copy link
Member

While it is overkill for this PR to introduce our own meta-path managing it is something we will have to move to long term.

I wonder if we shouldn't just do this first? It feels like we are setting ourselves up for failure here.

We might not see an edge case where this is a problem right now, but a customer will definitely find one for us... especially if we already can see this being an issue in the future.

@Kyle-Verhoog
Copy link
Member Author

I don't think we're quite setting ourselves up for failure since this is quite an edge-case (most modules break or have undefined behaviour when reimported/deleted and imported again). Plus there is the fact that our current patching does not account for reimports so the functionality is the same from the user's perspective (they would have to call ddtrace.patch(module=True) again after deleting the module.

With that said, it's something we're going to change out eventually so it's worth considering not introducing it in the first place. I will investigate bringing in the core pieces of wrapt or importhook with the modification that give us what we want from the start.

@brettlangdon
Copy link
Member

Fair enough. My only concern here is if we know what we probably need, if we hold off on it, will we ever implement it?

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

Mostly looks good.

It is probably worth having a test case where we setup a non-dd import hook and mess with importing and install/uninstall to ensure we don't mess with the default behavior of wrapt. Just in case someone is using dd-trace-py along with wrapt.register_post_import_hook

tests/utils/test_install.py Outdated Show resolved Hide resolved
tests/utils/test_install.py Outdated Show resolved Hide resolved
tests/utils/test_install.py Outdated Show resolved Hide resolved
tests/utils/test_install.py Outdated Show resolved Hide resolved
ddtrace/utils/install.py Outdated Show resolved Hide resolved
ddtrace/utils/install.py Outdated Show resolved Hide resolved
ddtrace/utils/hook.py Outdated Show resolved Hide resolved
ddtrace/utils/hook.py Show resolved Hide resolved
tests/utils/test_import_hook.py Outdated Show resolved Hide resolved
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

a few minor comments

ddtrace/utils/hook.py Show resolved Hide resolved
ddtrace/utils/hook.py Outdated Show resolved Hide resolved
ddtrace/utils/hook.py Show resolved Hide resolved
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

do we need a test case for installing a module hook twice?

we should probably try to deduplicate functions being registered for a module.

@brettlangdon brettlangdon modified the milestones: 0.18.0, 0.19.0 Dec 7, 2018
@brettlangdon brettlangdon self-assigned this Dec 12, 2018
@brettlangdon
Copy link
Member

@Kyle-Verhoog I'll take it from here

@brettlangdon brettlangdon changed the base branch from 0.18-dev to 0.19-dev December 12, 2018 21:12
@Kyle-Verhoog
Copy link
Member Author

@brettlangdon #769 took out the hook logic from this PR. This PR is now focused on adding the API in which integrations will use the import hook system.

@brettlangdon brettlangdon modified the milestones: 0.19.0, 0.20.0 Dec 21, 2018
@brettlangdon brettlangdon removed this from the 0.20.0 milestone Jan 7, 2019
@Kyle-Verhoog Kyle-Verhoog deleted the kyle-verhoog/poi-install branch November 11, 2020 02:43
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.

2 participants