From 19a59e4be73301140903826a445e27614435112c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Em=C3=ADdio=20Neto?= <9735060+emdneto@users.noreply.github.com> Date: Tue, 19 Nov 2024 16:40:06 -0300 Subject: [PATCH] fix(httpx): check if mount transport is None before wrap (#3022) --- CHANGELOG.md | 2 + .../instrumentation/httpx/__init__.py | 46 ++++++++-------- .../tests/test_httpx_integration.py | 54 +++++++++++++++++-- 3 files changed, 75 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd686f6089..92e366402a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `opentelemetry-instrumentation-httpx`: instrument_client is a static method again ([#3003](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3003)) +- `opentelemetry-instrumentation-httpx`: Check if mount transport is none before wrap it + ([#3022](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3022)) ### Breaking changes diff --git a/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py b/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py index 4a2026e6de..92e044bf06 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py @@ -1005,17 +1005,18 @@ def instrument_client( ), ) for transport in client._mounts.values(): - wrap_function_wrapper( - transport, - "handle_request", - partial( - cls._handle_request_wrapper, - tracer=tracer, - sem_conv_opt_in_mode=sem_conv_opt_in_mode, - request_hook=request_hook, - response_hook=response_hook, - ), - ) + if hasattr(transport, "handle_request"): + wrap_function_wrapper( + transport, + "handle_request", + partial( + cls._handle_request_wrapper, + tracer=tracer, + sem_conv_opt_in_mode=sem_conv_opt_in_mode, + request_hook=request_hook, + response_hook=response_hook, + ), + ) client._is_instrumented_by_opentelemetry = True if hasattr(client._transport, "handle_async_request"): wrap_function_wrapper( @@ -1030,17 +1031,18 @@ def instrument_client( ), ) for transport in client._mounts.values(): - wrap_function_wrapper( - transport, - "handle_async_request", - partial( - cls._handle_async_request_wrapper, - tracer=tracer, - sem_conv_opt_in_mode=sem_conv_opt_in_mode, - async_request_hook=async_request_hook, - async_response_hook=async_response_hook, - ), - ) + if hasattr(transport, "handle_async_request"): + wrap_function_wrapper( + transport, + "handle_async_request", + partial( + cls._handle_async_request_wrapper, + tracer=tracer, + sem_conv_opt_in_mode=sem_conv_opt_in_mode, + async_request_hook=async_request_hook, + async_response_hook=async_response_hook, + ), + ) client._is_instrumented_by_opentelemetry = True @staticmethod diff --git a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py index b934ae0861..0fdab381d5 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py @@ -741,6 +741,10 @@ def create_client( def create_proxy_transport(self, url: str): pass + @abc.abstractmethod + def get_transport_handler(self, transport): + pass + def setUp(self): super().setUp() self.client = self.create_client() @@ -763,17 +767,15 @@ def assert_proxy_mounts(self, mounts, num_mounts, transport_type=None): self.assertEqual(len(mounts), num_mounts) for transport in mounts: with self.subTest(transport): + if transport is None: + continue if transport_type: self.assertIsInstance( transport, transport_type, ) else: - handler = getattr(transport, "handle_request", None) - if not handler: - handler = getattr( - transport, "handle_async_request" - ) + handler = self.get_transport_handler(transport) self.assertTrue( isinstance(handler, ObjectProxy) and getattr(handler, "__wrapped__") @@ -983,6 +985,21 @@ def test_uninstrument_new_client(self): self.assertEqual(result.text, "Hello!") self.assert_span() + @mock.patch.dict( + "os.environ", {"NO_PROXY": "http://mock/status/200"}, clear=True + ) + def test_instrument_with_no_proxy(self): + proxy_mounts = self.create_proxy_mounts() + HTTPXClientInstrumentor().instrument() + client = self.create_client(mounts=proxy_mounts) + result = self.perform_request(self.URL, client=client) + self.assert_span(num_spans=1) + self.assertEqual(result.text, "Hello!") + self.assert_proxy_mounts( + client._mounts.values(), + 3, + ) + def test_instrument_proxy(self): proxy_mounts = self.create_proxy_mounts() HTTPXClientInstrumentor().instrument() @@ -994,6 +1011,27 @@ def test_instrument_proxy(self): 2, ) + @mock.patch.dict( + "os.environ", {"NO_PROXY": "http://mock/status/200"}, clear=True + ) + def test_instrument_client_with_no_proxy(self): + proxy_mounts = self.create_proxy_mounts() + client = self.create_client(mounts=proxy_mounts) + self.assert_proxy_mounts( + client._mounts.values(), + 3, + (httpx.HTTPTransport, httpx.AsyncHTTPTransport), + ) + HTTPXClientInstrumentor.instrument_client(client) + result = self.perform_request(self.URL, client=client) + self.assertEqual(result.text, "Hello!") + self.assert_span(num_spans=1) + self.assert_proxy_mounts( + client._mounts.values(), + 3, + ) + HTTPXClientInstrumentor.uninstrument_client(client) + def test_instrument_client_with_proxy(self): proxy_mounts = self.create_proxy_mounts() client = self.create_client(mounts=proxy_mounts) @@ -1188,6 +1226,9 @@ def perform_request( def create_proxy_transport(self, url): return httpx.HTTPTransport(proxy=httpx.Proxy(url)) + def get_transport_handler(self, transport): + return getattr(transport, "handle_request", None) + def test_can_instrument_subclassed_client(self): class CustomClient(httpx.Client): pass @@ -1241,6 +1282,9 @@ async def _perform_request(): def create_proxy_transport(self, url): return httpx.AsyncHTTPTransport(proxy=httpx.Proxy(url)) + def get_transport_handler(self, transport): + return getattr(transport, "handle_async_request", None) + def test_basic_multiple(self): # We need to create separate clients because in httpx >= 0.19, # closing the client after "with" means the second http call fails