From 2691368d88489ad92ad316a4a0c56c52ef5ba44a Mon Sep 17 00:00:00 2001 From: Bergutov Ruslan Date: Tue, 2 Jun 2020 17:56:20 +0500 Subject: [PATCH 1/4] requests exceptions handling --- .../opentelemetry/ext/requests/__init__.py | 31 +++++++---- .../tests/test_requests_integration.py | 54 +++++++++++++++++-- 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py b/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py index 1621c4a95e6..48999cd3ab9 100644 --- a/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py +++ b/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py @@ -45,12 +45,13 @@ import types from urllib.parse import urlparse +from requests import RequestException, Response from requests.sessions import Session from opentelemetry import context, propagators, trace from opentelemetry.auto_instrumentation.instrumentor import BaseInstrumentor from opentelemetry.ext.requests.version import __version__ -from opentelemetry.trace import SpanKind, get_tracer +from opentelemetry.trace import SpanKind from opentelemetry.trace.status import Status, StatusCanonicalCode @@ -87,6 +88,8 @@ def instrumented_request(self, method, url, *args, **kwargs): path = "" path = parsed_url.path + exception = None + with tracer.start_as_current_span(path, kind=SpanKind.CLIENT) as span: span.set_attribute("component", "http") span.set_attribute("http.method", method.upper()) @@ -94,17 +97,27 @@ def instrumented_request(self, method, url, *args, **kwargs): headers = kwargs.setdefault("headers", {}) propagators.inject(type(headers).__setitem__, headers) - result = wrapped(self, method, url, *args, **kwargs) # *** PROCEED - span.set_attribute("http.status_code", result.status_code) - span.set_attribute("http.status_text", result.reason) - span.set_status( - Status(_http_status_to_canonical_code(result.status_code)) - ) + try: + result = wrapped(self, method, url, *args, **kwargs) # *** PROCEED + except Exception as e: + exception = e + result = getattr(e, "response", None) + + if result is not None: + span.set_attribute("http.status_code", result.status_code) + span.set_attribute("http.status_text", result.reason) + span.set_status(Status(_http_status_to_canonical_code(result.status_code))) + else: + span.set_status(Status(StatusCanonicalCode.UNKNOWN)) + if span_callback is not None: span_callback(span, result) - return result + if exception is not None: + raise exception.with_traceback(exception.__traceback__) + + return result instrumented_request.opentelemetry_ext_requests_applied = True @@ -182,7 +195,7 @@ def _uninstrument(self, **kwargs): def uninstrument_session(session): """Disables instrumentation on the session object.""" if getattr( - session.request, "opentelemetry_ext_requests_applied", False + session.request, "opentelemetry_ext_requests_applied", False ): original = session.request.__wrapped__ # pylint:disable=no-member session.request = types.MethodType(original, session) diff --git a/ext/opentelemetry-ext-requests/tests/test_requests_integration.py b/ext/opentelemetry-ext-requests/tests/test_requests_integration.py index 28359d8f38a..645d8b58253 100644 --- a/ext/opentelemetry-ext-requests/tests/test_requests_integration.py +++ b/ext/opentelemetry-ext-requests/tests/test_requests_integration.py @@ -13,17 +13,18 @@ # limitations under the License. import sys +from unittest import mock import httpretty +import opentelemetry.ext.requests import requests import urllib3 - -import opentelemetry.ext.requests from opentelemetry import context, propagators, trace from opentelemetry.ext.requests import RequestsInstrumentor from opentelemetry.sdk import resources from opentelemetry.test.mock_httptextformat import MockHTTPTextFormat from opentelemetry.test.test_base import TestBase +from opentelemetry.trace.status import StatusCanonicalCode class TestRequestsIntegration(TestBase): @@ -94,7 +95,7 @@ def test_invalid_url(self): url = "http://[::1/nope" exception_type = requests.exceptions.InvalidURL if sys.version_info[:2] < (3, 5) and tuple( - map(int, urllib3.__version__.split(".")[:2]) + map(int, urllib3.__version__.split(".")[:2]) ) < (1, 25): exception_type = ValueError @@ -229,3 +230,50 @@ def test_custom_tracer_provider(self): span = span_list[0] self.assertIs(span.resource, resource) + + @mock.patch("requests.Session.send", side_effect=requests.RequestException) + def test_requests_exception_without_response(self, *_, **__): + + with self.assertRaises(requests.RequestException): + requests.get(self.URL) + + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) + span = span_list[0] + self.assertEqual(span.attributes, { + "component": "http", + "http.method": "GET", + "http.url": self.URL + }) + self.assertEqual(span.status.canonical_code, StatusCanonicalCode.UNKNOWN) + + mocked_response = requests.Response() + mocked_response.status_code = 500 + mocked_response.reason = "Internal Server Error" + + @mock.patch("requests.Session.send", side_effect=requests.RequestException(response=mocked_response)) + def test_requests_exception_with_response(self, *_, **__): + + with self.assertRaises(requests.RequestException): + requests.get(self.URL) + + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) + span = span_list[0] + self.assertEqual(span.attributes, { + "component": "http", + "http.method": "GET", + "http.url": self.URL, + "http.status_code": 500, + "http.status_text": "Internal Server Error", + }) + self.assertEqual(span.status.canonical_code, StatusCanonicalCode.INTERNAL) + + @mock.patch("requests.Session.send", side_effect=Exception) + def test_requests_basic_exception(self, *_, **__): + + with self.assertRaises(Exception): + requests.get(self.URL) + + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) From df292955d55587d6a6a7072a10f67a67cf603a46 Mon Sep 17 00:00:00 2001 From: Bergutov Ruslan Date: Tue, 2 Jun 2020 19:28:28 +0500 Subject: [PATCH 2/4] lint fixes --- .../opentelemetry/ext/requests/__init__.py | 50 +++++++++---- .../tests/test_requests_integration.py | 70 ++++++++++++------- 2 files changed, 81 insertions(+), 39 deletions(-) diff --git a/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py b/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py index 48999cd3ab9..e6ddf4d6b2c 100644 --- a/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py +++ b/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py @@ -45,7 +45,8 @@ import types from urllib.parse import urlparse -from requests import RequestException, Response +from requests import Timeout, URLRequired +from requests.exceptions import InvalidSchema, InvalidURL, MissingSchema from requests.sessions import Session from opentelemetry import context, propagators, trace @@ -81,16 +82,15 @@ def instrumented_request(self, method, url, *args, **kwargs): # https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#http-client try: parsed_url = urlparse(url) + span_name = parsed_url.path except ValueError as exc: # Invalid URL - path = "".format(exc) - else: - if parsed_url is None: - path = "" - path = parsed_url.path + span_name = "".format(exc) exception = None - with tracer.start_as_current_span(path, kind=SpanKind.CLIENT) as span: + with tracer.start_as_current_span( + span_name, kind=SpanKind.CLIENT + ) as span: span.set_attribute("component", "http") span.set_attribute("http.method", method.upper()) span.set_attribute("http.url", url) @@ -99,17 +99,24 @@ def instrumented_request(self, method, url, *args, **kwargs): propagators.inject(type(headers).__setitem__, headers) try: - result = wrapped(self, method, url, *args, **kwargs) # *** PROCEED - except Exception as e: - exception = e - result = getattr(e, "response", None) + result = wrapped( + self, method, url, *args, **kwargs + ) # *** PROCEED + except Exception as exc: # pylint: disable=W0703 + exception = exc + result = getattr(exc, "response", None) + + if exception is not None: + span.set_status( + Status(_exception_to_canonical_code(exception)) + ) if result is not None: span.set_attribute("http.status_code", result.status_code) span.set_attribute("http.status_text", result.reason) - span.set_status(Status(_http_status_to_canonical_code(result.status_code))) - else: - span.set_status(Status(StatusCanonicalCode.UNKNOWN)) + span.set_status( + Status(_http_status_to_canonical_code(result.status_code)) + ) if span_callback is not None: span_callback(span, result) @@ -170,6 +177,19 @@ def _http_status_to_canonical_code(code: int, allow_redirect: bool = True): return StatusCanonicalCode.UNKNOWN +def _exception_to_canonical_code(exc: Exception) -> StatusCanonicalCode: + if isinstance( + exc, (InvalidURL, InvalidSchema, MissingSchema, URLRequired) + ): + return StatusCanonicalCode.INVALID_ARGUMENT + if isinstance(exc, Timeout): + return StatusCanonicalCode.DEADLINE_EXCEEDED + if isinstance(exc, ValueError): + if len(exc.args) > 0 and exc.args[0] in ["Invalid IPv6 URL"]: + return StatusCanonicalCode.INVALID_ARGUMENT + return StatusCanonicalCode.UNKNOWN + + class RequestsInstrumentor(BaseInstrumentor): """An instrumentor for requests See `BaseInstrumentor` @@ -195,7 +215,7 @@ def _uninstrument(self, **kwargs): def uninstrument_session(session): """Disables instrumentation on the session object.""" if getattr( - session.request, "opentelemetry_ext_requests_applied", False + session.request, "opentelemetry_ext_requests_applied", False ): original = session.request.__wrapped__ # pylint:disable=no-member session.request = types.MethodType(original, session) diff --git a/ext/opentelemetry-ext-requests/tests/test_requests_integration.py b/ext/opentelemetry-ext-requests/tests/test_requests_integration.py index 645d8b58253..0ad5b9d19df 100644 --- a/ext/opentelemetry-ext-requests/tests/test_requests_integration.py +++ b/ext/opentelemetry-ext-requests/tests/test_requests_integration.py @@ -12,13 +12,12 @@ # See the License for the specific language governing permissions and # limitations under the License. -import sys from unittest import mock import httpretty -import opentelemetry.ext.requests import requests -import urllib3 + +import opentelemetry.ext.requests from opentelemetry import context, propagators, trace from opentelemetry.ext.requests import RequestsInstrumentor from opentelemetry.sdk import resources @@ -93,13 +92,8 @@ def test_not_foundbasic(self): def test_invalid_url(self): url = "http://[::1/nope" - exception_type = requests.exceptions.InvalidURL - if sys.version_info[:2] < (3, 5) and tuple( - map(int, urllib3.__version__.split(".")[:2]) - ) < (1, 25): - exception_type = ValueError - with self.assertRaises(exception_type): + with self.assertRaises(ValueError): requests.post(url) span_list = self.memory_exporter.get_finished_spans() @@ -111,6 +105,9 @@ def test_invalid_url(self): span.attributes, {"component": "http", "http.method": "POST", "http.url": url}, ) + self.assertEqual( + span.status.canonical_code, StatusCanonicalCode.INVALID_ARGUMENT + ) def test_uninstrument(self): RequestsInstrumentor().uninstrument() @@ -240,18 +237,22 @@ def test_requests_exception_without_response(self, *_, **__): span_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(span_list), 1) span = span_list[0] - self.assertEqual(span.attributes, { - "component": "http", - "http.method": "GET", - "http.url": self.URL - }) - self.assertEqual(span.status.canonical_code, StatusCanonicalCode.UNKNOWN) + self.assertEqual( + span.attributes, + {"component": "http", "http.method": "GET", "http.url": self.URL}, + ) + self.assertEqual( + span.status.canonical_code, StatusCanonicalCode.UNKNOWN + ) mocked_response = requests.Response() mocked_response.status_code = 500 mocked_response.reason = "Internal Server Error" - @mock.patch("requests.Session.send", side_effect=requests.RequestException(response=mocked_response)) + @mock.patch( + "requests.Session.send", + side_effect=requests.RequestException(response=mocked_response), + ) def test_requests_exception_with_response(self, *_, **__): with self.assertRaises(requests.RequestException): @@ -260,14 +261,19 @@ def test_requests_exception_with_response(self, *_, **__): span_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(span_list), 1) span = span_list[0] - self.assertEqual(span.attributes, { - "component": "http", - "http.method": "GET", - "http.url": self.URL, - "http.status_code": 500, - "http.status_text": "Internal Server Error", - }) - self.assertEqual(span.status.canonical_code, StatusCanonicalCode.INTERNAL) + self.assertEqual( + span.attributes, + { + "component": "http", + "http.method": "GET", + "http.url": self.URL, + "http.status_code": 500, + "http.status_text": "Internal Server Error", + }, + ) + self.assertEqual( + span.status.canonical_code, StatusCanonicalCode.INTERNAL + ) @mock.patch("requests.Session.send", side_effect=Exception) def test_requests_basic_exception(self, *_, **__): @@ -277,3 +283,19 @@ def test_requests_basic_exception(self, *_, **__): span_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(span_list), 1) + self.assertEqual( + span_list[0].status.canonical_code, StatusCanonicalCode.UNKNOWN + ) + + @mock.patch("requests.Session.send", side_effect=requests.Timeout) + def test_requests_timeout_exception(self, *_, **__): + + with self.assertRaises(Exception): + requests.get(self.URL) + + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) + self.assertEqual( + span_list[0].status.canonical_code, + StatusCanonicalCode.DEADLINE_EXCEEDED, + ) From ed16b6e031eae18396913e2a10f804a44ba0666c Mon Sep 17 00:00:00 2001 From: Bergutov Ruslan Date: Tue, 2 Jun 2020 20:33:35 +0500 Subject: [PATCH 3/4] make it simpler --- .../src/opentelemetry/ext/requests/__init__.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py b/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py index e6ddf4d6b2c..4befdfbc48a 100644 --- a/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py +++ b/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py @@ -179,14 +179,11 @@ def _http_status_to_canonical_code(code: int, allow_redirect: bool = True): def _exception_to_canonical_code(exc: Exception) -> StatusCanonicalCode: if isinstance( - exc, (InvalidURL, InvalidSchema, MissingSchema, URLRequired) + exc, (InvalidURL, InvalidSchema, MissingSchema, URLRequired, ValueError) ): return StatusCanonicalCode.INVALID_ARGUMENT if isinstance(exc, Timeout): return StatusCanonicalCode.DEADLINE_EXCEEDED - if isinstance(exc, ValueError): - if len(exc.args) > 0 and exc.args[0] in ["Invalid IPv6 URL"]: - return StatusCanonicalCode.INVALID_ARGUMENT return StatusCanonicalCode.UNKNOWN From 6c1696c2f0e6ac6a750c54947eaef5f634cebb5b Mon Sep 17 00:00:00 2001 From: Bergutov Ruslan Date: Tue, 2 Jun 2020 20:44:56 +0500 Subject: [PATCH 4/4] lint fix --- .../src/opentelemetry/ext/requests/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py b/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py index 4befdfbc48a..65bc21370f6 100644 --- a/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py +++ b/ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py @@ -179,7 +179,8 @@ def _http_status_to_canonical_code(code: int, allow_redirect: bool = True): def _exception_to_canonical_code(exc: Exception) -> StatusCanonicalCode: if isinstance( - exc, (InvalidURL, InvalidSchema, MissingSchema, URLRequired, ValueError) + exc, + (InvalidURL, InvalidSchema, MissingSchema, URLRequired, ValueError), ): return StatusCanonicalCode.INVALID_ARGUMENT if isinstance(exc, Timeout):