From d31d1cca85f7f0b50ed19501659603291ef393db Mon Sep 17 00:00:00 2001 From: Joe McGinley <116890464+jomcgi@users.noreply.github.com> Date: Sat, 1 Mar 2025 15:21:24 +0000 Subject: [PATCH] fix: error raised for env checks in LogLimits and SpanLimits Current implementation raises a value error trying to create the error message. Updated error message format to use positional instead of named params. Added test cases to validate the correct errors are raised. --- CHANGELOG.md | 4 +++ .../exporter/otlp/proto/grpc/exporter.py | 12 +++---- .../tests/test_otlp_metrics_exporter.py | 15 +++++++++ .../tests/test_otlp_trace_exporter.py | 15 +++++++++ .../sdk/_logs/_internal/__init__.py | 2 +- .../src/opentelemetry/sdk/trace/__init__.py | 2 +- .../tests/logs/test_log_limits.py | 32 +++++++++++++++++++ opentelemetry-sdk/tests/trace/test_trace.py | 31 ++++++++++++++++++ 8 files changed, 105 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a2b74a3203..65b3c51acbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#4402](https://github.com/open-telemetry/opentelemetry-python/pull/4402)) - Tolerates exceptions when loading resource detectors via `OTEL_EXPERIMENTAL_RESOURCE_DETECTORS` ([#4373](https://github.com/open-telemetry/opentelemetry-python/pull/4373)) +- Disconnect gRPC client stub when shutting down `OTLPSpanExporter` + ([#4370](https://github.com/open-telemetry/opentelemetry-python/pull/4370)) - opentelemetry-sdk: fix OTLP exporting of Histograms with explicit buckets advisory ([#4434](https://github.com/open-telemetry/opentelemetry-python/pull/4434)) - opentelemetry-exporter-otlp-proto-grpc: better dependency version range for Python 3.13 @@ -24,6 +26,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#4448](https://github.com/open-telemetry/opentelemetry-python/pull/4448)) - Make `trace_api.use_span()` record `BaseException` as well as `Exception` ([#4406](https://github.com/open-telemetry/opentelemetry-python/pull/4406)) +- Fix env var error message for TraceLimits/SpanLimits + ([#4458](https://github.com/open-telemetry/opentelemetry-python/pull/4458)) ## Version 1.30.0/0.51b0 (2025-02-03) 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 582d083e86f..4be75c5335e 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 @@ -243,8 +243,8 @@ def __init__( ) or Compression.NoCompression if insecure: - self._client = self._stub( - insecure_channel(self._endpoint, compression=compression) + self._channel = insecure_channel( + self._endpoint, compression=compression ) else: credentials = _get_credentials( @@ -253,11 +253,10 @@ def __init__( OTEL_EXPORTER_OTLP_CLIENT_KEY, OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE, ) - self._client = self._stub( - secure_channel( - self._endpoint, credentials, compression=compression - ) + self._channel = secure_channel( + self._endpoint, credentials, compression=compression ) + self._client = self._stub(self._channel) self._export_lock = threading.Lock() self._shutdown = False @@ -360,6 +359,7 @@ def shutdown(self, timeout_millis: float = 30_000, **kwargs) -> None: # wait for the last export if any self._export_lock.acquire(timeout=timeout_millis / 1e3) self._shutdown = True + self._channel.close() self._export_lock.release() @property diff --git a/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_metrics_exporter.py b/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_metrics_exporter.py index 1d2bae2486d..9cd7ac38358 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_metrics_exporter.py +++ b/exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_metrics_exporter.py @@ -874,6 +874,21 @@ def test_shutdown_wait_last_export(self): finally: export_thread.join() + def test_export_over_closed_grpc_channel(self): + # pylint: disable=protected-access + + add_MetricsServiceServicer_to_server( + MetricsServiceServicerSUCCESS(), self.server + ) + self.exporter.export(self.metrics["sum_int"]) + self.exporter.shutdown() + data = self.exporter._translate_data(self.metrics["sum_int"]) + with self.assertRaises(ValueError) as err: + self.exporter._client.Export(request=data) + self.assertEqual( + str(err.exception), "Cannot invoke RPC on closed channel!" + ) + def test_aggregation_temporality(self): # pylint: disable=protected-access 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 fe0b94ac787..f29b7fc611c 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 @@ -1017,6 +1017,21 @@ def test_shutdown_wait_last_export(self): finally: export_thread.join() + def test_export_over_closed_grpc_channel(self): + # pylint: disable=protected-access + + add_TraceServiceServicer_to_server( + TraceServiceServicerSUCCESS(), self.server + ) + self.exporter.export([self.span]) + self.exporter.shutdown() + data = self.exporter._translate_data([self.span]) + with self.assertRaises(ValueError) as err: + self.exporter._client.Export(request=data) + self.assertEqual( + str(err.exception), "Cannot invoke RPC on closed channel!" + ) + def _create_span_with_status(status: SDKStatus): span = _Span( diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py index 1e5f57725e7..ab1940f9dec 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py @@ -134,7 +134,7 @@ def _from_env_if_absent( if value == cls.UNSET: return None - err_msg = "{0} must be a non-negative integer but got {}" + err_msg = "{} must be a non-negative integer but got {}" # if no value is provided for the limit, try to load it from env if value is None: diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 729ceb8226b..3ac45806358 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -692,7 +692,7 @@ def _from_env_if_absent( if value == cls.UNSET: return None - err_msg = "{0} must be a non-negative integer but got {}" + err_msg = "{} must be a non-negative integer but got {}" # if no value is provided for the limit, try to load it from env if value is None: diff --git a/opentelemetry-sdk/tests/logs/test_log_limits.py b/opentelemetry-sdk/tests/logs/test_log_limits.py index c2135b65694..82a7ce9b4d6 100644 --- a/opentelemetry-sdk/tests/logs/test_log_limits.py +++ b/opentelemetry-sdk/tests/logs/test_log_limits.py @@ -13,11 +13,16 @@ # limitations under the License. import unittest +from unittest.mock import patch from opentelemetry.sdk._logs import LogLimits from opentelemetry.sdk._logs._internal import ( _DEFAULT_OTEL_ATTRIBUTE_COUNT_LIMIT, ) +from opentelemetry.sdk.environment_variables import ( + OTEL_ATTRIBUTE_COUNT_LIMIT, + OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT, +) class TestLogLimits(unittest.TestCase): @@ -38,3 +43,30 @@ def test_log_limits_max_attribute_length(self): limits = LogLimits(max_attribute_length=1) self.assertEqual(expected, limits.max_attribute_length) + + def test_invalid_env_vars_raise(self): + env_vars = [ + OTEL_ATTRIBUTE_COUNT_LIMIT, + OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT, + ] + + bad_values = ["bad", "-1"] + test_cases = { + env_var: bad_value + for env_var in env_vars + for bad_value in bad_values + } + + for env_var, bad_value in test_cases.items(): + with self.subTest(f"Testing {env_var}={bad_value}"): + with self.assertRaises(ValueError) as error, patch.dict( + "os.environ", {env_var: bad_value}, clear=True + ): + LogLimits() + + expected_msg = f"{env_var} must be a non-negative integer but got {bad_value}" + self.assertEqual( + expected_msg, + str(error.exception), + f"Unexpected error message for {env_var}={bad_value}", + ) diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 790d7fd2a07..7b23c11fa1f 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -2007,6 +2007,37 @@ def _test_span_no_limits(self, tracer): for attr_val in root.attributes.values(): self.assertEqual(attr_val, self.long_val) + def test_invalid_env_vars_raise(self): + env_vars = [ + OTEL_SPAN_EVENT_COUNT_LIMIT, + OTEL_SPAN_LINK_COUNT_LIMIT, + OTEL_ATTRIBUTE_COUNT_LIMIT, + OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT, + OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT, + OTEL_LINK_ATTRIBUTE_COUNT_LIMIT, + OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT, + ] + bad_values = ["bad", "-1"] + test_cases = { + env_var: bad_value + for env_var in env_vars + for bad_value in bad_values + } + + for env_var, bad_value in test_cases.items(): + with self.subTest(f"Testing {env_var}={bad_value}"): + with self.assertRaises(ValueError) as error, patch.dict( + "os.environ", {env_var: bad_value}, clear=True + ): + trace.SpanLimits() + + expected_msg = f"{env_var} must be a non-negative integer but got {bad_value}" + self.assertEqual( + expected_msg, + str(error.exception), + f"Unexpected error message for {env_var}={bad_value}", + ) + class TestTraceFlags(unittest.TestCase): def test_constant_default(self):