opentelemetry-instrumentation-urllib3: fix multiple arguments error (open-telemetry#4144)

* opentelemetry-instrumentation-urllib3: fix multiple arguments error

* update CHANGELOG.md

Author: herin049

CHANGELOG.md

@@ -102,6 +102,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#3729](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3729))
 - `opentelemetry-instrumentation-confluent-kafka`: Fix incorrect number of argument to `_inner_wrap_close`
   ([#3922](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3922))
+- `opentelemetry-instrumentation-urllib3`: fix multiple arguments error 
+  ([#4144](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/4144))
 
 ### Breaking changes
 

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

@@ -184,9 +184,11 @@ def response_hook(
 """
 
 import collections.abc
+import inspect
 import io
 import typing
 from dataclasses import dataclass
+from inspect import BoundArguments
 from timeit import default_timer
 from typing import Collection
 
@@ -287,12 +289,6 @@ class RequestInfo:
     ]
 ]
 
-_URL_OPEN_ARG_TO_INDEX_MAPPING = {
-    "method": 0,
-    "url": 1,
-    "body": 2,
-}
-
 
 class URLLib3Instrumentor(BaseInstrumentor):
     def instrumentation_dependencies(self) -> Collection[str]:
@@ -428,6 +424,7 @@ def _get_span_name(method: str) -> str:
     return method
 
 
+# pylint: disable=too-many-locals
 def _instrument(
     tracer: Tracer,
     duration_histogram_old: Histogram,
@@ -445,18 +442,31 @@ def _instrument(
     captured_response_headers: typing.Optional[list[str]] = None,
     sensitive_headers: typing.Optional[list[str]] = None,
 ):
-    # pylint: disable=too-many-locals
+    urlopen_signature = inspect.signature(
+        urllib3.connectionpool.HTTPConnectionPool.urlopen
+    )
+
     def instrumented_urlopen(wrapped, instance, args, kwargs):
         if not is_http_instrumentation_enabled():
             return wrapped(*args, **kwargs)
 
-        url = _get_url(instance, args, kwargs, url_filter)
+        try:
+            bound_args = urlopen_signature.bind(instance, *args, **kwargs)
+        except TypeError:
+            return wrapped(*args, **kwargs)
+
+        bound_args.apply_defaults()
+
+        method = bound_args.arguments.get("method").upper()
+        headers = bound_args.arguments.get("headers")
+        body = bound_args.arguments.get("body")
+        url = _get_url(instance, bound_args, url_filter)
+
         if excluded_urls and excluded_urls.url_disabled(url):
             return wrapped(*args, **kwargs)
 
-        method = _get_url_open_arg("method", args, kwargs).upper()
-        headers = _prepare_headers(kwargs)
-        body = _get_url_open_arg("body", args, kwargs)
+        # avoid modifying original headers on inject
+        headers = headers.copy() if headers is not None else {}
 
         span_name = _get_span_name(method)
         span_attributes = {}
@@ -496,10 +506,12 @@ def instrumented_urlopen(wrapped, instance, args, kwargs):
                     ),
                 )
             inject(headers)
+            bound_args.arguments["headers"] = headers
+
             # TODO: add error handling to also set exception `error.type` in new semconv
             with suppress_http_instrumentation():
                 start_time = default_timer()
-                response = wrapped(*args, **kwargs)
+                response = wrapped(*bound_args.args[1:], **bound_args.kwargs)
                 duration_s = default_timer() - start_time
             # set http status code based on semconv
             metric_attributes = {}
@@ -554,23 +566,12 @@ def instrumented_urlopen(wrapped, instance, args, kwargs):
     )
 
 
-def _get_url_open_arg(name: str, args: typing.List, kwargs: typing.Mapping):
-    arg_idx = _URL_OPEN_ARG_TO_INDEX_MAPPING.get(name)
-    if arg_idx is not None:
-        try:
-            return args[arg_idx]
-        except IndexError:
-            pass
-    return kwargs.get(name)
-
-
 def _get_url(
     instance: urllib3.connectionpool.HTTPConnectionPool,
-    args: typing.List,
-    kwargs: typing.Mapping,
+    bound_args: BoundArguments,
     url_filter: _UrlFilterT,
 ) -> str:
-    url_or_path = _get_url_open_arg("url", args, kwargs)
+    url_or_path = bound_args.arguments.get("url")
     if not url_or_path.startswith("/"):
         url = url_or_path
     else:
@@ -587,6 +588,7 @@ def _get_url(
 def _get_body_size(body: object) -> typing.Optional[int]:
     if body is None:
         return 0
+    # pylint: disable-next=no-member
     if isinstance(body, collections.abc.Sized):
         return len(body)
     if isinstance(body, io.BytesIO):
@@ -604,16 +606,6 @@ def _should_append_port(scheme: str, port: typing.Optional[int]) -> bool:
     return True
 
 
-def _prepare_headers(urlopen_kwargs: typing.Dict) -> typing.Dict:
-    headers = urlopen_kwargs.get("headers")
-
-    # avoid modifying original headers on inject
-    headers = headers.copy() if headers is not None else {}
-    urlopen_kwargs["headers"] = headers
-
-    return headers
-
-
 def _set_status_code_attribute(
     span: Span,
     status_code: int,

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

@@ -42,7 +42,7 @@
 from opentelemetry.trace import Span
 from opentelemetry.util.http import get_excluded_urls
 
-# pylint: disable=too-many-public-methods
+# pylint: disable=too-many-public-methods,too-many-lines
 
 
 class TestURLLib3Instrumentor(TestBase):
@@ -951,3 +951,54 @@ def test_capture_and_sanitize_environment_variables(self):
             "http.response.header.x_response_two",
             span.attributes,
         )
+
+    def test_urlopen_positional_headers(self):
+        URLLib3Instrumentor().uninstrument()
+        URLLib3Instrumentor().instrument(captured_request_headers=["X-Test"])
+        url = "http://mock/status/200"
+        httpretty.register_uri(httpretty.GET, url, body="Hello!")
+        pool = urllib3.HTTPConnectionPool("mock")
+        headers = {"X-Test": "Value"}
+        response = pool.urlopen("GET", "/status/200", None, headers)
+        self.assertEqual(b"Hello!", response.data)
+        span = self.assert_span()
+        self.assertEqual(
+            span.attributes["http.request.header.x_test"], ("Value",)
+        )
+
+    def test_urlopen_all_positional(self):
+        URLLib3Instrumentor().uninstrument()
+        URLLib3Instrumentor().instrument(captured_request_headers=["X-Test"])
+        url = "http://mock/status/200"
+        httpretty.register_uri(httpretty.GET, url, body="Hello!")
+        pool = urllib3.HTTPConnectionPool("mock")
+        response = pool.urlopen(
+            "GET",
+            "/status/200",
+            None,  # body
+            {"X-Test": "Value"},  # headers
+            None,  # retries
+            True,  # redirect
+            True,  # assert_same_host
+            urllib3.util.Timeout(5),  # timeout
+        )
+        self.assertEqual(b"Hello!", response.data)
+        span = self.assert_span()
+        self.assertEqual(
+            span.attributes["http.request.header.x_test"], ("Value",)
+        )
+
+    def test_urlopen_mixed_args(self):
+        URLLib3Instrumentor().uninstrument()
+        URLLib3Instrumentor().instrument(captured_request_headers=["X-Test"])
+        url = "http://mock/status/200"
+        httpretty.register_uri(httpretty.GET, url, body="Hello!")
+        pool = urllib3.HTTPConnectionPool("mock")
+        response = pool.urlopen(
+            "GET", "/status/200", headers={"X-Test": "Value"}, body=None
+        )
+        self.assertEqual(b"Hello!", response.data)
+        span = self.assert_span()
+        self.assertEqual(
+            span.attributes["http.request.header.x_test"], ("Value",)
+        )