Add support for programmatic instrumentation by ocelotl · Pull Request #579 · open-telemetry/opentelemetry-python

ocelotl · 2020-04-14T21:11:14Z

Fixes #554

This makes it possible to call the instrument method with arguments that make programmatic instrumentation possible.

This also makes the children of BaseInstrumentors to be singletons. In this way regardless of how many times the programmatic instrumentation or uninstrumentation methods are called they will only be executed once.

mauriciovasquezbernal · 2020-04-15T14:18:03Z

I think BaseInstrumentor is becoming too complicated for the value it adds. I strongly feels we could achieve the same without it.

BaseInstrumentor has currently two goals, to force developers of an instrumented library to implement an interface, _automatic_instrument and _automatic_uninstrument in terms of this PR glossary, and to avoid instrumenting twice, i.e, to guarantee idempotence.

That interface is important because it's called by the autoinstrumenation command here

opentelemetry-python/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/auto_instrumentation.py

Line 30 in b62c233

entry_point.load()().instrument() # type: ignore

I think we could implement the instrument and uninstrument functions directly at the module level. If we are worried about the signature not being the correct one, we can do a runtime check for it. The developer of the library should guarantee idempotence, it is simple to implement and will give the developer full control.

I also think we should just call them instrument() and uninstrument(), both functions could be called by the autoinstrumentation command and by the user too. The instrument function has some limitations, for instance it could not work if it is called after some module is imported or so on, we could make it clear in the documentation and we could also add some warnings to inform the user about it.

About programmatic instrumentation, I think it is a concept, having a function with that name is not clear at all. In the specific case of Flask we could make InstrumentedFlask public to be use as:

from opentelemetry.ext.flask import InstrumentedFlask
app = InstrumentedFlask(__name__)
# app is now instrumented

To summarize my proposal:

Get rid of BaseInstrumentor.
Implement instrument, uninstrument functions in the library modules. Developer should take care of idempotence. In the instrument case some warnings should be printed if it is not possible to perform the instrumentation, for instance if the module was already imported.
To avoid having a "programmatic instrumentation" function. Each framework could provide different mechanisms when the user wants a more granular control of instrumentation, for instance to provide an instrumented class (Flask), to provide a function to disable instrumentation in a Session object (requests) and so on.

I'd love to get more feedback on this.

ocelotl · 2020-04-15T16:57:01Z

I think BaseInstrumentor is becoming too complicated for the value it adds. I strongly feels we could achieve the same without it.

BaseInstrumentor has currently two goals, to force developers of an instrumented library to implement an interface, _automatic_instrument and _automatic_uninstrument in terms of this PR glossary, and to avoid instrumenting twice, i.e, to guarantee idempotence.

That interface is important because it's called by the autoinstrumenation command here

opentelemetry-python/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/auto_instrumentation.py

Line 30 in b62c233

entry_point.load()().instrument() # type: ignore

I think we could implement the instrument and uninstrument functions directly at the module level. If we are worried about the signature not being the correct one, we can do a runtime check for it. The developer of the library should guarantee idempotence, it is simple to implement and will give the developer full control.

I also think we should just call them instrument() and uninstrument(), both functions could be called by the autoinstrumentation command and by the user too. The instrument function has some limitations, for instance it could not work if it is called after some module is imported or so on, we could make it clear in the documentation and we could also add some warnings to inform the user about it.

About programmatic instrumentation, I think it is a concept, having a function with that name is not clear at all. In the specific case of Flask we could make InstrumentedFlask public to be use as:
from opentelemetry.ext.flask import InstrumentedFlask
app = InstrumentedFlask(__name__)
# app is now instrumented
To summarize my proposal:

Get rid of BaseInstrumentor.

Implement instrument, uninstrument functions in the library modules. Developer should take care of idempotence. In the instrument case some warnings should be printed if it is not possible to perform the instrumentation, for instance if the module was already imported.

We should not get rid of BaseInstrumentor, because if we do, we'll end up implementing manually the very same checks, accurate error reporting and idempotence that this ABC does for us already.

To avoid having a "programmatic instrumentation" function. Each framework could provide different mechanisms when the user wants a more granular control of instrumentation, for instance to provide an instrumented class (Flask), to provide a function to disable instrumentation in a Session object (requests) and so on.

Just to be clear, I am not suggesting that we have a programmatic instrumentation function for every instrumentation (that is why there is no programmatic_instrument method in BaseInstrumentor). This PR only adds some convenience functionality (the BaseInstrumentor.protect_instrument) decorator that may be used for programmatic instrumentation if the developer finds it convenient to implement instrumentation in this way.

I'd love to get more feedback on this.

codeboten · 2020-04-16T22:57:33Z

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py

-    def _instrument(self):
-        self._original_flask = flask.Flask
+    def _instrument(
+        self, flask_class=None


any reason not to use kwargs here? and in _uninstrument?

yes, only because kwargs is not used in _instrument nor in _uninstrument. I mean, there is no reference to kwargs in the code of _instrument or _uninstrument

but you could pull flask_class from the kwargs instead of overriding the method signature correct? any reason not to go that route?

we could do that but it may not play nice with the documentation of the optional flask_class argument (not that there is any here, though 😅)

codeboten

Only question regarding overriding the interface with different arguments. Otherwise this is a good improvement!

mauriciovasquezbernal

Even if I don't completely agree with having an ABC and so on, I think the changes to BaseInstrumentor in the last iteration are good. Making it a singleton and allowing arbitrary arguments helps.

However I think the changes to the Flask are not that good, the proposed way for users to enable instrumentation is rather complicated. I still think we should have instrument and uninstrument methods that are simple to use (just a single call, few parameters and not return value) that are used by both, the automatic instrumentation command and the users. I'm aware that in the users case this function could not work in all the cases, for instance if you call it too late in the code after importing some modules, but I think that with correct documentation and warnings they would be very useful.

For the programmatic case, we could use a much simpler approach, just expose and let the user interact with InstrumentedFlask.

from opentelemetry.ext.flask import InstrumentedFlask as Flask
app = Flask(...) # app will be instrumented

or

from flask import Flask
from opentelemetry.ext.flask import InstrumentedFlask
app1 = Flask(...) # this won't be instrument
app2 = InstrumentedFlask(...) # this will be

The instrument method should be used for automatic instrumentation and when the user want's to instrument everything in that framework, the later with some restrictions, like enforcing a proper order in the imports.

from opentelemetry.ext.flask import FlaskInstrumentor
FlaskInstrumentor().instrument()

from flask import Flask # needed to do after

app = Flask(...) # instrumented

The following case won't work, but we could print a warning to the user that the flask module is already imported and it might not work.

from flask import Flask
from opentelemetry.ext.flask import FlaskInstrumentor
FlaskInstrumentor().instrument()

app = Flask(...) # uninstrumented, reference to Flask was taken before instrumenting

mauriciovasquezbernal · 2020-04-17T15:05:42Z

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py

    from flask import Flask
+    from opentelemetry.ext.flask import FlaskInstrumentor
+
+    Flask = FlaskInstrumentor().instrument(flask_class=Flask)


This looks too complicated to me. The instrument method does not do any instrumentation but just returns an instrumented version of the Flask class that the user has to assign to the local Flask reference. Btw, the current uninstrument will do nothing after that call, somehow the user will have to update the Flask reference to the original flask.Flask class.

This leads me to believe we might need to patch the flask object instead of the class itself (like a middleware)? This way we will have direct control over what is instrumented or not. What if the user wants only some flask instances to be instrumented and others not to be? And same goes for uninstrumenting as well.

mauriciovasquezbernal · 2020-04-17T15:07:45Z

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py

+    def _instrument(
+        self, flask_class=None
+    ):  # pylint: disable=arguments-differ
+        if flask_class is not None:


This if makes this function to behave very different. Related to my long comment above, the case where you pass flask_class is not doing any instrumentation but just returning _InstrumentedFlask.

I think we can find another way of using _instrument here. Maybe pass the Flask app as was done before? That would be fine. I still would like to have instrumentation depend on it being done before something is imported only as a very last option because of how fragile and hard to debug this approach is (it also breaks PEP8).

What is the reason to implement extra functionalities in _instrument()?, I think the scope of this method is to be called by the autoinstrument command and by users that want to enable instrumentation in all that framework. If we start adding extra functionalities and special cases we'll end up with a lot of different _instrument() what must be used in a specific way to work.

What do you think about exposing InstrumentedFlask to the user for the "programmatic instrumentation" case as I exposed before?

lzchen · 2020-04-19T18:15:46Z

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py

    from flask import Flask
+    from opentelemetry.ext.flask import FlaskInstrumentor
+
+    Flask = FlaskInstrumentor().instrument(flask_class=Flask)


Any naming conflicts in this example due to Flask imported library and instantiated variable?

That's actually the purpose of it, to overwrite the imported Flask to make it instrumented.

lzchen · 2020-04-19T18:17:26Z

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py


-    def _uninstrument(self):
-        flask.Flask = self._original_flask
+        return None


Why is None returned if the original class is not supplied? Shouldn't InstrumentedFlask be returned?

lzchen · 2020-04-19T18:27:19Z

I'm not able to find the automatic_instrument and automatic_uninstrument methods. Also the protect_instrument decorator is not there either? It seems like the description does not match the changes or some changes have been made to deviate from the original proposal?

ocelotl · 2020-04-19T20:56:21Z

I'm not able to find the automatic_instrument and automatic_uninstrument methods. Also the protect_instrument decorator is not there either? It seems like the description does not match the changes or some changes have been made to deviate from the original proposal?

Thanks for pointing that out, I have updated the comment.

mauriciovasquezbernal · 2020-04-20T15:29:41Z

I'd propose to split this PR up. I think we could easily agree on the changes to the instrumentor (make it a singleton and add kwargs). About the Flask I think there is still some discussion to do, so better to move ahead with the part we agree now.

ocelotl · 2020-04-20T18:12:07Z

I'd propose to split this PR up. I think we could easily agree on the changes to the instrumentor (make it a singleton and add kwargs). About the Flask I think there is still some discussion to do, so better to move ahead with the part we agree now.

Good idea, splitting...

ocelotl · 2020-04-20T19:12:04Z

Ok, this has been split, let's move the conversation of Flask changes to #601.

mauriciovasquezbernal

A couple of non blocking comments.

opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py

mauriciovasquezbernal · 2020-04-20T19:58:09Z

opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py


-        _LOG.warning("Attempting to instrument while already instrumented")
+        _LOG.warning(
+            "Attempting to automatically instrument while already instrumented"


What about removing "auomatically" and keeping the message as it was?

+1, since this can be called both from auto and programmatic instrumentation, the original warning was clearer

codeboten

Couple of minor comments, otherwise this makes the interface more usable. thanks for separating this from the flask changes, it makes the review much simpler

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py

codeboten · 2020-04-21T16:42:21Z

opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py


-        _LOG.warning("Attempting to instrument while already instrumented")
+        _LOG.warning(
+            "Attempting to automatically instrument while already instrumented"


+1, since this can be called both from auto and programmatic instrumentation, the original warning was clearer

codeboten · 2020-04-21T16:42:53Z

opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py


-        _LOG.warning("Attempting to uninstrument while already uninstrumented")
+        _LOG.warning(
+            "Attempting to automatically uninstrument while already"


same comment about the warning message

Fixes open-telemetry#554

lzchen

LGTM

…try#579) Fixes open-telemetry#554 This makes it possible to call the instrument method with arguments that make programmatic instrumentation possible. This also makes the children of BaseInstrumentors to be singletons. In this way regardless of how many times the programmatic instrumentation or uninstrumentation methods are called they will only be executed once.

closes open-telemetry#579 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

ocelotl added doc Documentation-related ext instrumentation Related to the instrumentation of third party libraries or frameworks labels Apr 14, 2020

ocelotl requested a review from a team April 14, 2020 21:11

ocelotl self-assigned this Apr 14, 2020

ocelotl mentioned this pull request Apr 14, 2020

Add some support for programmatic instrumentation #554

Closed

ocelotl force-pushed the issue_554 branch from c97076b to e60a814 Compare April 15, 2020 16:40

codeboten reviewed Apr 16, 2020

View reviewed changes

c24t added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Apr 16, 2020

codeboten mentioned this pull request Apr 17, 2020

Porting sqlalchemy instrumentation from contrib repo #591

Merged

mauriciovasquezbernal reviewed Apr 17, 2020

View reviewed changes

codeboten mentioned this pull request Apr 17, 2020

Porting redis instrumentation from contrib repo #595

Merged

lzchen reviewed Apr 19, 2020

View reviewed changes

ocelotl changed the title ~~Add some support for programmatic instrumentation~~ Add support for programmatic instrumentation Apr 19, 2020

ocelotl mentioned this pull request Apr 20, 2020

Move Flask instrumentation enhancements in a separate PR #599

Closed

ocelotl force-pushed the issue_554 branch from 0a5db5e to 69d8616 Compare April 20, 2020 18:28

mauriciovasquezbernal approved these changes Apr 20, 2020

View reviewed changes

codeboten approved these changes Apr 21, 2020

View reviewed changes

ocelotl added 3 commits April 21, 2020 16:14

Add support for programmatic instrumentation

eb634a6

Fixes open-telemetry#554

Fix isort issues

dd48985

Refactor API

5de387e

ocelotl added 7 commits April 21, 2020 16:14

Fix API

985dc94

Fix automatic instrumentation

b5c627c

Fix arguments

71062b8

Adding lint fixes

ff233b8

Revert Flask changes

83570db

Fix lint

928e3d5

Remove flask changes

f349b56

ocelotl force-pushed the issue_554 branch from 499574b to f349b56 Compare April 21, 2020 22:27

ocelotl added 2 commits April 21, 2020 16:46

Add several fixes

02c390b

Fix lint

6216580

lzchen approved these changes Apr 21, 2020

View reviewed changes

Merge branch 'master' into issue_554

0e35477

toumorokoshi merged commit 305c1f4 into open-telemetry:master Apr 22, 2020

This was referenced Apr 27, 2020

Add Flask Instrumentation fixes #601

Merged

Fix autoinstrumentation docs #544

Merged

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

fix(plugin-http): http.url attribute (open-telemetry#580)

06d21e3

closes open-telemetry#579 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

Conversation

ocelotl commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mauriciovasquezbernal commented Apr 15, 2020

Uh oh!

ocelotl commented Apr 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocelotl Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

mauriciovasquezbernal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mauriciovasquezbernal Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen commented Apr 19, 2020

Uh oh!

ocelotl commented Apr 19, 2020

Uh oh!

mauriciovasquezbernal commented Apr 20, 2020

Uh oh!

ocelotl commented Apr 20, 2020

Uh oh!

ocelotl commented Apr 20, 2020

Uh oh!

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ocelotl commented Apr 14, 2020 •

edited

Loading

ocelotl Apr 16, 2020 •

edited

Loading

mauriciovasquezbernal left a comment •

edited

Loading

mauriciovasquezbernal Apr 20, 2020 •

edited

Loading