Add Flask Instrumentation fixes

[ocelotl]

Fixes #599

This is just a separate PR to keep the Flask changes from blocking the instrumentation enhancements.

[lzchen]

Reopening up the discussion.
The changes here are good. It address @mauriciovasquezbernal comments on your previous PR. I'm still wondering if patching the class instead of the app itself is the way to go.

[mauriciovasquezbernal]

I think it is actually not addressing my comments yet.
I have some concerns about they way instrument is implemented here and how it should be used #579 (comment).

I exposed an alternative to this here #579 (review).

[ocelotl]

I think it is actually not addressing my comments yet.
I have some concerns about they way instrument is implemented here and how it should be used #579 (comment).

I exposed an alternative to this here #579 (review).

Adding @mauriciovasquezbernal comments from past reviews:

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]

Overall looks good to me.

I have some concerns about the conditionals in the (un)-instrument_app functions. I'll do a more detailed review starting next week.

[ocelotl]

Addressed all your comments, @mauriciovasquezbernal, please review again when you have a moment. @lzchen, these changes address your previous concern about doing instrumentation by replacing the Flask class, please take a look.

Thanks!

[lzchen]

This looks good. I'm curious as to whether we can merge the two methods of auto-instrumentation and programmatic instrumentation but still have it patch an Flask app instance instead of the class.
Similar to what we are doing in SQLAlchemy.

[ocelotl]

This looks good. I'm curious as to whether we can merge the two methods of auto-instrumentation and programmatic instrumentation but still have it patch an Flask app instance instead of the class.
Similar to what we are doing in SQLAlchemy.

It is possible but we preferred not to do so. We prefer not to have a method whose actions are different depending on the optional attributes it gets, we rather use **kwargs in _instrument only to pass configuration attributes that will override the configuration options read from the Configuration object.

[lzchen]

LGTM

[mauriciovasquezbernal]

I like the changes in FlaskInstrumentor.
I think the tests could be improved a bit, specially removing code duplication and simplifying the programmatic / automatic tests to avoid checking things that are already covered in other cases.

[mauriciovasquezbernal]

btw, would you mind moving the entry point definitions to setup.cfg file to keep things uniform with other instrumentations?

[ocelotl]

All your comments have been addressed, @mauriciovasquezbernal, thanks for the review!

[mauriciovasquezbernal]

Some minor nits. LGTM now, thanks for handling the feedback.

[mauriciovasquezbernal]

Is it worth to check that calling (un)-instrument twice doesn't cause any issue?

[mauriciovasquezbernal]

Not related to this PR, but is this example missing app.run(), how am I supposed to run it?

[toumorokoshi]

have you considered using the instrumented flag to turn off and on behavior, like the pymongo instrumentation?

Or you could remove by value by comparing the id of the function:

for k, v in before_request_funcs.items():
    if v is _before_request:
        del before_request_funcs[k]

Although the latter feels more error prone as you are relying on the data structure remaining the same across different versions of flask. The checking of the _is_instrumented value feels more robust here.

[toumorokoshi]

I think one small edge case around making sure uninstrument is robust, but all looks good otherwise!

[toumorokoshi]

The issue I raised currently won't affect anyone, since the instrumentation only instruments apps.