From 2bbc8bea0605a63502180f48064bef38e2411501 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Wed, 18 Sep 2019 11:08:21 +0200 Subject: [PATCH 1/9] wsgi: Fix non-absolute http.url (fixes #143). --- .../src/opentelemetry/ext/wsgi/__init__.py | 33 +++++++++++++++---- .../tests/test_wsgi_middleware.py | 30 +++++++++++++++++ 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py index 28caa77e52b..00d54339f9d 100644 --- a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py +++ b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py @@ -21,6 +21,7 @@ import functools import typing import wsgiref.util as wsgiref_util +from urllib.parse import urlparse from opentelemetry import propagators, trace from opentelemetry.ext.wsgi.version import __version__ # noqa @@ -45,13 +46,33 @@ def _add_request_attributes(span, environ): span.set_attribute("http.method", environ["REQUEST_METHOD"]) host = environ.get("HTTP_HOST") or environ["SERVER_NAME"] - span.set_attribute("http.host", host) + span.set_attribute("http.host", host) # NOTE: Nonstandard. + + url = environ.get("REQUEST_URI") or environ.get("RAW_URI") + + if url: # We got something, but is absolute? + # The simplistic ``"://" in url` is not sufficient, + # as that could be contained in the query string. + try: + urlparts = urlparse(url) + except Exception: # pylint:disable=broad-except + url = wsgiref_util.request_uri(environ) + else: + if not urlparts.netloc: + scheme = environ["wsgi.url_scheme"] + portstr = "" + if scheme == "https": + if environ.get("SERVER_PORT", "443") != "443": + portstr = ":" + environ["SERVER_PORT"] + elif environ.get("SERVER_PORT", "80") != "80": + portstr = ":" + environ["SERVER_PORT"] + + url = ( + scheme + "://" + (host or "localhost") + portstr + url + ) + else: + url = wsgiref_util.request_uri(environ) - url = ( - environ.get("REQUEST_URI") - or environ.get("RAW_URI") - or wsgiref_util.request_uri(environ, include_query=False) - ) span.set_attribute("http.url", url) @staticmethod diff --git a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py index 52cffc051ad..305d51b55f2 100644 --- a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py +++ b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py @@ -171,6 +171,36 @@ def test_request_attributes(self): self.assertEqual(self.span.set_attribute.call_count, len(expected)) self.span.set_attribute.assert_has_calls(expected, any_order=True) + def test_request_attributes_with_partial_raw_uri(self): + self.environ["RAW_URI"] = "/#top" + OpenTelemetryMiddleware._add_request_attributes( # noqa pylint: disable=protected-access + self.span, self.environ + ) + self.span.set_attribute.assert_any_call( + "http.url", "http://127.0.0.1/#top" + ) + + def test_request_attributes_with_partial_raw_uri_and_nonstandard_port( + self + ): + self.environ["RAW_URI"] = "/#top" + self.environ["SERVER_PORT"] = "8080" + OpenTelemetryMiddleware._add_request_attributes( # noqa pylint: disable=protected-access + self.span, self.environ + ) + self.span.set_attribute.assert_any_call( + "http.url", "http://127.0.0.1:8080/#top" + ) + + def test_request_attributes_with_full_request_uri(self): + self.environ["REQUEST_URI"] = "http://foobar.com:8080/?foo=bar#top" + OpenTelemetryMiddleware._add_request_attributes( # noqa pylint: disable=protected-access + self.span, self.environ + ) + self.span.set_attribute.assert_any_call( + "http.url", "http://foobar.com:8080/?foo=bar#top" + ) + def test_response_attributes(self): OpenTelemetryMiddleware._add_response_attributes( # noqa pylint: disable=protected-access self.span, "404 Not Found" From cfdcaef598ef21963df05e2a3b15d00d632e46c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Wed, 18 Sep 2019 13:34:20 +0200 Subject: [PATCH 2/9] wsgi: Don't delay calling wrapped app. --- .../src/opentelemetry/ext/wsgi/__init__.py | 29 ++++++++--- .../tests/test_wsgi_middleware.py | 51 ++++++++++++------- 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py index 00d54339f9d..5f0fb23de71 100644 --- a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py +++ b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py @@ -108,19 +108,32 @@ def __call__(self, environ, start_response): path_info = environ["PATH_INFO"] or "/" parent_span = propagators.extract(get_header_from_environ, environ) - with tracer.start_span( + span = tracer.create_span( path_info, parent_span, kind=trace.SpanKind.SERVER - ) as span: + ) + span.start() + try: self._add_request_attributes(span, environ) start_response = self._create_start_response(span, start_response) iterable = self.wsgi(environ, start_response) - try: - for yielded in iterable: - yield yielded - finally: - if hasattr(iterable, "close"): - iterable.close() + + # Put this in a subfunction to not delay the call to the wrapped + # WSGI application (instrumentation should change the application + # behavior as little as possible). + def iter_result(): + try: + for yielded in iterable: + yield yielded + finally: + if hasattr(iterable, "close"): + iterable.close() + span.end() + + return iter_result() + except: # noqa + span.end() + raise def get_header_from_environ( diff --git a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py index 305d51b55f2..9dbc751c6f7 100644 --- a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py +++ b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py @@ -52,6 +52,15 @@ def iter_wsgi(environ, start_response): return iter_wsgi +def create_gen_wsgi(response): + def gen_wsgi(environ, start_response): + result = create_iter_wsgi(response)(environ, start_response) + yield from result + getattr(result, "close", lambda: None)() + + return gen_wsgi + + def error_wsgi(environ, start_response): assert isinstance(environ, dict) try: @@ -66,18 +75,15 @@ def error_wsgi(environ, start_response): class TestWsgiApplication(unittest.TestCase): def setUp(self): tracer = trace_api.tracer() - self.span_context_manager = mock.MagicMock() - self.span_context_manager.__enter__.return_value = mock.create_autospec( - trace_api.Span, spec_set=True - ) - self.patcher = mock.patch.object( + self.span = mock.create_autospec(trace_api.Span, spec_set=True) + self.create_span_patcher = mock.patch.object( tracer, - "start_span", + "create_span", autospec=True, spec_set=True, - return_value=self.span_context_manager, + return_value=self.span, ) - self.start_span = self.patcher.start() + self.create_span = self.create_span_patcher.start() self.write_buffer = io.BytesIO() self.write = self.write_buffer.write @@ -90,11 +96,11 @@ def setUp(self): self.exc_info = None def tearDown(self): - self.patcher.stop() + self.create_span_patcher.stop() def start_response(self, status, response_headers, exc_info=None): # The span should have started already - self.span_context_manager.__enter__.assert_called_with() + self.span.start.assert_called_once_with() self.status = status self.response_headers = response_headers @@ -105,12 +111,10 @@ def validate_response(self, response, error=None): while True: try: value = next(response) - self.span_context_manager.__exit__.assert_not_called() + self.assertEqual(0, self.span.end.call_count) self.assertEqual(value, b"*") except StopIteration: - self.span_context_manager.__exit__.assert_called_with( - None, None, None - ) + self.span.end.assert_called_once_with() break self.assertEqual(self.status, "200 OK") @@ -125,9 +129,10 @@ def validate_response(self, response, error=None): self.assertIsNone(self.exc_info) # Verify that start_span has been called - self.start_span.assert_called_once_with( + self.create_span.assert_called_with( "/", trace_api.INVALID_SPAN_CONTEXT, kind=trace_api.SpanKind.SERVER ) + self.span.start.assert_called_with() def test_basic_wsgi_call(self): app = OpenTelemetryMiddleware(simple_wsgi) @@ -139,12 +144,24 @@ def test_wsgi_iterable(self): iter_wsgi = create_iter_wsgi(original_response) app = OpenTelemetryMiddleware(iter_wsgi) response = app(self.environ, self.start_response) - # Verify that start_response has not been called yet + # Verify that start_response has been called + self.assertTrue(self.status) + self.validate_response(response) + + # Verify that close has been called exactly once + self.assertEqual(original_response.close_calls, 1) + + def test_wsgi_generator(self): + original_response = Response() + gen_wsgi = create_gen_wsgi(original_response) + app = OpenTelemetryMiddleware(gen_wsgi) + response = app(self.environ, self.start_response) + # Verify that start_response has not been called self.assertIsNone(self.status) self.validate_response(response) # Verify that close has been called exactly once - assert original_response.close_calls == 1 + self.assertEqual(original_response.close_calls, 1) def test_wsgi_exc_info(self): app = OpenTelemetryMiddleware(error_wsgi) From 48fa68438c0098ba6a5dd0fe4450f6312e700fac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Wed, 18 Sep 2019 14:09:30 +0200 Subject: [PATCH 3/9] Test query string handling. --- ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py index 9dbc751c6f7..603f01e9e02 100644 --- a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py +++ b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py @@ -176,14 +176,17 @@ def setUp(self): self.span = mock.create_autospec(trace_api.Span, spec_set=True) def test_request_attributes(self): + self.environ["QUERY_STRING"] = "foo=bar" + OpenTelemetryMiddleware._add_request_attributes( # noqa pylint: disable=protected-access self.span, self.environ ) + expected = ( mock.call("component", "http"), mock.call("http.method", "GET"), mock.call("http.host", "127.0.0.1"), - mock.call("http.url", "http://127.0.0.1/"), + mock.call("http.url", "http://127.0.0.1/?foo=bar"), ) self.assertEqual(self.span.set_attribute.call_count, len(expected)) self.span.set_attribute.assert_has_calls(expected, any_order=True) From 77e641b1c5292f46336bd5beb458bd4ba3cca2f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Wed, 18 Sep 2019 15:10:58 +0200 Subject: [PATCH 4/9] http.host & url: Fix and test edge cases. --- .../src/opentelemetry/ext/wsgi/__init__.py | 37 +++++++---- .../tests/test_wsgi_middleware.py | 65 ++++++++++++++----- 2 files changed, 70 insertions(+), 32 deletions(-) diff --git a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py index 5f0fb23de71..9aee437119e 100644 --- a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py +++ b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py @@ -45,8 +45,17 @@ def _add_request_attributes(span, environ): span.set_attribute("component", "http") span.set_attribute("http.method", environ["REQUEST_METHOD"]) - host = environ.get("HTTP_HOST") or environ["SERVER_NAME"] - span.set_attribute("http.host", host) # NOTE: Nonstandard. + host = environ.get("HTTP_HOST") + if not host: + host = environ["SERVER_NAME"] + if environ["wsgi.url_scheme"] == "https": + if environ.get("SERVER_PORT", "443") != "443": + host += ":" + environ["SERVER_PORT"] + elif environ.get("SERVER_PORT", "80") != "80": + host += ":" + environ["SERVER_PORT"] + + # NOTE: Nonstandard, may (not) include port + span.set_attribute("http.host", host) url = environ.get("REQUEST_URI") or environ.get("RAW_URI") @@ -58,18 +67,18 @@ def _add_request_attributes(span, environ): except Exception: # pylint:disable=broad-except url = wsgiref_util.request_uri(environ) else: - if not urlparts.netloc: - scheme = environ["wsgi.url_scheme"] - portstr = "" - if scheme == "https": - if environ.get("SERVER_PORT", "443") != "443": - portstr = ":" + environ["SERVER_PORT"] - elif environ.get("SERVER_PORT", "80") != "80": - portstr = ":" + environ["SERVER_PORT"] - - url = ( - scheme + "://" + (host or "localhost") + portstr + url - ) + if url.startswith("//"): # Scheme-relative URL + url = url[2:] + if ( + not urlparts.netloc and urlparts.scheme + ): # E.g., "http:///?" + scheme, path = url.split("://", 1) + url = scheme + "://" + host + path + elif not urlparts.netloc or not urlparts.scheme: + scheme = environ["wsgi.url_scheme"] + "://" + if not urlparts.netloc: + url = host + url + url = scheme + url else: url = wsgiref_util.request_uri(environ) diff --git a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py index 603f01e9e02..29d4dc794ff 100644 --- a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py +++ b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py @@ -17,6 +17,7 @@ import unittest import unittest.mock as mock import wsgiref.util as wsgiref_util +from urllib.parse import urlparse from opentelemetry import trace as trace_api from opentelemetry.ext.wsgi import OpenTelemetryMiddleware @@ -191,35 +192,63 @@ def test_request_attributes(self): self.assertEqual(self.span.set_attribute.call_count, len(expected)) self.span.set_attribute.assert_has_calls(expected, any_order=True) - def test_request_attributes_with_partial_raw_uri(self): - self.environ["RAW_URI"] = "/#top" + def validate_url(self, expected_url): OpenTelemetryMiddleware._add_request_attributes( # noqa pylint: disable=protected-access self.span, self.environ ) - self.span.set_attribute.assert_any_call( - "http.url", "http://127.0.0.1/#top" + attrs = { + args[0][0]: args[0][1] + for args in self.span.set_attribute.call_args_list + } + self.assertIn("http.url", attrs) + self.assertEqual(attrs["http.url"], expected_url) + self.assertIn("http.host", attrs) + self.assertEqual( + attrs["http.host"], urlparse(attrs["http.url"]).netloc ) + def test_request_attributes_with_partial_raw_uri(self): + self.environ["RAW_URI"] = "/#top" + self.validate_url("http://127.0.0.1/#top") + def test_request_attributes_with_partial_raw_uri_and_nonstandard_port( self ): - self.environ["RAW_URI"] = "/#top" + self.environ["RAW_URI"] = "/?" + del self.environ["HTTP_HOST"] self.environ["SERVER_PORT"] = "8080" - OpenTelemetryMiddleware._add_request_attributes( # noqa pylint: disable=protected-access - self.span, self.environ - ) - self.span.set_attribute.assert_any_call( - "http.url", "http://127.0.0.1:8080/#top" - ) + self.validate_url("http://127.0.0.1:8080/?") + + def test_request_attributes_with_nonstandard_port_and_no_host(self): + del self.environ["HTTP_HOST"] + self.environ["SERVER_PORT"] = "8080" + self.validate_url("http://127.0.0.1:8080/") + + def test_request_attributes_with_nonstandard_port(self): + self.environ["HTTP_HOST"] += ":8080" + self.validate_url("http://127.0.0.1:8080/") + + def test_request_attributes_with_scheme_relative_raw_uri(self): + self.environ["RAW_URI"] = "//127.0.0.1/?" + self.validate_url("http://127.0.0.1/?") + + def test_request_attributes_with_netlocless_raw_uri(self): + self.environ["RAW_URI"] = "http:///?" + self.validate_url("http://127.0.0.1/?") + + def test_request_attributes_with_pathless_raw_uri(self): + self.environ["RAW_URI"] = "http://hello" + self.environ["HTTP_HOST"] = "hello" + self.validate_url("http://hello") + + def test_request_attributes_with_strange_raw_uri(self): + self.environ["RAW_URI"] = "http://?" + self.validate_url("http://127.0.0.1?") def test_request_attributes_with_full_request_uri(self): - self.environ["REQUEST_URI"] = "http://foobar.com:8080/?foo=bar#top" - OpenTelemetryMiddleware._add_request_attributes( # noqa pylint: disable=protected-access - self.span, self.environ - ) - self.span.set_attribute.assert_any_call( - "http.url", "http://foobar.com:8080/?foo=bar#top" - ) + self.environ["HTTP_HOST"] = "127.0.0.1:8080" + self.environ["REQUEST_URI"] = "http://127.0.0.1:8080/?foo=bar#top" + self.validate_url("http://127.0.0.1:8080/?foo=bar#top") def test_response_attributes(self): OpenTelemetryMiddleware._add_response_attributes( # noqa pylint: disable=protected-access From 28e69b59521d87d17e5654e05e2869505a742f8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Thu, 19 Sep 2019 13:52:32 +0200 Subject: [PATCH 5/9] Simplify. --- .../src/opentelemetry/ext/wsgi/__init__.py | 43 ++++++++----------- .../tests/test_wsgi_middleware.py | 28 +++++++----- 2 files changed, 35 insertions(+), 36 deletions(-) diff --git a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py index 9aee437119e..0869a2a198e 100644 --- a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py +++ b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py @@ -48,37 +48,28 @@ def _add_request_attributes(span, environ): host = environ.get("HTTP_HOST") if not host: host = environ["SERVER_NAME"] - if environ["wsgi.url_scheme"] == "https": - if environ.get("SERVER_PORT", "443") != "443": - host += ":" + environ["SERVER_PORT"] - elif environ.get("SERVER_PORT", "80") != "80": - host += ":" + environ["SERVER_PORT"] - - # NOTE: Nonstandard, may (not) include port + port = environ.get("SERVER_PORT") + if port and ( + port != "80" + and environ["wsgi.url_scheme"] == "http" + or port != "443" + ): + host += ":" + port + + # NOTE: Nonstandard span.set_attribute("http.host", host) url = environ.get("REQUEST_URI") or environ.get("RAW_URI") - if url: # We got something, but is absolute? - # The simplistic ``"://" in url` is not sufficient, - # as that could be contained in the query string. - try: - urlparts = urlparse(url) - except Exception: # pylint:disable=broad-except + 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) - else: - if url.startswith("//"): # Scheme-relative URL - url = url[2:] - if ( - not urlparts.netloc and urlparts.scheme - ): # E.g., "http:///?" - scheme, path = url.split("://", 1) - url = scheme + "://" + host + path - elif not urlparts.netloc or not urlparts.scheme: - scheme = environ["wsgi.url_scheme"] + "://" - if not urlparts.netloc: - url = host + url - url = scheme + url else: url = wsgiref_util.request_uri(environ) diff --git a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py index 29d4dc794ff..cd1778c492e 100644 --- a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py +++ b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py @@ -219,32 +219,40 @@ def test_request_attributes_with_partial_raw_uri_and_nonstandard_port( self.environ["SERVER_PORT"] = "8080" self.validate_url("http://127.0.0.1:8080/?") + def test_https_uri_port(self): + del self.environ["HTTP_HOST"] + self.environ["SERVER_PORT"] = "443" + self.environ["wsgi.url_scheme"] = "https" + self.validate_url("https://127.0.0.1/") + + self.environ["SERVER_PORT"] = "8080" + self.validate_url("https://127.0.0.1:8080/") + + self.environ["SERVER_PORT"] = "80" + self.validate_url("https://127.0.0.1:80/") + def test_request_attributes_with_nonstandard_port_and_no_host(self): del self.environ["HTTP_HOST"] self.environ["SERVER_PORT"] = "8080" self.validate_url("http://127.0.0.1:8080/") + self.environ["SERVER_PORT"] = "443" + self.validate_url("http://127.0.0.1:443/") + def test_request_attributes_with_nonstandard_port(self): self.environ["HTTP_HOST"] += ":8080" self.validate_url("http://127.0.0.1:8080/") - def test_request_attributes_with_scheme_relative_raw_uri(self): + def test_request_attributes_with_faux_scheme_relative_raw_uri(self): self.environ["RAW_URI"] = "//127.0.0.1/?" - self.validate_url("http://127.0.0.1/?") - - def test_request_attributes_with_netlocless_raw_uri(self): - self.environ["RAW_URI"] = "http:///?" - self.validate_url("http://127.0.0.1/?") + self.validate_url("http://127.0.0.1//127.0.0.1/?") def test_request_attributes_with_pathless_raw_uri(self): + self.environ["PATH_INFO"] = "" self.environ["RAW_URI"] = "http://hello" self.environ["HTTP_HOST"] = "hello" self.validate_url("http://hello") - def test_request_attributes_with_strange_raw_uri(self): - self.environ["RAW_URI"] = "http://?" - self.validate_url("http://127.0.0.1?") - def test_request_attributes_with_full_request_uri(self): self.environ["HTTP_HOST"] = "127.0.0.1:8080" self.environ["REQUEST_URI"] = "http://127.0.0.1:8080/?foo=bar#top" From fa7aa210422afc484b2f1945ddbb4b79565b846b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Thu, 19 Sep 2019 13:59:17 +0200 Subject: [PATCH 6/9] Optimize iter_result. --- .../src/opentelemetry/ext/wsgi/__init__.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py index 0869a2a198e..d54674476b6 100644 --- a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py +++ b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py @@ -21,7 +21,6 @@ import functools import typing import wsgiref.util as wsgiref_util -from urllib.parse import urlparse from opentelemetry import propagators, trace from opentelemetry.ext.wsgi.version import __version__ # noqa @@ -121,16 +120,17 @@ def __call__(self, environ, start_response): # Put this in a subfunction to not delay the call to the wrapped # WSGI application (instrumentation should change the application # behavior as little as possible). - def iter_result(): + def iter_result(iterable, span): try: for yielded in iterable: yield yielded finally: - if hasattr(iterable, "close"): - iterable.close() + close = getattr(iterable, "close", None) + if close: + close() span.end() - return iter_result() + return iter_result(iterable, span) except: # noqa span.end() raise From 8329a2f6b5487e7231d7ceb503239e6e9db15275 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Thu, 19 Sep 2019 16:49:29 +0200 Subject: [PATCH 7/9] Fix active span in WSGI instrumentation. --- .../src/opentelemetry/ext/wsgi/__init__.py | 40 ++++++++++--------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py index d54674476b6..3969f0d935d 100644 --- a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py +++ b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py @@ -112,25 +112,27 @@ def __call__(self, environ, start_response): ) span.start() try: - self._add_request_attributes(span, environ) - start_response = self._create_start_response(span, start_response) - - iterable = self.wsgi(environ, start_response) - - # Put this in a subfunction to not delay the call to the wrapped - # WSGI application (instrumentation should change the application - # behavior as little as possible). - def iter_result(iterable, span): - try: - for yielded in iterable: - yield yielded - finally: - close = getattr(iterable, "close", None) - if close: - close() - span.end() - - return iter_result(iterable, span) + with tracer.use_span(span): + self._add_request_attributes(span, environ) + start_response = self._create_start_response(span, start_response) + + iterable = self.wsgi(environ, start_response) + + # Put this in a subfunction to not delay the call to the wrapped + # WSGI application (instrumentation should change the application + # behavior as little as possible). + def iter_result(iterable, span): + try: + with tracer.use_span(span): + for yielded in iterable: + yield yielded + finally: + close = getattr(iterable, "close", None) + if close: + close() + span.end() + + return iter_result(iterable, span) except: # noqa span.end() raise From 99fcb9096a8c26a0d3749e9582a14ae1cfea112b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Mon, 23 Sep 2019 16:38:49 +0200 Subject: [PATCH 8/9] Lint, SERVER_PORT. --- .../src/opentelemetry/ext/wsgi/__init__.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py index 3969f0d935d..80cdf50d08c 100644 --- a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py +++ b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py @@ -47,8 +47,8 @@ def _add_request_attributes(span, environ): host = environ.get("HTTP_HOST") if not host: host = environ["SERVER_NAME"] - port = environ.get("SERVER_PORT") - if port and ( + port = environ["SERVER_PORT"] + if ( port != "80" and environ["wsgi.url_scheme"] == "http" or port != "443" @@ -114,7 +114,9 @@ def __call__(self, environ, start_response): try: with tracer.use_span(span): self._add_request_attributes(span, environ) - start_response = self._create_start_response(span, start_response) + start_response = self._create_start_response( + span, start_response + ) iterable = self.wsgi(environ, start_response) From df9ea84f91e15cf89ec04401000181b2696857bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Tue, 24 Sep 2019 11:43:43 +0200 Subject: [PATCH 9/9] Move iter_result to a global function. Co-authored-by: Allan Feldman <6374032+a-feld@users.noreply.github.com> --- .../src/opentelemetry/ext/wsgi/__init__.py | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py index 80cdf50d08c..61f061948d3 100644 --- a/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py +++ b/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py @@ -105,7 +105,7 @@ def __call__(self, environ, start_response): tracer = trace.tracer() path_info = environ["PATH_INFO"] or "/" - parent_span = propagators.extract(get_header_from_environ, environ) + parent_span = propagators.extract(_get_header_from_environ, environ) span = tracer.create_span( path_info, parent_span, kind=trace.SpanKind.SERVER @@ -119,28 +119,13 @@ def __call__(self, environ, start_response): ) iterable = self.wsgi(environ, start_response) - - # Put this in a subfunction to not delay the call to the wrapped - # WSGI application (instrumentation should change the application - # behavior as little as possible). - def iter_result(iterable, span): - try: - with tracer.use_span(span): - for yielded in iterable: - yield yielded - finally: - close = getattr(iterable, "close", None) - if close: - close() - span.end() - - return iter_result(iterable, span) + return _end_span_after_iterating(iterable, span, tracer) except: # noqa span.end() raise -def get_header_from_environ( +def _get_header_from_environ( environ: dict, header_name: str ) -> typing.List[str]: """Retrieve the header value from the wsgi environ dictionary. @@ -153,3 +138,18 @@ def get_header_from_environ( if value: return [value] return [] + + +# Put this in a subfunction to not delay the call to the wrapped +# WSGI application (instrumentation should change the application +# behavior as little as possible). +def _end_span_after_iterating(iterable, span, tracer): + try: + with tracer.use_span(span): + for yielded in iterable: + yield yielded + finally: + close = getattr(iterable, "close", None) + if close: + close() + span.end()