Add Flask integration based on WSGI ext #206

Oberon00 · 2019-10-08T16:40:39Z

The flask integration has (only) two advantages over the plain WSGI middleware approach:

It can use the endpoint as span name (which is lower cardinality than the route; cf Do not recommend URL path as acceptable span name opentelemetry-specification#270)
It can set the http.route attribute.

In addition, it also has an easier syntax to enable (you don't have to know about Flask.wsgi_app).

(above summary copied from #206 (comment))

I did not add flask to the dependencies (except the test extra) because it seems kinda useless: To use the extension, you have to pass a Flask object which makes it obvious that you need Flask. If you think that's too odd, I will split the flask integration into new distribution that depends on the WSGI integration.

Oberon00 · 2019-10-08T16:42:34Z

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

+    span.set_attribute("http.url", url)
+
+
+def add_response_attributes(


Since this is now a public API (no underscore & docstring), I thought it is better to add the response_haders argument, since we might very well need it in the future, should we ever want to capture certain response headers as span attributes.

a-feld · 2019-10-08T18:15:16Z

Why is a Flask integration needed in addition to the base WSGI integration? Flask supports WSGI middlewares through the mechanism described in the example document.

Oberon00 · 2019-10-08T18:29:48Z

The flask integration has (only) two advantages:

It can use the endpoint as span name (which is lower cardinality than the route; cf Do not recommend URL path as acceptable span name opentelemetry-specification#270)
It can set the http.route attribute.

In addition, it also has an easier syntax to enable (you don't have to know about Flask.wsgi_app).

I'll copy that to the PR description, as it should have been there from the beginning. Thanks for asking that important question, @a-feld 👍

a-feld · 2019-10-08T18:43:18Z

That makes sense, thanks for clarifying @Oberon00 ! Those bullet points definitely seem worth pursuing! I wonder if there's any way to layer / compose those additions so that we maintain separation between the WSGI middleware and the flask specific features?

One approach I can think of is to use the OpenTelemetryMiddlware on the wsgi_app and then use the before_request and after_request hooks to name the span appropriately (using update_name) / add the route attributes while relying purely on the OpenTelemetryMiddlware to create the initial span.

We could continue the approach of inserting the WSGI middleware + before_request / after_request hooks with a wrap_flask function!

I also agree with your assessment that it makes sense to split the flask integration into its own package!

Oberon00 · 2019-10-08T19:03:42Z

But using update_name is "highly discouraged" and in fact the prototype Dynatrace-prototype-API based implementation that caused me to write this can't handle update_name properly (the spanName metadata would be updated but the spans would be grouped according to the initial span name).

toumorokoshi · 2019-10-11T03:14:30Z

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

+            host += ":" + port
+
+    # NOTE: Nonstandard (but see
+    # https://github.com/open-telemetry/opentelemetry-specification/pull/263)


since this will likely be merged anyway (3 approvals), maybe just remove the comment?

Not yet, according to that spec PR the http.host must only be set if there is an http.host header.

toumorokoshi · 2019-10-11T03:19:18Z

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

+            # //foo which looks like scheme-relative but isn't.
+            url = environ["wsgi.url_scheme"] + "://" + host + url
+        elif not url.startswith(environ["wsgi.url_scheme"] + ":"):
+            # Something fishy is in RAW_URL. Let's fall back to request_uri()


when does this happen? Just out of curiosity. Sounds like a common occurence.

Why do you think this is common? Personally, I don't really know how often that occurs in practice, but I imagine a buggy client could send a line like GET foobar HTTP/1.0 instead of GET /foobar HTTP/1.0.

ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/flask_util.py

toumorokoshi · 2019-10-11T04:05:53Z

ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/flask_util.py

+    otel_wsgi.add_request_attributes(span, environ)
+    if flask_request.url_rule:
+        # For 404 that result from no route found, etc, we don't have a url_rule.
+        span.set_attribute("http.route", flask_request.url_rule.rule)


should this default to the method then? as recommended in the opentelemetry-specification#270?

open-telemetry/opentelemetry-specification#270 only talks about the span name, the http.route attribute is unrelated.

ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/flask_util.py

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask_util/version.py

Oberon00 · 2019-10-23T12:05:03Z

Anyone care to review this?

c24t

The middleware changes LGTM, but I've got some reservations about the package structure changes.

I'd be interested to get another pass from @a-feld after your last round of changes.

c24t · 2019-10-24T23:40:16Z

ext/opentelemetry-ext-flask/README.rst

+    from opentelemetry.ext.flask_util import instrument_app
+
+    app = Flask(__name__)
+    instrument_app(app)  # This is where the magic happens. ✨


You think I should tone down the emojis? 😁

ext/opentelemetry-ext-flask/README.rst

c24t · 2019-10-25T01:14:15Z

ext/opentelemetry-ext-flask/setup.cfg

+    opentelemetry-ext-wsgi
+
+[options.extras_require]
+test = flask~=1.0


That's interesting, do you think we should specify all our test dependencies this way?

This is a bit weird since the tests still wouldn't be included in the distribution if you do pip install 'opentelemetry-ext-flask[test]', this is only useful for editable installs.

Hmm, I'm not sure. Since we now have separate coverage & test targets, in the interest of not growing the toxfile too much, I think I'll keep it here.

c24t · 2019-10-25T01:22:08Z

ext/test-util/README.txt

@@ -0,0 +1,3 @@
+This (non-installable) directory should be added to the PYTHONPATH of any unit tests that require it.


I think the cure is worse than the disease here. It may be inevitable that we'll have enough shared code between tests that we'll have to do something like this, but for now duplicating WsgiTestBase seems less bad than forcing users to edit the PYTHONPATH.

I don't think we should expect that people will only run the tests via tox. Doing e.g. pytest ext/opentelemetry-ext-flask/tests/test_flask_integration.py is much faster while developing.

I changed this to an ordinary installable package, so no manual PYTHONPATH manipulations required anymore.

tox.ini

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

Conflicts: tox.ini

Pylint seems to grok it now.

codecov-io · 2019-11-13T12:11:29Z

Codecov Report

Merging #206 into master will increase coverage by 0.01%.
The diff coverage is 91.11%.

@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
+ Coverage   85.65%   85.66%   +0.01%     
==========================================
  Files          33       33              
  Lines        1610     1612       +2     
  Branches      180      180              
==========================================
+ Hits         1379     1381       +2     
  Misses        185      185              
  Partials       46       46

Impacted Files	Coverage Δ
...app/src/opentelemetry_example_app/flask_example.py	`100% <100%> (ø)`	⬆️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py	`90.9% <90.69%> (+0.24%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb3e715...b20aea5. Read the comment docs.

Oberon00 · 2019-11-13T12:41:29Z

@open-telemetry/python-approvers I think this is now ready for another review, I hope I could address all issues satisfyingly.

codeboten

Looks good overall. Only blocking comment is the change to the example in the README

ext/opentelemetry-ext-flask/README.rst

ext/opentelemetry-ext-flask/tests/test_flask_integration.py

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

Co-Authored-By: alrex <alrex.boten@gmail.com>

Oberon00 · 2019-11-20T16:31:38Z

Technically, this branch is ready to be merged, even though @toumorokoshi' review is outdated. Personally I think the PR is ready.

Also, I'd like to get this merged before working on updating the WSGI extension to follow open-telemetry/opentelemetry-specification#263 to avoid merge conflicts.

carlosalberto · 2019-11-21T15:35:22Z

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

+        otel_wsgi.get_header_from_environ, environ
+    )
+
+    tracer = trace.tracer()


(non-blocking comment) There was a mention in the past about allowing users to optionally specify their own Tracer instead of relying on the global one. Probably something to consider adding in the future ;)

carlosalberto · 2019-11-21T15:35:51Z

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

+
+    tracer = trace.tracer()
+
+    span = tracer.create_span(


We will be changing create_span() to not exist in the public API, IIRC?

Yes, PR #290 adapted the code, whoever is merged second will have to resolve merge conflicts.

…n-telemetry#206) * fix(basic-tracer): performance.now() is relative from start time * refactor(time): tuples * refactor(span): change name of toHrTime * fix(span): duration default * fix(hrtime): conver to tuple * fix(basic-tracer): nanosecond accuracy * feat(core): move time utils to core * feat(core): add millis and nanos converter * test(basic-tracer): fix span time tests

* fix: performance.now() is relative from start time * fix: grpc plugin tests * fix: import performance

Oberon00 added 4 commits October 8, 2019 13:45

WIP

86d10e4

WIP

d218493

WIP _Context

bb9bc8f

Add flask integration.

4fbb01f

Oberon00 requested review from a-feld, c24t, carlosalberto, lzchen, reyang and toumorokoshi as code owners October 8, 2019 16:40

Oberon00 commented Oct 8, 2019

View reviewed changes

Oberon00 mentioned this pull request Oct 9, 2019

Consider fixing pylint's nameclash bug with namespace packages #210

Closed

toumorokoshi approved these changes Oct 11, 2019

View reviewed changes

Oberon00 added 4 commits October 15, 2019 16:30

Move flask support to new distribution.

798315c

Fix move.

5c242c4

Undo local test changes.

772d999

wrap_flask -> instrument_app, docs.

4715aa6

Oberon00 commented Oct 15, 2019

View reviewed changes

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask_util/version.py Outdated Show resolved Hide resolved

Oberon00 requested review from a-feld, reyang and c24t and removed request for a-feld, reyang and c24t October 15, 2019 16:59

Oberon00 changed the title ~~Add flask integration to wsgi ext~~ Add flask integration based on WSGI ext Oct 23, 2019

c24t reviewed Oct 25, 2019

View reviewed changes

c24t mentioned this pull request Oct 25, 2019

Rename flask_util pkg to flask dynatrace-oss-contrib/opentelemetry-python#1

Closed

c24t reviewed Oct 25, 2019

View reviewed changes

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask_util/__init__.py Outdated Show resolved Hide resolved

Oberon00 added 6 commits November 12, 2019 16:08

Merge branch 'master' into wsgi-flask

ad2ac6c

Conflicts: tox.ini

Fix merge.

c3d4d98

flask integration: Rework build.

6daa087

Fix lint, reformat README.

2357136

Fix test-ext-flask env.

636ffdf

Remove _flask_impl workaround.

2f7938c

Pylint seems to grok it now.

codeboten suggested changes Nov 14, 2019

View reviewed changes

ext/opentelemetry-ext-flask/README.rst Outdated Show resolved Hide resolved

ext/opentelemetry-ext-flask/tests/test_flask_integration.py Outdated Show resolved Hide resolved

ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py Show resolved Hide resolved

Fix import in README.

1a8ee17

Co-Authored-By: alrex <alrex.boten@gmail.com>

codeboten approved these changes Nov 15, 2019

View reviewed changes

Oberon00 mentioned this pull request Nov 15, 2019

Removing create_span from the api #290

Merged

Oberon00 added 2 commits November 15, 2019 17:47

Remove useless comment.

35c6595

Merge branch 'master' into wsgi-flask

b20aea5

carlosalberto reviewed Nov 21, 2019

View reviewed changes

carlosalberto approved these changes Nov 21, 2019

View reviewed changes

Oberon00 mentioned this pull request Nov 21, 2019

Update WSGI & Flask integrations to follow new semantic conventions #299

Merged

Oberon00 added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Nov 21, 2019

c24t merged commit da8b8d9 into open-telemetry:master Nov 23, 2019

c24t removed the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Nov 23, 2019

Oberon00 mentioned this pull request Jan 21, 2020

[WIP] flask: refactoring to build on opentelemetry wsgi #370

Closed

lzchen mentioned this pull request Aug 19, 2020

Move samplers to SDK package #1023

Merged

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

Continuation of open-telemetry#206 (open-telemetry#279)

7fff538

* fix: performance.now() is relative from start time * fix: grpc plugin tests * fix: import performance

lzchen mentioned this pull request Apr 4, 2022

Fix dependencies for test utils #2587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Flask integration based on WSGI ext #206

Add Flask integration based on WSGI ext #206

Oberon00 commented Oct 8, 2019 •

edited

Loading

Oberon00 Oct 8, 2019

a-feld commented Oct 8, 2019

Oberon00 commented Oct 8, 2019 •

edited

Loading

a-feld commented Oct 8, 2019

Oberon00 commented Oct 8, 2019

toumorokoshi Oct 11, 2019

Oberon00 Oct 11, 2019

toumorokoshi Oct 11, 2019

Oberon00 Oct 11, 2019

toumorokoshi Oct 11, 2019

Oberon00 Oct 11, 2019

Oberon00 commented Oct 23, 2019

c24t left a comment

c24t Oct 24, 2019

Oberon00 Nov 13, 2019

c24t Oct 25, 2019

Oberon00 Nov 13, 2019

c24t Oct 25, 2019

Oberon00 Nov 13, 2019

codecov-io commented Nov 13, 2019 •

edited

Loading

Oberon00 commented Nov 13, 2019

codeboten left a comment

Oberon00 commented Nov 20, 2019

carlosalberto Nov 21, 2019

carlosalberto Nov 21, 2019

Oberon00 Nov 21, 2019

		span.set_attribute("http.url", url)


		def add_response_attributes(

		@@ -0,0 +1,3 @@
		This (non-installable) directory should be added to the PYTHONPATH of any unit tests that require it.

Add Flask integration based on WSGI ext #206

Add Flask integration based on WSGI ext #206

Conversation

Oberon00 commented Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

a-feld commented Oct 8, 2019

Oberon00 commented Oct 8, 2019 • edited Loading

a-feld commented Oct 8, 2019

Oberon00 commented Oct 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 commented Oct 23, 2019

c24t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 13, 2019 • edited Loading

Codecov Report

Oberon00 commented Nov 13, 2019

codeboten left a comment

Choose a reason for hiding this comment

Oberon00 commented Nov 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 commented Oct 8, 2019 •

edited

Loading

Oberon00 commented Oct 8, 2019 •

edited

Loading

codecov-io commented Nov 13, 2019 •

edited

Loading