diff --git a/.changelog/5448.fixed b/.changelog/5448.fixed new file mode 100644 index 00000000000..02f264466e9 --- /dev/null +++ b/.changelog/5448.fixed @@ -0,0 +1,4 @@ +`opentelemetry-exporter-otlp-proto-grpc`, +`opentelemetry-exporter-otlp-proto-http`: warn and fall back to the default +instead of raising `ValueError` when a timeout environment variable holds an +invalid value diff --git a/exporter/opentelemetry-exporter-otlp-proto-common/src/opentelemetry/exporter/otlp/proto/common/_internal/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-common/src/opentelemetry/exporter/otlp/proto/common/_internal/__init__.py index 7528b5994b8..60bd82e87f4 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-common/src/opentelemetry/exporter/otlp/proto/common/_internal/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-common/src/opentelemetry/exporter/otlp/proto/common/_internal/__init__.py @@ -6,9 +6,11 @@ import logging from collections.abc import Callable, Mapping, Sequence +from os import environ from typing import ( Any, TypeVar, + overload, ) from opentelemetry.proto.common.v1.common_pb2 import AnyValue as PB2AnyValue @@ -35,6 +37,40 @@ _ResourceDataT = TypeVar("_ResourceDataT") +@overload +def _timeout_from_env(*environ_keys: str, default: float) -> float: ... + + +@overload +def _timeout_from_env( + *environ_keys: str, default: None = None +) -> float | None: ... + + +def _timeout_from_env( + *environ_keys: str, default: float | None = None +) -> float | None: + """Return the first environment variable in ``environ_keys`` holding a + valid float, or ``default`` if none does. + + Unset or empty variables are skipped silently; variables with a + non-numeric value are skipped with a warning. + """ + for environ_key in environ_keys: + value = environ.get(environ_key) + if value is None or not value.strip(): + continue + try: + return float(value) + except ValueError: + _logger.warning( + "Invalid value %r for environment variable %s, ignoring it.", + value, + environ_key, + ) + return default + + def _encode_instrumentation_scope( instrumentation_scope: InstrumentationScope, ) -> PB2InstrumentationScope: diff --git a/exporter/opentelemetry-exporter-otlp-proto-common/tests/test_timeout_from_env.py b/exporter/opentelemetry-exporter-otlp-proto-common/tests/test_timeout_from_env.py new file mode 100644 index 00000000000..089a7e00699 --- /dev/null +++ b/exporter/opentelemetry-exporter-otlp-proto-common/tests/test_timeout_from_env.py @@ -0,0 +1,61 @@ +# Copyright The OpenTelemetry Authors +# SPDX-License-Identifier: Apache-2.0 + +import unittest +from logging import WARNING +from unittest.mock import patch + +from opentelemetry.exporter.otlp.proto.common._internal import ( + _timeout_from_env, +) + + +class TestTimeoutFromEnv(unittest.TestCase): + def test_simple_cases(self): + cases = [ + ("valid value", {"TEST_TIMEOUT": "15"}, 10, 15), + ("unset falls back to default", {}, 10, 10), + ("unset with no default returns None", {}, None, None), + ( + "empty/whitespace falls back to default", + {"TEST_TIMEOUT": " "}, + 10, + 10, + ), + ] + for name, env, default, expected in cases: + with self.subTest(name), patch.dict("os.environ", env, clear=True): + self.assertEqual( + _timeout_from_env("TEST_TIMEOUT", default=default), + expected, + ) + + @patch.dict("os.environ", {"TEST_TIMEOUT": "abc"}) + def test_invalid_value_warns_and_returns_default(self): + with self.assertLogs(level=WARNING) as warning: + self.assertEqual(_timeout_from_env("TEST_TIMEOUT", default=10), 10) + self.assertIn("Invalid value", warning.records[0].message) + self.assertIn("TEST_TIMEOUT", warning.records[0].message) + + @patch.dict( + "os.environ", {"TEST_SIGNAL_TIMEOUT": "5", "TEST_TIMEOUT": "15"} + ) + def test_first_key_takes_priority(self): + self.assertEqual( + _timeout_from_env( + "TEST_SIGNAL_TIMEOUT", "TEST_TIMEOUT", default=10 + ), + 5, + ) + + @patch.dict( + "os.environ", {"TEST_SIGNAL_TIMEOUT": "abc", "TEST_TIMEOUT": "15"} + ) + def test_invalid_first_key_falls_back_to_next(self): + with self.assertLogs(level=WARNING): + self.assertEqual( + _timeout_from_env( + "TEST_SIGNAL_TIMEOUT", "TEST_TIMEOUT", default=10 + ), + 15, + ) diff --git a/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/_log_exporter/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/_log_exporter/__init__.py index 7f86fe8fa61..c3451456e2c 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/_log_exporter/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/_log_exporter/__init__.py @@ -7,6 +7,9 @@ from typing import Literal from grpc import ChannelCredentials, Compression, StatusCode +from opentelemetry.exporter.otlp.proto.common._internal import ( + _timeout_from_env, +) from opentelemetry.exporter.otlp.proto.common._log_encoder import encode_logs from opentelemetry.exporter.otlp.proto.grpc.exporter import ( OTLPExporterMixin, @@ -82,10 +85,7 @@ def __init__( OTEL_EXPORTER_OTLP_LOGS_CLIENT_CERTIFICATE, ) - environ_timeout = environ.get(OTEL_EXPORTER_OTLP_LOGS_TIMEOUT) - environ_timeout = ( - float(environ_timeout) if environ_timeout is not None else None - ) + environ_timeout = _timeout_from_env(OTEL_EXPORTER_OTLP_LOGS_TIMEOUT) compression = ( environ_to_compression(OTEL_EXPORTER_OTLP_LOGS_COMPRESSION) @@ -99,7 +99,7 @@ def __init__( insecure=insecure, credentials=credentials, headers=headers or environ.get(OTEL_EXPORTER_OTLP_LOGS_HEADERS), - timeout=timeout or environ_timeout, + timeout=timeout if timeout is not None else environ_timeout, compression=compression, stub=LogsServiceStub, result=LogRecordExportResult, diff --git a/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py b/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py index f7fa0b8697d..f04b9517180 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py +++ b/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py @@ -49,6 +49,7 @@ ) from opentelemetry.exporter.otlp.proto.common._internal import ( _get_resource_data, + _timeout_from_env, ) from opentelemetry.exporter.otlp.proto.grpc import ( _OTLP_GRPC_CHANNEL_OPTIONS, @@ -350,8 +351,10 @@ def __init__( else: self._channel_options = tuple(_OTLP_GRPC_CHANNEL_OPTIONS) - self._timeout = timeout or float( - environ.get(OTEL_EXPORTER_OTLP_TIMEOUT, 10) + self._timeout = ( + timeout + if timeout is not None + else _timeout_from_env(OTEL_EXPORTER_OTLP_TIMEOUT, default=10) ) self._collector_kwargs = None diff --git a/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/metric_exporter/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/metric_exporter/__init__.py index de97551f9fe..b33f61e658f 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/metric_exporter/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/metric_exporter/__init__.py @@ -10,6 +10,9 @@ from os import environ from grpc import ChannelCredentials, Compression, StatusCode +from opentelemetry.exporter.otlp.proto.common._internal import ( + _timeout_from_env, +) from opentelemetry.exporter.otlp.proto.common._internal.metrics_encoder import ( OTLPMetricExporterMixin, ) @@ -125,10 +128,7 @@ def __init__( OTEL_EXPORTER_OTLP_METRICS_CLIENT_CERTIFICATE, ) - environ_timeout = environ.get(OTEL_EXPORTER_OTLP_METRICS_TIMEOUT) - environ_timeout = ( - float(environ_timeout) if environ_timeout is not None else None - ) + environ_timeout = _timeout_from_env(OTEL_EXPORTER_OTLP_METRICS_TIMEOUT) compression = ( environ_to_compression(OTEL_EXPORTER_OTLP_METRICS_COMPRESSION) @@ -149,7 +149,7 @@ def __init__( insecure=insecure, credentials=credentials, headers=headers or environ.get(OTEL_EXPORTER_OTLP_METRICS_HEADERS), - timeout=timeout or environ_timeout, + timeout=timeout if timeout is not None else environ_timeout, compression=compression, channel_options=channel_options, retryable_error_codes=retryable_error_codes, diff --git a/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/trace_exporter/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/trace_exporter/__init__.py index 7d396c17a80..e59dd0f3cf6 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/trace_exporter/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/trace_exporter/__init__.py @@ -9,6 +9,9 @@ from os import environ from grpc import ChannelCredentials, Compression, StatusCode +from opentelemetry.exporter.otlp.proto.common._internal import ( + _timeout_from_env, +) from opentelemetry.exporter.otlp.proto.common.trace_encoder import ( encode_spans, ) @@ -110,10 +113,7 @@ def __init__( OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE, ) - environ_timeout = environ.get(OTEL_EXPORTER_OTLP_TRACES_TIMEOUT) - environ_timeout = ( - float(environ_timeout) if environ_timeout is not None else None - ) + environ_timeout = _timeout_from_env(OTEL_EXPORTER_OTLP_TRACES_TIMEOUT) compression = ( environ_to_compression(OTEL_EXPORTER_OTLP_TRACES_COMPRESSION) @@ -130,7 +130,7 @@ def __init__( insecure=insecure, credentials=credentials, headers=headers or environ.get(OTEL_EXPORTER_OTLP_TRACES_HEADERS), - timeout=timeout or environ_timeout, + timeout=timeout if timeout is not None else environ_timeout, compression=compression, channel_options=channel_options, retryable_error_codes=retryable_error_codes, diff --git a/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_exporter_mixin.py b/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_exporter_mixin.py index 50996264a09..aae5e84d69f 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_exporter_mixin.py +++ b/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_exporter_mixin.py @@ -45,6 +45,7 @@ from opentelemetry.sdk.environment_variables import ( _OTEL_PYTHON_EXPORTER_OTLP_GRPC_CREDENTIAL_PROVIDER, OTEL_EXPORTER_OTLP_COMPRESSION, + OTEL_EXPORTER_OTLP_TIMEOUT, OTEL_PYTHON_SDK_INTERNAL_METRICS_ENABLED, ) from opentelemetry.sdk.metrics import MeterProvider @@ -609,6 +610,21 @@ def test_timeout_set_correctly(self): self.assertEqual(mock_trace_service.num_requests, 2) self.assertAlmostEqual(after - before, 1.4, 1) + @patch.dict("os.environ", {OTEL_EXPORTER_OTLP_TIMEOUT: "invalid"}) + def test_invalid_timeout_from_env_uses_default(self): + # pylint: disable=protected-access + with self.assertLogs(level=WARNING) as warning: + exporter = OTLPSpanExporterForTesting(insecure=True) + self.assertEqual(exporter._timeout, 10) + self.assertIn("Invalid value", warning.records[0].message) + self.assertIn(OTEL_EXPORTER_OTLP_TIMEOUT, warning.records[0].message) + + @patch.dict("os.environ", {OTEL_EXPORTER_OTLP_TIMEOUT: "15"}) + def test_timeout_zero_takes_priority_over_env(self): + # pylint: disable=protected-access + exporter = OTLPSpanExporterForTesting(insecure=True, timeout=0) + self.assertEqual(exporter._timeout, 0) + def test_channel_options_set_correctly(self): """Test that gRPC channel options are set correctly for keepalive and reconnection""" # This test verifies that the channel is created with the right options diff --git a/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_trace_exporter.py b/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_trace_exporter.py index 64cd091a6c1..15aca3dad2a 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_trace_exporter.py +++ b/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_trace_exporter.py @@ -4,6 +4,7 @@ # pylint: disable=too-many-lines import os +from logging import WARNING from unittest import TestCase from unittest.mock import Mock, PropertyMock, patch @@ -179,6 +180,23 @@ def test_env_variables(self, mock_exporter_mixin): self.assertEqual(kwargs["compression"], Compression.Gzip) self.assertIsNone(kwargs["credentials"]) + @patch.dict( + "os.environ", + {OTEL_EXPORTER_OTLP_TRACES_TIMEOUT: "invalid"}, + ) + @patch( + "opentelemetry.exporter.otlp.proto.grpc.exporter.OTLPExporterMixin.__init__" + ) + def test_env_variables_invalid_timeout(self, mock_exporter_mixin): + with self.assertLogs(level=WARNING) as warning: + OTLPSpanExporter() + _, kwargs = mock_exporter_mixin.call_args_list[0] + self.assertIsNone(kwargs["timeout"]) + self.assertIn("Invalid value", warning.records[0].message) + self.assertIn( + OTEL_EXPORTER_OTLP_TRACES_TIMEOUT, warning.records[0].message + ) + @patch.dict( "os.environ", { diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py index c25ebec6756..f3344acceca 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py @@ -19,6 +19,9 @@ from opentelemetry.exporter.otlp.proto.common._exporter_metrics import ( create_exporter_metrics, ) +from opentelemetry.exporter.otlp.proto.common._internal import ( + _timeout_from_env, +) from opentelemetry.exporter.otlp.proto.common._log_encoder import encode_logs from opentelemetry.exporter.otlp.proto.http import ( _OTLP_HTTP_HEADERS, @@ -145,10 +148,13 @@ def __init__( self._headers = headers or parse_env_headers( headers_string, liberal=True ) - self._timeout = timeout or float( - environ.get( + self._timeout = ( + timeout + if timeout is not None + else _timeout_from_env( OTEL_EXPORTER_OTLP_LOGS_TIMEOUT, - environ.get(OTEL_EXPORTER_OTLP_TIMEOUT, DEFAULT_TIMEOUT), + OTEL_EXPORTER_OTLP_TIMEOUT, + default=DEFAULT_TIMEOUT, ) ) self._max_request_size = ( diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/metric_exporter/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/metric_exporter/__init__.py index 7020beb7f32..1fee232ccdf 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/metric_exporter/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/metric_exporter/__init__.py @@ -27,6 +27,7 @@ ) from opentelemetry.exporter.otlp.proto.common._internal import ( _get_resource_data, + _timeout_from_env, ) from opentelemetry.exporter.otlp.proto.common._internal.metrics_encoder import ( OTLPMetricExporterMixin, @@ -189,10 +190,13 @@ def __init__( self._headers = headers or parse_env_headers( headers_string, liberal=True ) - self._timeout = timeout or float( - environ.get( + self._timeout = ( + timeout + if timeout is not None + else _timeout_from_env( OTEL_EXPORTER_OTLP_METRICS_TIMEOUT, - environ.get(OTEL_EXPORTER_OTLP_TIMEOUT, DEFAULT_TIMEOUT), + OTEL_EXPORTER_OTLP_TIMEOUT, + default=DEFAULT_TIMEOUT, ) ) self._compression = compression or _compression_from_env() diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/trace_exporter/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/trace_exporter/__init__.py index 56d0a92a9e6..625c3983deb 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/trace_exporter/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/trace_exporter/__init__.py @@ -19,6 +19,9 @@ from opentelemetry.exporter.otlp.proto.common._exporter_metrics import ( create_exporter_metrics, ) +from opentelemetry.exporter.otlp.proto.common._internal import ( + _timeout_from_env, +) from opentelemetry.exporter.otlp.proto.common.trace_encoder import ( encode_spans, ) @@ -140,10 +143,13 @@ def __init__( self._headers = headers or parse_env_headers( headers_string, liberal=True ) - self._timeout = timeout or float( - environ.get( + self._timeout = ( + timeout + if timeout is not None + else _timeout_from_env( OTEL_EXPORTER_OTLP_TRACES_TIMEOUT, - environ.get(OTEL_EXPORTER_OTLP_TIMEOUT, DEFAULT_TIMEOUT), + OTEL_EXPORTER_OTLP_TIMEOUT, + default=DEFAULT_TIMEOUT, ) ) self._max_request_size = ( diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_span_exporter.py b/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_span_exporter.py index fa8b5fc1f44..8164f4ade74 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_span_exporter.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_span_exporter.py @@ -248,6 +248,37 @@ def test_exporter_env(self): }, ) + @patch.dict( + "os.environ", + {OTEL_EXPORTER_OTLP_TIMEOUT: "invalid"}, + ) + def test_exporter_env_invalid_timeout(self): + with self.assertLogs(level=WARNING) as warning: + exporter = OTLPSpanExporter() + self.assertEqual(exporter._timeout, DEFAULT_TIMEOUT) + self.assertIn("Invalid value", warning.records[0].message) + self.assertIn(OTEL_EXPORTER_OTLP_TIMEOUT, warning.records[0].message) + + @patch.dict( + "os.environ", + { + OTEL_EXPORTER_OTLP_TRACES_TIMEOUT: "invalid", + OTEL_EXPORTER_OTLP_TIMEOUT: OS_ENV_TIMEOUT, + }, + ) + def test_exporter_env_invalid_traces_timeout_falls_back(self): + with self.assertLogs(level=WARNING): + exporter = OTLPSpanExporter() + self.assertEqual(exporter._timeout, float(OS_ENV_TIMEOUT)) + + @patch.dict( + "os.environ", + {OTEL_EXPORTER_OTLP_TIMEOUT: OS_ENV_TIMEOUT}, + ) + def test_exporter_constructor_timeout_zero_takes_priority(self): + exporter = OTLPSpanExporter(timeout=0) + self.assertEqual(exporter._timeout, 0) + @patch.dict( "os.environ", {OTEL_EXPORTER_OTLP_ENDPOINT: OS_ENV_ENDPOINT},