diff --git a/.changelog/5711.fixed b/.changelog/5711.fixed new file mode 100644 index 00000000000..a01df70322f --- /dev/null +++ b/.changelog/5711.fixed @@ -0,0 +1 @@ +`opentelemetry-exporter-http-transport`: default to urllib3 transport only when requests-specific env vars are absent diff --git a/exporter/opentelemetry-exporter-http-transport/README.rst b/exporter/opentelemetry-exporter-http-transport/README.rst index dfbb1f723b1..2ddb2774a75 100644 --- a/exporter/opentelemetry-exporter-http-transport/README.rst +++ b/exporter/opentelemetry-exporter-http-transport/README.rst @@ -26,6 +26,16 @@ With the ``urllib3`` backend:: pip install opentelemetry-exporter-http-transport[urllib3] +Proxies and CA bundles +---------------------- + +The ``urllib3`` transport is the default. It ignores the ``HTTP_PROXY``, +``HTTPS_PROXY``, ``ALL_PROXY``, ``NO_PROXY``, ``REQUESTS_CA_BUNDLE`` and +``CURL_CA_BUNDLE`` environment variables, which only ``requests`` honors. If +any of them is set, exporters use the ``requests`` transport instead. If +``requests`` is not installed, a warning is logged and the ``urllib3`` +transport is used. + References ---------- diff --git a/exporter/opentelemetry-exporter-http-transport/src/opentelemetry/exporter/http/transport/__init__.py b/exporter/opentelemetry-exporter-http-transport/src/opentelemetry/exporter/http/transport/__init__.py index e701872883b..bbb6a7b036b 100644 --- a/exporter/opentelemetry-exporter-http-transport/src/opentelemetry/exporter/http/transport/__init__.py +++ b/exporter/opentelemetry-exporter-http-transport/src/opentelemetry/exporter/http/transport/__init__.py @@ -3,6 +3,8 @@ from __future__ import annotations +import logging +import os from typing import TYPE_CHECKING, cast # pylint: disable-next=import-error @@ -30,11 +32,26 @@ def __call__( ) -> BaseHTTPTransport: ... +_logger = logging.getLogger(__name__) + _KNOWN_TRANSPORTS: dict[str, BaseHTTPTransportFactory] = { "requests": _RequestsHTTPTransport, "urllib3": _Urllib3HTTPTransport, } +# Environment variables honored by ``requests`` but ignored by +# ``urllib3`` transport. Matched case-insensitively. +_REQUESTS_ENV_VARS = frozenset( + { + "HTTP_PROXY", + "HTTPS_PROXY", + "ALL_PROXY", + "NO_PROXY", + "REQUESTS_CA_BUNDLE", + "CURL_CA_BUNDLE", + } +) + def _load_http_transport_factory(name: str) -> BaseHTTPTransportFactory: """Return the transport factory registered under *name*. @@ -71,3 +88,30 @@ def _load_http_transport_factory(name: str) -> BaseHTTPTransportFactory: if not callable(factory): raise TypeError(f"Transport {name!r} loaded from entry point is not callable (got {factory!r}).") return cast("BaseHTTPTransportFactory", factory) + + +def _get_default_http_transport_factory() -> BaseHTTPTransportFactory: + """Return the transport factory to use when none is configured. + + Defaults to the ``urllib3`` transport. If any environment variable that + only ``requests`` honors (proxy settings or CA bundle) is set, the + ``requests`` transport is returned instead so those settings keep working. + If ``requests`` is not installed, a warning is logged and the ``urllib3`` + transport is returned. + """ + detected = sorted(name for name, value in os.environ.items() if value and name.upper() in _REQUESTS_ENV_VARS) + if not detected: + return _Urllib3HTTPTransport + try: + # pylint: disable-next=import-outside-toplevel,unused-import + import requests # noqa: F401, PLC0415 + except ImportError: + _logger.warning( + "Environment variables %s are only honored by the 'requests' HTTP " + "transport, but 'requests' is not installed; falling back to the " + "'urllib3' transport, which ignores them. Install 'requests' to " + "use them.", + ", ".join(detected), + ) + return _Urllib3HTTPTransport + return _RequestsHTTPTransport diff --git a/exporter/opentelemetry-exporter-http-transport/tests/test_load_transport.py b/exporter/opentelemetry-exporter-http-transport/tests/test_load_transport.py index d4471bcf6dc..0ece9fce1bf 100644 --- a/exporter/opentelemetry-exporter-http-transport/tests/test_load_transport.py +++ b/exporter/opentelemetry-exporter-http-transport/tests/test_load_transport.py @@ -2,10 +2,16 @@ # SPDX-License-Identifier: Apache-2.0 # pylint: disable=import-error +import os +import sys import unittest +from logging import WARNING from unittest.mock import MagicMock, patch -from opentelemetry.exporter.http.transport import _load_http_transport_factory +from opentelemetry.exporter.http.transport import ( + _get_default_http_transport_factory, + _load_http_transport_factory, +) from opentelemetry.exporter.http.transport._requests import ( RequestsHTTPTransport, ) @@ -53,3 +59,38 @@ def test_entry_point_non_callable_raises_type_error(self): def test_unknown_transport_raises_value_error(self): with patch(_ENTRY_POINTS_TARGET, return_value=[]): self.assertRaises(ValueError, _load_http_transport_factory, "nonexistent") + + +class TestGetDefaultHTTPTransportFactory(unittest.TestCase): + @patch.dict(os.environ, {}, clear=True) + def test_returns_urllib3_without_requests_env_vars(self): + self.assertIs(_get_default_http_transport_factory(), Urllib3HTTPTransport) + + def test_returns_requests_when_requests_env_var_set(self): + names = [ + "HTTP_PROXY", + "HTTPS_PROXY", + "ALL_PROXY", + "NO_PROXY", + "REQUESTS_CA_BUNDLE", + "CURL_CA_BUNDLE", + ] + for name in names + [name.lower() for name in names]: + with self.subTest(name=name), patch.dict(os.environ, {name: "value"}, clear=True): + self.assertIs(_get_default_http_transport_factory(), RequestsHTTPTransport) + + @patch.dict(os.environ, {"HTTPS_PROXY": ""}, clear=True) + def test_ignores_empty_env_var(self): + self.assertIs(_get_default_http_transport_factory(), Urllib3HTTPTransport) + + @patch.dict(os.environ, {"OTHER_PROXY": "http://proxy:3128"}, clear=True) + def test_ignores_unrelated_env_var(self): + self.assertIs(_get_default_http_transport_factory(), Urllib3HTTPTransport) + + @patch.dict(os.environ, {"HTTPS_PROXY": "http://user:secret@proxy:3128"}, clear=True) + @patch.dict(sys.modules, {"requests": None}) + def test_warns_and_returns_urllib3_when_requests_missing(self): + with self.assertLogs(level=WARNING) as logs: + self.assertIs(_get_default_http_transport_factory(), Urllib3HTTPTransport) + self.assertIn("HTTPS_PROXY", logs.output[0]) + self.assertNotIn("secret", logs.output[0]) diff --git a/exporter/opentelemetry-exporter-otlp-json-http/src/opentelemetry/exporter/otlp/json/http/_internal.py b/exporter/opentelemetry-exporter-otlp-json-http/src/opentelemetry/exporter/otlp/json/http/_internal.py index f90f0057940..33d0af5b705 100644 --- a/exporter/opentelemetry-exporter-otlp-json-http/src/opentelemetry/exporter/otlp/json/http/_internal.py +++ b/exporter/opentelemetry-exporter-otlp-json-http/src/opentelemetry/exporter/otlp/json/http/_internal.py @@ -8,7 +8,7 @@ from collections.abc import Mapping from typing import TYPE_CHECKING, Literal -from opentelemetry.exporter.http.transport._urllib3 import Urllib3HTTPTransport +from opentelemetry.exporter.http.transport import _get_default_http_transport_factory from opentelemetry.exporter.otlp.common.http import Compression from opentelemetry.exporter.otlp.json.http.version import __version__ from opentelemetry.sdk.environment_variables import ( @@ -105,7 +105,7 @@ def _build_transport( certificate_env_var: str, client_key_env_var: str, client_certificate_env_var: str, - transport_factory: BaseHTTPTransportFactory = Urllib3HTTPTransport, + transport_factory: BaseHTTPTransportFactory | None = None, ) -> BaseHTTPTransport: verify: bool | str = ( certificate_file @@ -129,6 +129,8 @@ def _build_transport( ) or os.environ.get(OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE) ) + if transport_factory is None: + transport_factory = _get_default_http_transport_factory() return transport_factory( verify=verify, cert=(client_certificate_file, client_key_file) diff --git a/exporter/opentelemetry-exporter-otlp-json-http/tests/test_internal.py b/exporter/opentelemetry-exporter-otlp-json-http/tests/test_internal.py index 0ab67182ad9..0d88796857b 100644 --- a/exporter/opentelemetry-exporter-otlp-json-http/tests/test_internal.py +++ b/exporter/opentelemetry-exporter-otlp-json-http/tests/test_internal.py @@ -4,6 +4,7 @@ # pylint: disable=protected-access import os +import sys import unittest from logging import WARNING from unittest.mock import MagicMock, patch @@ -308,6 +309,36 @@ def test_default_transport_factory_is_urllib3(self): ) self.assertIsInstance(result, Urllib3HTTPTransport) + @patch.dict(os.environ, {}, clear=True) + def test_default_transport_factory_is_resolved_per_call(self): + with patch( + "opentelemetry.exporter.otlp.json.http._internal._get_default_http_transport_factory" + ) as mock_get_factory: + result = _build_transport( + None, + None, + None, + OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE, + OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY, + OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE, + ) + mock_get_factory.return_value.assert_called_once_with(verify=True, cert=None) + self.assertIs(result, mock_get_factory.return_value.return_value) + + @patch.dict(os.environ, {"HTTPS_PROXY": "http://proxy:3128"}, clear=True) + @patch.dict(sys.modules, {"requests": None}) + def test_requests_env_var_without_requests_falls_back_to_urllib3(self): + with self.assertLogs(level=WARNING): + result = _build_transport( + None, + None, + None, + OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE, + OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY, + OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE, + ) + self.assertIsInstance(result, Urllib3HTTPTransport) + def test_build_transport(self): cases = [ ( diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py index 31bc650b6ca..33428e13d06 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py @@ -9,12 +9,12 @@ from os import environ from typing import TYPE_CHECKING, Literal +from opentelemetry.exporter.http.transport import ( + _get_default_http_transport_factory, +) from opentelemetry.exporter.http.transport._requests import ( RequestsHTTPTransport, ) -from opentelemetry.exporter.http.transport._urllib3 import ( - Urllib3HTTPTransport, -) from opentelemetry.exporter.otlp.common import http as _http from opentelemetry.exporter.otlp.proto.http import ( _OTLP_HTTP_HEADERS, @@ -212,8 +212,6 @@ def _build_transport( else client_certificate_file ) - return ( - RequestsHTTPTransport(verify=verify, cert=cert, session=session) - if session - else Urllib3HTTPTransport(verify=verify, cert=cert) - ) + if session: + return RequestsHTTPTransport(verify=verify, cert=cert, session=session) + return _get_default_http_transport_factory()(verify=verify, cert=cert) diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_common.py b/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_common.py index 88e4dc51fcc..7d8dd4dad2d 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_common.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_common.py @@ -367,6 +367,34 @@ def test_session_forces_requests_transport(self): # pylint: disable-next=protected-access self.assertIs(result._session, session) + @patch.dict(os.environ, {"HTTPS_PROXY": "http://proxy:3128"}, clear=True) + def test_requests_env_var_selects_requests_transport(self): + result = _build_transport( + None, + None, + None, + OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE, + OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY, + OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE, + session=None, + ) + self.assertIsInstance(result, RequestsHTTPTransport) + + @patch.dict(os.environ, {"HTTPS_PROXY": "http://proxy:3128"}, clear=True) + @patch.dict(sys.modules, {"requests": None}) + def test_requests_env_var_without_requests_falls_back_to_urllib3(self): + with self.assertLogs(level=WARNING): + result = _build_transport( + None, + None, + None, + OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE, + OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY, + OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE, + session=None, + ) + self.assertIsInstance(result, Urllib3HTTPTransport) + def test_build_transport_verify_and_cert(self): cases = [ ( @@ -454,7 +482,10 @@ def test_build_transport_verify_and_cert(self): expected_cert, ) in cases: with self.subTest(label), patch.dict(os.environ, env, clear=True): - with patch("opentelemetry.exporter.otlp.proto.http._common.Urllib3HTTPTransport") as mock_transport: + with patch( + "opentelemetry.exporter.otlp.proto.http._common._get_default_http_transport_factory" + ) as mock_get_factory: + mock_transport = mock_get_factory.return_value result = _build_transport( certificate_file, client_key_file,