From 67d736bd2f82c024b4a057899a3d5a0f2686b143 Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Mon, 18 Nov 2024 19:30:18 -0300 Subject: [PATCH 1/6] fix #3020 Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com> --- CHANGELOG.md | 2 + .../instrumentation/httpx/__init__.py | 46 ++++++++-------- .../tests/test_httpx_integration.py | 53 ++++++++++++++++--- 3 files changed, 73 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 772384bdde..3c323aa734 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,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 + ([#3003](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3003)) ### 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..f108222965 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 transport is not None: + 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 transport is not None: + 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..0b79bd2572 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py @@ -725,7 +725,6 @@ def test_client_mounts_with_instrumented_transport(self): spans[1].attributes[SpanAttributes.HTTP_URL], https_url ) - @mock.patch.dict("os.environ", {"NO_PROXY": ""}, clear=True) class BaseInstrumentorTest(BaseTest, metaclass=abc.ABCMeta): @abc.abstractmethod def create_client( @@ -741,6 +740,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() @@ -769,11 +772,9 @@ def assert_proxy_mounts(self, mounts, num_mounts, transport_type=None): transport_type, ) else: - handler = getattr(transport, "handle_request", None) - if not handler: - handler = getattr( - transport, "handle_async_request" - ) + if transport is None: + continue + handler = self.get_transport_handler(transport) self.assertTrue( isinstance(handler, ObjectProxy) and getattr(handler, "__wrapped__") @@ -983,6 +984,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) + self.perform_request(self.URL, client=client) + self.assert_span(num_spans=1) + print(client._mounts) + self.assert_proxy_mounts( + client._mounts.values(), + 3, + ) + def test_instrument_proxy(self): proxy_mounts = self.create_proxy_mounts() HTTPXClientInstrumentor().instrument() @@ -994,6 +1010,25 @@ def test_instrument_proxy(self): 2, ) + @mock.patch.dict("os.environ", {"NO_PROXY": "http://mock"}, 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, type(None)), + ) + 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 +1223,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 +1279,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 From 4574a4955780b343755a7642f41eed06d10e9433 Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Mon, 18 Nov 2024 19:35:14 -0300 Subject: [PATCH 2/6] skip none in test too Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com> --- .../tests/test_httpx_integration.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py index 0b79bd2572..b8c678288e 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py @@ -766,14 +766,14 @@ 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: - if transport is None: - continue handler = self.get_transport_handler(transport) self.assertTrue( isinstance(handler, ObjectProxy) @@ -1017,7 +1017,7 @@ def test_instrument_client_with_no_proxy(self): self.assert_proxy_mounts( client._mounts.values(), 3, - (httpx.HTTPTransport, httpx.AsyncHTTPTransport, type(None)), + (httpx.HTTPTransport, httpx.AsyncHTTPTransport), ) HTTPXClientInstrumentor.instrument_client(client) result = self.perform_request(self.URL, client=client) From 27f06e9c359593b6d27563333334bff646b6e07f Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Mon, 18 Nov 2024 19:38:11 -0300 Subject: [PATCH 3/6] improve logic Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com> --- .../src/opentelemetry/instrumentation/httpx/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 f108222965..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,7 +1005,7 @@ def instrument_client( ), ) for transport in client._mounts.values(): - if transport is not None: + if hasattr(transport, "handle_request"): wrap_function_wrapper( transport, "handle_request", @@ -1031,7 +1031,7 @@ def instrument_client( ), ) for transport in client._mounts.values(): - if transport is not None: + if hasattr(transport, "handle_async_request"): wrap_function_wrapper( transport, "handle_async_request", From 4c583fe64c1577589d0d593cd2342a53370dc6e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Em=C3=ADdio=20Neto?= <9735060+emdneto@users.noreply.github.com> Date: Mon, 18 Nov 2024 19:46:55 -0300 Subject: [PATCH 4/6] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c323aa734..4448dec06c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,7 @@ 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 - ([#3003](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3003)) + ([#3022](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3022)) ### Breaking changes From b47b39e30af9f97ff99b0b514a5da12ce7de4a13 Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Mon, 18 Nov 2024 21:12:13 -0300 Subject: [PATCH 5/6] assert text Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com> --- .../tests/test_httpx_integration.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py index b8c678288e..8eddc61abd 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py @@ -991,8 +991,9 @@ def test_instrument_with_no_proxy(self): proxy_mounts = self.create_proxy_mounts() HTTPXClientInstrumentor().instrument() client = self.create_client(mounts=proxy_mounts) - self.perform_request(self.URL, client=client) + result = self.perform_request(self.URL, client=client) self.assert_span(num_spans=1) + self.assertEqual(result.text, "Hello!") print(client._mounts) self.assert_proxy_mounts( client._mounts.values(), @@ -1010,7 +1011,9 @@ def test_instrument_proxy(self): 2, ) - @mock.patch.dict("os.environ", {"NO_PROXY": "http://mock"}, clear=True) + @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) @@ -1027,7 +1030,7 @@ def test_instrument_client_with_no_proxy(self): client._mounts.values(), 3, ) - HTTPXClientInstrumentor().uninstrument_client(client) + HTTPXClientInstrumentor.uninstrument_client(client) def test_instrument_client_with_proxy(self): proxy_mounts = self.create_proxy_mounts() From 18f54fd02e9d8cf128a1390218628434e4cd3d76 Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Tue, 19 Nov 2024 09:09:53 -0300 Subject: [PATCH 6/6] revert first mock Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com> --- .../tests/test_httpx_integration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py index 8eddc61abd..0fdab381d5 100644 --- a/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py +++ b/instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py @@ -725,6 +725,7 @@ def test_client_mounts_with_instrumented_transport(self): spans[1].attributes[SpanAttributes.HTTP_URL], https_url ) + @mock.patch.dict("os.environ", {"NO_PROXY": ""}, clear=True) class BaseInstrumentorTest(BaseTest, metaclass=abc.ABCMeta): @abc.abstractmethod def create_client( @@ -994,7 +995,6 @@ def test_instrument_with_no_proxy(self): result = self.perform_request(self.URL, client=client) self.assert_span(num_spans=1) self.assertEqual(result.text, "Hello!") - print(client._mounts) self.assert_proxy_mounts( client._mounts.values(), 3,