feat(urllib3)!: refactor request hook parameters (open-telemetry#2711)

Author: eigenein

CHANGELOG.md

@@ -67,6 +67,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#2726](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2726))
 - `opentelemetry-instrumentation-fastapi` Add dependency support for fastapi-slim
   ([#2702](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2702))
+- `opentelemetry-instrumentation-urllib3` improve request_hook, replacing `headers` and `body` parameters with a single `request_info: RequestInfo` parameter that now contains the `method` and `url` ([#2711](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2711))
 
 ### Fixed
 

instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py

@@ -47,14 +47,19 @@ def strip_query_params(url: str) -> str:
 
 .. code:: python
 
-    # `request` is an instance of urllib3.connectionpool.HTTPConnectionPool
-    def request_hook(span, request):
-        pass
-
-    # `request` is an instance of urllib3.connectionpool.HTTPConnectionPool
-    # `response` is an instance of urllib3.response.HTTPResponse
-    def response_hook(span, request, response):
-        pass
+    def request_hook(
+        span: Span,
+        pool: urllib3.connectionpool.HTTPConnectionPool,
+        request_info: RequestInfo,
+    ) -> Any:
+        ...
+
+    def response_hook(
+        span: Span,
+        pool: urllib3.connectionpool.HTTPConnectionPool,
+        response: urllib3.response.HTTPResponse,
+    ) -> Any:
+        ...
 
     URLLib3Instrumentor().instrument(
         request_hook=request_hook, response_hook=response_hook
@@ -81,6 +86,7 @@ def response_hook(span, request, response):
 import collections.abc
 import io
 import typing
+from dataclasses import dataclass
 from timeit import default_timer
 from typing import Collection
 
@@ -135,14 +141,29 @@ def response_hook(span, request, response):
 
 _excluded_urls_from_env = get_excluded_urls("URLLIB3")
 
+
+@dataclass
+class RequestInfo:
+    """Arguments that were passed to the ``urlopen()`` call."""
+
+    __slots__ = ("method", "url", "headers", "body")
+
+    # The type annotations here come from ``HTTPConnectionPool.urlopen()``.
+    method: str
+    url: str
+    headers: typing.Optional[typing.Mapping[str, str]]
+    body: typing.Union[
+        bytes, typing.IO[typing.Any], typing.Iterable[bytes], str, None
+    ]
+
+
 _UrlFilterT = typing.Optional[typing.Callable[[str], str]]
 _RequestHookT = typing.Optional[
     typing.Callable[
         [
             Span,
             urllib3.connectionpool.HTTPConnectionPool,
-            typing.Dict,
-            typing.Optional[str],
+            RequestInfo,
         ],
         None,
     ]
@@ -317,7 +338,16 @@ def instrumented_urlopen(wrapped, instance, args, kwargs):
             span_name, kind=SpanKind.CLIENT, attributes=span_attributes
         ) as span, set_ip_on_next_http_connection(span):
             if callable(request_hook):
-                request_hook(span, instance, headers, body)
+                request_hook(
+                    span,
+                    instance,
+                    RequestInfo(
+                        method=method,
+                        url=url,
+                        headers=headers,
+                        body=body,
+                    ),
+                )
             inject(headers)
             # TODO: add error handling to also set exception `error.type` in new semconv
             with suppress_http_instrumentation():

instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py

@@ -27,7 +27,10 @@
     _HTTPStabilityMode,
     _OpenTelemetrySemanticConventionStability,
 )
-from opentelemetry.instrumentation.urllib3 import URLLib3Instrumentor
+from opentelemetry.instrumentation.urllib3 import (
+    RequestInfo,
+    URLLib3Instrumentor,
+)
 from opentelemetry.instrumentation.utils import (
     suppress_http_instrumentation,
     suppress_instrumentation,
@@ -42,6 +45,7 @@
 from opentelemetry.semconv.trace import SpanAttributes
 from opentelemetry.test.mock_textmap import MockTextMapPropagator
 from opentelemetry.test.test_base import TestBase
+from opentelemetry.trace import Span
 from opentelemetry.util.http import get_excluded_urls
 
 # pylint: disable=too-many-public-methods
@@ -521,10 +525,10 @@ def test_credential_removal(self):
         self.assert_success_span(response, self.HTTP_URL)
 
     def test_hooks(self):
-        def request_hook(span, request, body, headers):
+        def request_hook(span, pool, request_info):
             span.update_name("name set from hook")
 
-        def response_hook(span, request, response):
+        def response_hook(span, pool, response):
             span.set_attribute("response_hook_attr", "value")
 
         URLLib3Instrumentor().uninstrument()
@@ -541,11 +545,17 @@ def response_hook(span, request, response):
         self.assertEqual(span.attributes["response_hook_attr"], "value")
 
     def test_request_hook_params(self):
-        def request_hook(span, request, headers, body):
+        def request_hook(
+            span: Span,
+            _pool: urllib3.connectionpool.ConnectionPool,
+            request_info: RequestInfo,
+        ) -> None:
+            span.set_attribute("request_hook_method", request_info.method)
+            span.set_attribute("request_hook_url", request_info.url)
             span.set_attribute(
-                "request_hook_headers", json.dumps(dict(headers))
+                "request_hook_headers", json.dumps(dict(request_info.headers))
             )
-            span.set_attribute("request_hook_body", body)
+            span.set_attribute("request_hook_body", request_info.body)
 
         URLLib3Instrumentor().uninstrument()
         URLLib3Instrumentor().instrument(
@@ -564,6 +574,10 @@ def request_hook(span, request, headers, body):
 
         span = self.assert_span()
 
+        self.assertEqual(span.attributes["request_hook_method"], "POST")
+        self.assertEqual(
+            span.attributes["request_hook_url"], "http://mock/status/200"
+        )
         self.assertIn("request_hook_headers", span.attributes)
         self.assertEqual(
             span.attributes["request_hook_headers"], json.dumps(headers)
@@ -572,8 +586,12 @@ def request_hook(span, request, headers, body):
         self.assertEqual(span.attributes["request_hook_body"], body)
 
     def test_request_positional_body(self):
-        def request_hook(span, request, headers, body):
-            span.set_attribute("request_hook_body", body)
+        def request_hook(
+            span: Span,
+            _pool: urllib3.connectionpool.ConnectionPool,
+            request_info: RequestInfo,
+        ) -> None:
+            span.set_attribute("request_hook_body", request_info.body)
 
         URLLib3Instrumentor().uninstrument()
         URLLib3Instrumentor().instrument(