-
Notifications
You must be signed in to change notification settings - Fork 651
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
Add an autoinstrumentation mechanism and an instrumentor for Flask #327
Conversation
This PR includes an autoinstrumentation mechanism and a Flask instrumentor (an instrumentor is a class that implements the It works like this:
|
1b300de
to
577d5c6
Compare
Please leave your comments 👍 |
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.
Nice! this is a big step, thanks for focusing on this.
My two cents (and I think we need some more opinions here) are:
Enable programmatic control of patching
I'm not a huge fan of the patch executable (like auto_agent) myself, because I think that inverts control away from the application author and toward some other entity. These are hard to chain (e.g. if something else wants to go modify the import and startup logic), and hard to customize.
Looking at dd-agent-py, which we're taking inspiration from, customization of the dd agent happens via several environment variables. https://docs.datadoghq.com/tracing/setup/python/#environment-variable. I've personally found environment variables (in essence a dictionary of key-value pairs) fairly limiting, as it doesn't enable things like more complex types or programmatic control.
Also I don't know how robust or flexible sitecustomize is as an interface. Will it impact it's use with other systems that modify sitecustomize, such as zc.buildout?
Patching Interface
I think the patch interface looks good. I feel like some extensions will probably want to expose more primitives (e.g. flasks' instrument_app) to allow finer control of the extensions themselves.
Overall I think this achieves standardization of patching for auto-instrumentation, which is great, as well as it's utilization of setuptools entry points which is best practice.
ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py
Outdated
Show resolved
Hide resolved
@@ -36,7 +36,9 @@ def setspanattr(key, value): | |||
|
|||
self.span.set_attribute = setspanattr | |||
|
|||
self.app = Flask(__name__) | |||
FlaskPatcher().patch() |
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.
will this cause issues with other unit tests that might want the raw Flask, as it will permanently Flask with an instrumented variety?
We may also use Flask apps to bring up example test servers, or we may in the future.
There's a technique used to reload modules we could use during teardown to ensure it gets set back: f328909
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.
It may cause issues, but I think it is necessary for patching purposes. I can investigate a way that the patching is done in a way that only happens for the testing purposes of the patching agent. Also, I can think about a unpatching method that gets called automatically when patching is no longer needed.
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.
Does the reload
function not work for your purposes? I believe it should reset everything you've patched: https://docs.python.org/3/library/importlib.html#importlib.reload
Although maybe a best-effort unpatch API is also a good idea as it'll be hard for people to be aware of the new patches that were added from version to version, so they may find their previous custom unpatching function not unpatching new patches added by a newer version of the auto-instrumentation.
opentelemetry-api/setup.py
Outdated
@@ -40,6 +40,14 @@ | |||
"Programming Language :: Python :: 3.6", | |||
"Programming Language :: Python :: 3.7", | |||
], | |||
entry_points={ | |||
"console_scripts": [ | |||
"auto_agent = opentelemetry.commands.auto_agent:run" |
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 "agent" is probably the wrong word. how about Instrument? auto_instrument would also match the opentelemetry spec name (autoinstrumentation) that we're trying to support here.
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.
also we should probably prefix any commands with opentelemetry, as console_scripts is a global namespace.
so "opentelemetry-auto-instrument" as a final suggestion.
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, I'll have to read the documentation on console_scripts
thoroughly, I am under the impression it must have this name for our purposes.
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.
Never mind, I understand now what you mean.
|
||
_LOG = getLogger(__file__) | ||
|
||
for entry_point in iter_entry_points("opentelemetry_patcher"): |
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.
can this be also be available as a public method? I know we're looking at ddtrace for inspiration, and that codebase includes a way to patch things via patch_all.
Thanks for your comments, @toumorokoshi. 👍 Regarding the patch executable, I think there is value in having a fire-and-forget auto instrumentation executable that allows the user to just take non-instrumented code and make it instrumented automatically. This seems useful for situations when quick instrumentation is just needed an not much else needs consideration. That being said, I see our users in need of configuration soon after they get the simplest of automatic instrumentations done. Of course, this means supporting both scenarios. I'm not much a fan of environment variables myself, they are vulnerable to them being lost after a reboot or a new shell being started and they require usually long prefixes to keep them separated from each other. I prefer configuration files myself, since it is possible to keep them in version control. That being said, other people prefer command line arguments because they make it very easy to adjust stuff when making many runs of a command 🤷♂️. I'll look into this and will propose a solution. You raise a valid point when you mention the lack of a programmatic approach to auto instrumentation. This looks like a job for a well defined and documented API that can be used to create customized auto instrumentation tools. Also, will look into this and will propose a solution. Please let me know if you have more design-level ideas for me to consider. Thanks! |
d55604c
to
a81cbc4
Compare
Codecov Report
@@ Coverage Diff @@
## master #327 +/- ##
==========================================
+ Coverage 89.50% 89.64% +0.14%
==========================================
Files 43 43
Lines 2229 2240 +11
Branches 248 248
==========================================
+ Hits 1995 2008 +13
Misses 159 159
+ Partials 75 73 -2
Continue to review full report at Codecov.
|
I have fixed the last issues I had in this PR, @toumorokoshi, please let me know what you think. |
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.
Requesting changes because:
- Please put everything new, maybe except BasePatcher, inside a new distribution, not in API.
- Do not make unrelated style changes here, e.g. https://github.com/open-telemetry/opentelemetry-python/pull/327/files#r362112308 and related.
ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py
Outdated
Show resolved
Hide resolved
opentelemetry-api/src/opentelemetry/auto_instrument/auto_instrument.py
Outdated
Show resolved
Hide resolved
opentelemetry-api/src/opentelemetry/auto_instrument/auto_instrument.py
Outdated
Show resolved
Hide resolved
opentelemetry-api/src/opentelemetry/auto_instrument/sitecustomize.py
Outdated
Show resolved
Hide resolved
opentelemetry-api/src/opentelemetry/auto_instrument/sitecustomize.py
Outdated
Show resolved
Hide resolved
Something like the BasePatcher interface is much-needed! 👍 |
Thank you for your review, @Oberon00! |
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.
Requested a few changes, but this is looking pretty good! Excited to see this functionality in the library.
opentelemetry-api/setup.py
Outdated
@@ -40,6 +40,15 @@ | |||
"Programming Language :: Python :: 3.6", | |||
"Programming Language :: Python :: 3.7", | |||
], | |||
entry_points={ | |||
"console_scripts": [ |
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.
+1 on moving the console script to a separate package. Maybe it ends up moving into the https://github.com/open-telemetry/opentelemetry-auto-instr-python/ repo, just a thought
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.
Thanks for addressing my feedback! LGTM
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.
Overall looks good to me.
I think the documentation needs a little bit of rework, specially because it has been changed a lot after this PR was opened.
About the example, I think the right place for that is docs/examples
.
...elemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/auto_instrumentation.py
Outdated
Show resolved
Hide resolved
opentelemetry-auto-instrumentation/example/publisher_instrumented.py
Outdated
Show resolved
Hide resolved
opentelemetry-auto-instrumentation/example/publisher_instrumented.py
Outdated
Show resolved
Hide resolved
|
||
The code in ``program.py`` needs to use one of the packages for which there is | ||
an OpenTelemetry extension. For a list of the available extensions please check | ||
`here <https://opentelemetry-python.readthedocsio/>`_. |
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.
It's better to use an "internal" target for this link. So if we decide to host our documentation somewhere else the generated link is not broken.
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.
Makes sense, but what do you mean with "internal"? Can you provide an example, please?
Co-Authored-By: Mauricio Vásquez <[email protected]>
Co-Authored-By: Mauricio Vásquez <[email protected]>
Co-Authored-By: Mauricio Vásquez <[email protected]>
Co-Authored-By: Mauricio Vásquez <[email protected]>
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 there's still a lot of additional work that will have to go into auto-instrumentation in general, but I think the patterns you've laid our here look really good, and put us in a great position to actually build this out.
Also great idea to try to find a pattern that allows extensions to be leveraged for auto-instrumentation as well. I see that as as huge win for maintainability in the future.
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.
LGTM! Thanks for pushing through this long review cycle @ocelotl.
My only remaining (and nit-level) comment is that opentelemetry_auto_instrumentation_instrumentor
is a mouthfull, opentelemetry_instrumentor
seems to describe the same thing and is much shorter.
@@ -0,0 +1,28 @@ | |||
# Copyright The OpenTelemetry Authors |
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.
FYI it looks like this isn't included in the built docs.
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.
Sorry, which docs are the built docs?
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.
The ones you get running tox -e docs
or make html
in the docs dir.
👍 Thanks for the review! I have shortened the entry point name |
Fixes #300