Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix the auto instrumentation command #567
Fix the auto instrumentation command #567
Changes from all commits
0523233
99600e2
4adf1bb
80d7167
b18b3cf
038a1de
82ac06c
fc9a106
d8d4149
b0f0a84
e3bd21b
791eda8
1e43a14
f8a654b
65d9d6f
2cbeeff
45542ab
860cfe6
91a95ea
91adb43
d91b90e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If I understand this, the goal is to load all available instrumentations if the
auto_instrumentation
package is on the path, which should only be true if the user calledauto_instrumentation/auto_instrumentation.py:run
?Why did you decide to use
sitecustomize
to do this instead of leaving this inrun
? Loading the instrumentations as an import effect seems dangerous, but I don't know if this is conventional forsitecustomize
.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 did a little research here. Looks like this is how ddtrace does it too: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/bootstrap/sitecustomize.py
The main issue that this PR is fixing is the loading + activating of the instrumentations before anything in the downstream script is invoked. In commands like flask_run, you have no control over the actual code, so you can't programatically insert the instrumentation activation into the script itself.
in this scenario the "run" entry point is never invoked by the script that is being execl'd, so auto_instrumentation.py:run will never actually execute in the new python process.
There's very few places that you can run arbitary python code before the script itself starts to run. One of those that I'm aware of is sitecustomize. I'm having trouble thinking of any other place to inject startup code that is guaranteed to run before the script that you're trying to invoke.
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.
Yes, exactly. As @toumorokoshi says, this has been changed to support the frameworks or libraries that use launcher executables (thus requiring
execl
) and are executed in a new Python process.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.
This change is similar to how
ddtrace-run
works to add the directory where thesitecustomize.py
@toumorokoshi just linked is located to thePYTHONPATH
before callingexecl
:https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/commands/ddtrace_run.py
I tried this PR out with the example auto-instrumentation but using
uwsgi
to load the app and it worked as expected: