Merge branch 'master' into named-tracers-merge

Author: c24t

.flake8

@@ -3,13 +3,16 @@ ignore =
   E501  # line too long, defer to black
   F401  # unused import, defer to pylint
   W503  # allow line breaks after binary ops, not after
+  E203  # allow whitespace before ':' (https://github.com/psf/black#slices) 
 exclude =
   .bzr
   .git
   .hg
   .svn
   .tox
   CVS
+  .venv*/
+  venv*/
   target
   __pycache__
   ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/gen/

.github/CODEOWNERS

@@ -2,4 +2,4 @@
 # This file controls who is tagged for review for any given pull request.
 
 # For anything not explicitly taken by someone else:
-*               @a-feld @c24t @carlosalberto @lzchen @Oberon00 @reyang @toumorokoshi
+*               @open-telemetry/python-approvers

.gitignore

@@ -20,6 +20,7 @@ lib
 lib64
 __pycache__
 venv*/
+.venv*/
 
 # Installer logs
 pip-log.txt

.isort.cfg

@@ -13,5 +13,5 @@ line_length=79
 ; docs: https://github.com/timothycrosley/isort#multi-line-output-modes
 multi_line_output=3
 skip=target
-skip_glob=ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/gen/*
+skip_glob=ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/gen/*,.venv*/*,venv*/*
 known_first_party=opentelemetry

CONTRIBUTING.md

@@ -28,7 +28,14 @@ during their normal contribution hours.
 
 ## Development
 
-This project uses [`tox`](https://tox.readthedocs.io) to automate some aspects
+To quickly get up and running, you can use the `scripts/eachdist.py` tool that
+ships with this project. First create a virtualenv and activate it.
+Then run `python scripts/eachdist.py develop` to install all required packages
+as well as the project's packages themselves (in `--editable` mode).
+You can then run `scripts/eachdist.py test` to test everything or
+`scripts/eachdist.py lint` to lint everything (fixing anything that is auto-fixable).
+
+Additionally, this project uses [`tox`](https://tox.readthedocs.io) to automate some aspects
 of development, including testing against multiple Python versions.
 
 You can run:

dev-requirements.txt

@@ -0,0 +1,10 @@
+pylint~=2.3
+flake8~=3.7
+isort~=4.3
+black>=19.3b0,==19.*
+mypy==0.740
+sphinx~=2.1
+sphinx-rtd-theme~=0.4
+sphinx-autodoc-typehints~=1.10.2
+pytest!=5.2.3
+pytest-cov>=2.8

eachdist.ini

@@ -0,0 +1,16 @@
+# These will be sorted first in that order.
+# All packages that are depended upon by others should be listed here.
+[DEFAULT]
+sortfirst=
+    opentelemetry-api
+    opentelemetry-sdk
+    ext/opentelemetry-ext-wsgi
+    ext/*
+
+[lintroots]
+extraroots=examples/*,scripts/
+subglob=*.py,tests/,test/,src/*,examples/*
+
+[testroots]
+extraroots=examples/*,tests/
+subglob=tests/,test/

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

@@ -65,20 +65,21 @@ def _before_flask_request():
         "opentelemetry-ext-flask", __version__
     )
 
+    attributes = otel_wsgi.collect_request_attributes(environ)
+    if flask_request.url_rule:
+        # For 404 that result from no route found, etc, we don't have a url_rule.
+        attributes["http.route"] = flask_request.url_rule.rule
     span = tracer.start_span(
         span_name,
         parent_span,
         kind=trace.SpanKind.SERVER,
+        attributes=attributes,
         start_time=environ.get(_ENVIRON_STARTTIME_KEY),
     )
     activation = tracer.use_span(span, end_on_exit=True)
     activation.__enter__()
     environ[_ENVIRON_ACTIVATION_KEY] = activation
     environ[_ENVIRON_SPAN_KEY] = span
-    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)
 
 
 def _teardown_flask_request(exc):

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

@@ -57,22 +57,25 @@ def test_simple(self):
             "hello_endpoint",
             trace_api.INVALID_SPAN_CONTEXT,
             kind=trace_api.SpanKind.SERVER,
+            attributes={
+                "component": "http",
+                "http.method": "GET",
+                "http.server_name": "localhost",
+                "http.scheme": "http",
+                "host.port": 80,
+                "http.host": "localhost",
+                "http.target": "/hello/123",
+                "http.flavor": "1.1",
+                "http.route": "/hello/<int:helloid>",
+            },
             start_time=mock.ANY,
         )
 
         # TODO: Change this test to use the SDK, as mocking becomes painful
 
         self.assertEqual(
             self.span_attrs,
-            {
-                "component": "http",
-                "http.method": "GET",
-                "http.host": "localhost",
-                "http.url": "http://localhost/hello/123",
-                "http.route": "/hello/<int:helloid>",
-                "http.status_code": 200,
-                "http.status_text": "OK",
-            },
+            {"http.status_code": 200, "http.status_text": "OK"},
         )
 
     def test_404(self):
@@ -84,6 +87,16 @@ def test_404(self):
             "/bye",
             trace_api.INVALID_SPAN_CONTEXT,
             kind=trace_api.SpanKind.SERVER,
+            attributes={
+                "component": "http",
+                "http.method": "POST",
+                "http.server_name": "localhost",
+                "http.scheme": "http",
+                "host.port": 80,
+                "http.host": "localhost",
+                "http.target": "/bye",
+                "http.flavor": "1.1",
+            },
             start_time=mock.ANY,
         )
 
@@ -93,14 +106,7 @@ def test_404(self):
 
         self.assertEqual(
             self.span_attrs,
-            {
-                "component": "http",
-                "http.method": "POST",
-                "http.host": "localhost",
-                "http.url": "http://localhost/bye",
-                "http.status_code": 404,
-                "http.status_text": "NOT FOUND",
-            },
+            {"http.status_code": 404, "http.status_text": "NOT FOUND"},
         )
 
     def test_internal_error(self):
@@ -112,6 +118,17 @@ def test_internal_error(self):
             "hello_endpoint",
             trace_api.INVALID_SPAN_CONTEXT,
             kind=trace_api.SpanKind.SERVER,
+            attributes={
+                "component": "http",
+                "http.method": "GET",
+                "http.server_name": "localhost",
+                "http.scheme": "http",
+                "host.port": 80,
+                "http.host": "localhost",
+                "http.target": "/hello/500",
+                "http.flavor": "1.1",
+                "http.route": "/hello/<int:helloid>",
+            },
             start_time=mock.ANY,
         )
 
@@ -122,11 +139,6 @@ def test_internal_error(self):
         self.assertEqual(
             self.span_attrs,
             {
-                "component": "http",
-                "http.method": "GET",
-                "http.host": "localhost",
-                "http.url": "http://localhost/hello/500",
-                "http.route": "/hello/<int:helloid>",
                 "http.status_code": 500,
                 "http.status_text": "INTERNAL SERVER ERROR",
             },

ext/opentelemetry-ext-testutil/setup.cfg

@@ -38,7 +38,7 @@ package_dir=
     =src
 packages=find_namespace:
 install_requires =
-    opentelemetry-ext-wsgi
+    opentelemetry-api
 
 [options.extras_require]
 test = flask~=1.0

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

@@ -24,6 +24,9 @@
 
 from opentelemetry import propagators, trace
 from opentelemetry.ext.wsgi.version import __version__
+from opentelemetry.trace.status import Status, StatusCanonicalCode
+
+_HTTP_VERSION_PREFIX = "HTTP/"
 
 
 def get_header_from_environ(
@@ -41,44 +44,80 @@ def get_header_from_environ(
     return []
 
 
-def add_request_attributes(span, environ):
-    """Adds HTTP request attributes from the PEP3333-conforming WSGI environ to span."""
-
-    span.set_attribute("component", "http")
-    span.set_attribute("http.method", environ["REQUEST_METHOD"])
-
-    host = environ.get("HTTP_HOST")
-    if not host:
-        host = environ["SERVER_NAME"]
-        port = environ["SERVER_PORT"]
-        scheme = environ["wsgi.url_scheme"]
-        if (
-            scheme == "http"
-            and port != "80"
-            or scheme == "https"
-            and port != "443"
-        ):
-            host += ":" + port
-
-    # NOTE: Nonstandard (but see
-    # https://github.com/open-telemetry/opentelemetry-specification/pull/263)
-    span.set_attribute("http.host", host)
-
-    url = environ.get("REQUEST_URI") or environ.get("RAW_URI")
-
-    if url:
-        if url[0] == "/":
-            # We assume that no scheme-relative URLs will be in url here.
-            # After all, if a request is made to http://myserver//foo, we may get
-            # //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()
-            url = wsgiref_util.request_uri(environ)
+def setifnotnone(dic, key, value):
+    if value is not None:
+        dic[key] = value
+
+
+def http_status_to_canonical_code(code: int, allow_redirect: bool = True):
+    # pylint:disable=too-many-branches,too-many-return-statements
+    if code < 100:
+        return StatusCanonicalCode.UNKNOWN
+    if code <= 299:
+        return StatusCanonicalCode.OK
+    if code <= 399:
+        if allow_redirect:
+            return StatusCanonicalCode.OK
+        return StatusCanonicalCode.DEADLINE_EXCEEDED
+    if code <= 499:
+        if code == 401:  # HTTPStatus.UNAUTHORIZED:
+            return StatusCanonicalCode.UNAUTHENTICATED
+        if code == 403:  # HTTPStatus.FORBIDDEN:
+            return StatusCanonicalCode.PERMISSION_DENIED
+        if code == 404:  # HTTPStatus.NOT_FOUND:
+            return StatusCanonicalCode.NOT_FOUND
+        if code == 429:  # HTTPStatus.TOO_MANY_REQUESTS:
+            return StatusCanonicalCode.RESOURCE_EXHAUSTED
+        return StatusCanonicalCode.INVALID_ARGUMENT
+    if code <= 599:
+        if code == 501:  # HTTPStatus.NOT_IMPLEMENTED:
+            return StatusCanonicalCode.UNIMPLEMENTED
+        if code == 503:  # HTTPStatus.SERVICE_UNAVAILABLE:
+            return StatusCanonicalCode.UNAVAILABLE
+        if code == 504:  # HTTPStatus.GATEWAY_TIMEOUT:
+            return StatusCanonicalCode.DEADLINE_EXCEEDED
+        return StatusCanonicalCode.INTERNAL
+    return StatusCanonicalCode.UNKNOWN
+
+
+def collect_request_attributes(environ):
+    """Collects HTTP request attributes from the PEP3333-conforming
+    WSGI environ and returns a dictionary to be used as span creation attributes."""
+
+    result = {
+        "component": "http",
+        "http.method": environ["REQUEST_METHOD"],
+        "http.server_name": environ["SERVER_NAME"],
+        "http.scheme": environ["wsgi.url_scheme"],
+        "host.port": int(environ["SERVER_PORT"]),
+    }
+
+    setifnotnone(result, "http.host", environ.get("HTTP_HOST"))
+    target = environ.get("RAW_URI")
+    if target is None:  # Note: `"" or None is None`
+        target = environ.get("REQUEST_URI")
+    if target is not None:
+        result["http.target"] = target
     else:
-        url = wsgiref_util.request_uri(environ)
+        result["http.url"] = wsgiref_util.request_uri(environ)
+
+    remote_addr = environ.get("REMOTE_ADDR")
+    if remote_addr:
+        result[
+            "peer.ipv6" if ":" in remote_addr else "peer.ipv4"
+        ] = remote_addr
+    remote_host = environ.get("REMOTE_HOST")
+    if remote_host and remote_host != remote_addr:
+        result["peer.hostname"] = remote_host
 
-    span.set_attribute("http.url", url)
+    setifnotnone(result, "peer.port", environ.get("REMOTE_PORT"))
+    flavor = environ.get("SERVER_PROTOCOL", "")
+    if flavor.upper().startswith(_HTTP_VERSION_PREFIX):
+        flavor = flavor[len(_HTTP_VERSION_PREFIX) :]
+    if flavor:
+        result["http.flavor"] = flavor
+
+    return result
 
 
 def add_response_attributes(
@@ -93,9 +132,15 @@ def add_response_attributes(
     try:
         status_code = int(status_code)
     except ValueError:
-        pass
+        span.set_status(
+            Status(
+                StatusCanonicalCode.UNKNOWN,
+                "Non-integer HTTP status: " + repr(status_code),
+            )
+        )
     else:
         span.set_attribute("http.status_code", status_code)
+        span.set_status(Status(http_status_to_canonical_code(status_code)))
 
 
 def get_default_span_name(environ):
@@ -144,18 +189,21 @@ def __call__(self, environ, start_response):
         span_name = get_default_span_name(environ)
 
         span = self.tracer.start_span(
-            span_name, parent_span, kind=trace.SpanKind.SERVER
+            span_name,
+            parent_span,
+            kind=trace.SpanKind.SERVER,
+            attributes=collect_request_attributes(environ),
         )
 
         try:
             with self.tracer.use_span(span):
-                add_request_attributes(span, environ)
                 start_response = self._create_start_response(
                     span, start_response
                 )
                 iterable = self.wsgi(environ, start_response)
                 return _end_span_after_iterating(iterable, span, self.tracer)
         except:  # noqa
+            # TODO Set span status (cf. https://github.com/open-telemetry/opentelemetry-python/issues/292)
             span.end()
             raise