fix(zipkin): tolerate invalid OTEL_EXPORTER_ZIPKIN_TIMEOUT env var

.changelog/5704.fixed

@@ -0,0 +1 @@
+`opentelemetry-exporter-zipkin-json`: warn and fall back to the default instead of raising `ValueError` when `OTEL_EXPORTER_ZIPKIN_TIMEOUT` holds a non-integer value; the same fix is applied to `opentelemetry-exporter-zipkin-proto-http`.

exporter/opentelemetry-exporter-zipkin-json/src/opentelemetry/exporter/zipkin/json/__init__.py

@@ -81,10 +81,27 @@
 
 DEFAULT_ENDPOINT = "http://localhost:9411/api/v2/spans"
 REQUESTS_SUCCESS_STATUS_CODES = (200, 202)
+_DEFAULT_TIMEOUT = 10
 
 logger = logging.getLogger(__name__)
 
 
+def _timeout_from_env() -> int:
+    value = environ.get(OTEL_EXPORTER_ZIPKIN_TIMEOUT)
+    if value is None or not value.strip():
+        return _DEFAULT_TIMEOUT
+    try:
+        return int(value)
+    except ValueError:
+        logger.warning(
+            "Invalid value %r for %s, using default of %s seconds",
+            value,
+            OTEL_EXPORTER_ZIPKIN_TIMEOUT,
+            _DEFAULT_TIMEOUT,
+        )
+        return _DEFAULT_TIMEOUT
+
+
 class ZipkinExporter(SpanExporter):
     def __init__(
         self,
@@ -128,7 +145,7 @@ def __init__(
         self.session = session or requests.Session()
         self.session.headers.update({"Content-Type": self.encoder.content_type()})
         self._closed = False
-        self.timeout = timeout or int(environ.get(OTEL_EXPORTER_ZIPKIN_TIMEOUT, 10))
+        self.timeout = timeout or _timeout_from_env()
 
     def export(self, spans: Sequence[Span]) -> SpanExportResult:
         # After the call to Shutdown subsequent calls to Export are

exporter/opentelemetry-exporter-zipkin-json/tests/test_zipkin_exporter.py

@@ -63,6 +63,52 @@ def test_constructor_env_vars(self):
         self.assertEqual(exporter.local_node.port, None)
         self.assertEqual(exporter.timeout, 15)
 
+    def test_constructor_invalid_timeout_env_var_falls_back(self):
+        with patch.dict(os.environ, {OTEL_EXPORTER_ZIPKIN_TIMEOUT: "10s"}):
+            with self.assertLogs("opentelemetry.exporter.zipkin.json", level="WARNING") as cm:
+                exporter = ZipkinExporter()
+
+            self.assertEqual(exporter.timeout, 10)
+            self.assertEqual(len(cm.records), 1)
+            self.assertTrue(
+                any("Invalid value" in message for message in cm.output),
+                cm.output,
+            )
+
+    def test_constructor_empty_timeout_env_var_falls_back(self):
+        for value in ("", " ", "\t\n"):
+            with self.subTest(value=value), patch.dict(os.environ, {OTEL_EXPORTER_ZIPKIN_TIMEOUT: value}):
+                with self.assertNoLogs("opentelemetry.exporter.zipkin.json", level="WARNING"):
+                    exporter = ZipkinExporter()
+                self.assertEqual(exporter.timeout, 10)
+
+    def test_constructor_unset_timeout_env_var_uses_default(self):
+        with patch.dict(os.environ):
+            os.environ.pop(OTEL_EXPORTER_ZIPKIN_TIMEOUT, None)
+            with self.assertNoLogs("opentelemetry.exporter.zipkin.json", level="WARNING"):
+                exporter = ZipkinExporter()
+            self.assertEqual(exporter.timeout, 10)
+
+    def test_constructor_numeric_timeout_env_var_preserves_values(self):
+        for value, expected in (("15", 15), (" 15 ", 15), ("0", 0), ("-1", -1)):
+            with self.subTest(value=value), patch.dict(os.environ, {OTEL_EXPORTER_ZIPKIN_TIMEOUT: value}):
+                with self.assertNoLogs("opentelemetry.exporter.zipkin.json", level="WARNING"):
+                    exporter = ZipkinExporter()
+                self.assertEqual(exporter.timeout, expected)
+
+    def test_constructor_explicit_timeout_ignores_invalid_env_var(self):
+        for value in ("10s", "", "\t\n"):
+            with self.subTest(value=value), patch.dict(os.environ, {OTEL_EXPORTER_ZIPKIN_TIMEOUT: value}):
+                with self.assertNoLogs("opentelemetry.exporter.zipkin.json", level="WARNING"):
+                    exporter = ZipkinExporter(timeout=7)
+                self.assertEqual(exporter.timeout, 7)
+
+    def test_constructor_explicit_timeout_preserves_truthiness(self):
+        with patch.dict(os.environ, {OTEL_EXPORTER_ZIPKIN_TIMEOUT: "15"}):
+            with self.assertNoLogs("opentelemetry.exporter.zipkin.json", level="WARNING"):
+                self.assertEqual(ZipkinExporter(timeout=0).timeout, 15)
+                self.assertEqual(ZipkinExporter(timeout=-1).timeout, -1)
+
     def test_constructor_protocol_endpoint(self):
         """Test the constructor for the common usage of providing the
         protocol and endpoint arguments."""

exporter/opentelemetry-exporter-zipkin-proto-http/src/opentelemetry/exporter/zipkin/proto/http/__init__.py

@@ -78,10 +78,27 @@
 
 DEFAULT_ENDPOINT = "http://localhost:9411/api/v2/spans"
 REQUESTS_SUCCESS_STATUS_CODES = (200, 202)
+_DEFAULT_TIMEOUT = 10
 
 logger = logging.getLogger(__name__)
 
 
+def _timeout_from_env() -> int:
+    value = environ.get(OTEL_EXPORTER_ZIPKIN_TIMEOUT)
+    if value is None or not value.strip():
+        return _DEFAULT_TIMEOUT
+    try:
+        return int(value)
+    except ValueError:
+        logger.warning(
+            "Invalid value %r for %s, using default of %s seconds",
+            value,
+            OTEL_EXPORTER_ZIPKIN_TIMEOUT,
+            _DEFAULT_TIMEOUT,
+        )
+        return _DEFAULT_TIMEOUT
+
+
 class ZipkinExporter(SpanExporter):
     def __init__(
         self,
@@ -121,7 +138,7 @@ def __init__(
         self.session = session or requests.Session()
         self.session.headers.update({"Content-Type": self.encoder.content_type()})
         self._closed = False
-        self.timeout = timeout or int(environ.get(OTEL_EXPORTER_ZIPKIN_TIMEOUT, 10))
+        self.timeout = timeout or _timeout_from_env()
 
     def export(self, spans: Sequence[Span]) -> SpanExportResult:
         # After the call to Shutdown subsequent calls to Export are

exporter/opentelemetry-exporter-zipkin-proto-http/tests/test_zipkin_exporter.py

@@ -65,6 +65,52 @@ def test_constructor_env_vars(self):
         self.assertEqual(exporter.local_node.port, None)
         self.assertEqual(exporter.timeout, 15)
 
+    def test_constructor_invalid_timeout_env_var_falls_back(self):
+        with patch.dict(os.environ, {OTEL_EXPORTER_ZIPKIN_TIMEOUT: "10s"}):
+            with self.assertLogs("opentelemetry.exporter.zipkin.proto.http", level="WARNING") as cm:
+                exporter = ZipkinExporter()
+
+            self.assertEqual(exporter.timeout, 10)
+            self.assertEqual(len(cm.records), 1)
+            self.assertTrue(
+                any("Invalid value" in message for message in cm.output),
+                cm.output,
+            )
+
+    def test_constructor_empty_timeout_env_var_falls_back(self):
+        for value in ("", " ", "\t\n"):
+            with self.subTest(value=value), patch.dict(os.environ, {OTEL_EXPORTER_ZIPKIN_TIMEOUT: value}):
+                with self.assertNoLogs("opentelemetry.exporter.zipkin.proto.http", level="WARNING"):
+                    exporter = ZipkinExporter()
+                self.assertEqual(exporter.timeout, 10)
+
+    def test_constructor_unset_timeout_env_var_uses_default(self):
+        with patch.dict(os.environ):
+            os.environ.pop(OTEL_EXPORTER_ZIPKIN_TIMEOUT, None)
+            with self.assertNoLogs("opentelemetry.exporter.zipkin.proto.http", level="WARNING"):
+                exporter = ZipkinExporter()
+            self.assertEqual(exporter.timeout, 10)
+
+    def test_constructor_numeric_timeout_env_var_preserves_values(self):
+        for value, expected in (("15", 15), (" 15 ", 15), ("0", 0), ("-1", -1)):
+            with self.subTest(value=value), patch.dict(os.environ, {OTEL_EXPORTER_ZIPKIN_TIMEOUT: value}):
+                with self.assertNoLogs("opentelemetry.exporter.zipkin.proto.http", level="WARNING"):
+                    exporter = ZipkinExporter()
+                self.assertEqual(exporter.timeout, expected)
+
+    def test_constructor_explicit_timeout_ignores_invalid_env_var(self):
+        for value in ("10s", "", "\t\n"):
+            with self.subTest(value=value), patch.dict(os.environ, {OTEL_EXPORTER_ZIPKIN_TIMEOUT: value}):
+                with self.assertNoLogs("opentelemetry.exporter.zipkin.proto.http", level="WARNING"):
+                    exporter = ZipkinExporter(timeout=7)
+                self.assertEqual(exporter.timeout, 7)
+
+    def test_constructor_explicit_timeout_preserves_truthiness(self):
+        with patch.dict(os.environ, {OTEL_EXPORTER_ZIPKIN_TIMEOUT: "15"}):
+            with self.assertNoLogs("opentelemetry.exporter.zipkin.proto.http", level="WARNING"):
+                self.assertEqual(ZipkinExporter(timeout=0).timeout, 15)
+                self.assertEqual(ZipkinExporter(timeout=-1).timeout, -1)
+
     def test_constructor_protocol_endpoint(self):
         """Test the constructor for the common usage of providing the
         protocol and endpoint arguments."""