Update WSGI & Flask integrations to follow new semantic conventions by Oberon00 · Pull Request #299 · open-telemetry/opentelemetry-python

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

Attention: Patch coverage is 47.36842% with 30 lines in your changes missing coverage. Please review.

Project coverage is 84.16%. Comparing base (eee7d84) to head (9c603cd).

Files with missing lines	Patch %	Lines
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py	47.36%	20 Missing and 10 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
- Coverage   85.56%   84.16%   -1.40%     
==========================================
  Files          33       33              
  Lines        1600     1636      +36     
  Branches      178      193      +15     
==========================================
+ Hits         1369     1377       +8     
- Misses        185      204      +19     
- Partials       46       55       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

+        result["http.target"] = target
    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)

Conversation

Oberon00 commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Nov 25, 2019 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codeboten left a comment

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!

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!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

c24t commented Dec 5, 2019

Uh oh!

Oberon00 commented Dec 5, 2019

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oberon00 Dec 9, 2019 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Oberon00 commented Nov 21, 2019 •

edited

Loading

codecov-io commented Nov 25, 2019 •

edited by codecov bot

Loading

Oberon00 Dec 9, 2019 •

edited

Loading