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

Oberon00 · 2019-11-21T15:43:57Z

Updates flask & WSGI integration to follow new semantic conventions for HTTP as of open-telemetry/opentelemetry-specification#263 (see https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md).

The HTTP client (requests) integration will need an update too, and for that the wsgi opentelemetry.ext.wsgi.http_status_to_canonical_code will need to be used by the requests-instrumentation -- either by just adding a dependency (since ext-wsgi is and will stay dependency-free itself, this would technically not be a problem), or by splitting this function into a new package. This will be done in a follow-up PR.

…on#263.

codecov-io · 2019-11-25T10:34:06Z

Codecov Report

Merging #299 into master will decrease coverage by 1.38%.
The diff coverage is 47.36%.

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
- Coverage   85.66%   84.28%   -1.39%     
==========================================
  Files          33       33              
  Lines        1612     1648      +36     
  Branches      180      195      +15     
==========================================
+ Hits         1381     1389       +8     
- Misses        185      204      +19     
- Partials       46       55       +9

Impacted Files	Coverage Δ
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py	`69.02% <47.36%> (-21.89%)`	⬇️

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 693391c...bd0584d. Read the comment docs.

codeboten

Couple of comments, nothing blocking.

codeboten · 2019-11-26T01:01:28Z

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

+    # pylint:disable=too-many-branches,too-many-return-statements
+    if code < 100:
+        return StatusCanonicalCode.UNKNOWN
+    if code <= 299:


Not sure if it would be worth adding a check for 200 at the top since i suspect that's going to be by large the status code we hit. Possibly a premature optimization though

I wouldn't want to guess. 204 No Content is probably also a popular response code to POST requests and the like. I think before that I'd rather do things like the classical _StatusCanonicalCode=StatusCanonicalCode default parameter to avoid accessing globals.

It feels to me that, rather than this range, just creating a dict with hard-coded values would be more performant. Might be worth a benchmark but would make behavior consistent regardless of what codes are common.

You mean a dict with 500 entries?

codeboten · 2019-11-26T01:03:27Z

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

+        "http.server_name": environ["SERVER_NAME"],
+        "http.scheme": environ["wsgi.url_scheme"],
+        "host.port": int(environ["SERVER_PORT"]),
+    }


Is there ever a case where any of these environment variables wouldn't be present? If so using environ.get may be better, unless raising an exception is preferable

According to PEP3333, these are required: https://www.python.org/dev/peps/pep-3333/#environ-variables. I guess there might be nonconforming implementations out there. I have no strong opinion here. Maybe it would be better to come up with a convention of using try:/except: to make sure the integrations don't break applications.

codeboten · 2019-11-26T01:09:45Z

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

+    if flavor.upper().startswith(_HTTP_VERSION_PREFIX):
+        flavor = flavor[len(_HTTP_VERSION_PREFIX) :]
+    if flavor:
+        result["http.flavor"] = flavor


Could use setifnotnone here

Unfortunately not, since I use "" as second argument to get above, in order to unconditionally use upper and startswith. I think there is really no way to save a conditional here.

Conflicts: ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py ext/opentelemetry-ext-flask/tests/test_flask_integration.py ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py

c24t

LGTM, and I think it couldn't hurt to protect against non-PEP3333-compliant envs as in https://github.com/open-telemetry/opentelemetry-python/pull/299/files#r350626307.

c24t · 2019-12-05T17:14:50Z

@Oberon00 let me know when you think this is ready to merge

Oberon00 · 2019-12-05T17:32:57Z

Unless you think I should address the non-PEP3333-compliant environ safety issues first, I think this is ready (but it might cause merge conflicts for the named tracers PR #301).

toumorokoshi

Approved, looks like you had enough, but I guess another one wouldn't hurt :)

toumorokoshi · 2019-12-09T04:42:02Z

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

-                "http.status_code": 404,
-                "http.status_text": "NOT FOUND",
-            },
+            {"http.status_code": 404, "http.status_text": "NOT FOUND"},


random and more general question: is it needed to have "status_text"? feels to me like it adds a lot to the payload and is often identical.

No, the status text is optional. But note that the integration does not insert static text here but actual "output" from the WSGI application. EDIT: See spec: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#common-attributes

toumorokoshi · 2019-12-09T04:44:31Z

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

+    # pylint:disable=too-many-branches,too-many-return-statements
+    if code < 100:
+        return StatusCanonicalCode.UNKNOWN
+    if code <= 299:


It feels to me that, rather than this range, just creating a dict with hard-coded values would be more performant. Might be worth a benchmark but would make behavior consistent regardless of what codes are common.

toumorokoshi · 2019-12-09T04:45:02Z

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

-        elif not url.startswith(environ["wsgi.url_scheme"] + ":"):
-            # Something fishy is in RAW_URL. Let's fall back to request_uri()
-            url = wsgiref_util.request_uri(environ)
+def setifnotnone(dic, key, value):


I think we should follow the underscore naming convention, even for this small method name.

toumorokoshi · 2019-12-09T04:48:40Z

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

    else:
-        url = wsgiref_util.request_uri(environ)
+        result["http.url"] = wsgiref_util.request_uri(environ)


it feels to me like http.url could be decomposed into http.target and http.host or something along those lines: otherwise if I wanted the target, I would have to know and check for url in the case that target did not exist, and perform a split to extract that myself.

Yeah, we could decompose, but I see no benefit in it. Who do you mean with "myself" above? I think the back-end is a better place for such logic, the integrations should send information to the back-end as "raw" as they want as more raw usually means more info.

…etry#299)

Oberon00 added 2 commits November 25, 2019 11:20

ext-{wsgi,flask}: Update for open-telemetry/opentelemetry-specificati…

9c61977

…on#263.

Fix Python 3.4 compatibility.

bd0584d

Oberon00 force-pushed the http-v2 branch from 98b04c6 to bd0584d Compare November 25, 2019 10:22

Oberon00 marked this pull request as ready for review November 25, 2019 10:34

Oberon00 requested a review from a-feld as a code owner November 25, 2019 10:34

Oberon00 requested review from c24t, carlosalberto, lzchen, reyang and toumorokoshi as code owners November 25, 2019 10:34

codeboten approved these changes Nov 26, 2019

View reviewed changes

c24t approved these changes Dec 5, 2019

View reviewed changes

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

toumorokoshi approved these changes Dec 9, 2019

View reviewed changes

c24t merged commit 889baba into open-telemetry:master Dec 9, 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 Dec 11, 2019

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

chore: add OpenTracing shim release date and package info (open-telem…

579850e

…etry#299)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Oberon00 commented Nov 21, 2019 •

edited

Loading

codecov-io commented Nov 25, 2019 •

edited

Loading

codeboten left a comment

codeboten Nov 26, 2019

Oberon00 Nov 26, 2019

toumorokoshi Dec 9, 2019

Oberon00 Dec 9, 2019

codeboten Nov 26, 2019

Oberon00 Nov 26, 2019

codeboten Nov 26, 2019

Oberon00 Nov 26, 2019

codeboten Nov 26, 2019

c24t left a comment

c24t commented Dec 5, 2019

Oberon00 commented Dec 5, 2019

toumorokoshi left a comment

toumorokoshi Dec 9, 2019

Oberon00 Dec 9, 2019 •

edited

Loading

toumorokoshi Dec 9, 2019

toumorokoshi Dec 9, 2019

toumorokoshi Dec 9, 2019

Oberon00 Dec 9, 2019

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

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

Conversation

Oberon00 commented Nov 21, 2019 • edited Loading

codecov-io commented Nov 25, 2019 • edited Loading

Codecov Report

codeboten 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c24t left a comment

Choose a reason for hiding this comment

c24t commented Dec 5, 2019

Oberon00 commented Dec 5, 2019

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 Dec 9, 2019 • edited Loading

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 Nov 21, 2019 •

edited

Loading

codecov-io commented Nov 25, 2019 •

edited

Loading

Oberon00 Dec 9, 2019 •

edited

Loading