diff --git a/.changelog/5336.fixed b/.changelog/5336.fixed new file mode 100644 index 0000000000..48d0fc724e --- /dev/null +++ b/.changelog/5336.fixed @@ -0,0 +1 @@ +`opentelemetry-sdk`: drop non-finite measurements (NaN and Inf) at the instrument level to prevent permanent aggregation poisoning diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py index 946cdf1df1..548aa8ee1f 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py @@ -4,6 +4,7 @@ # pylint: disable=too-many-ancestors, unused-import from __future__ import annotations +import math from collections.abc import Generator, Iterable, Sequence from logging import getLogger from time import time_ns @@ -163,6 +164,13 @@ def callback( for callback in self._callbacks: try: for api_measurement in callback(callback_options): + if not math.isfinite(api_measurement.value): + _logger.warning( + "Callback returned a non-finite value %s for instrument %s, ignoring measurement.", + api_measurement.value, + self.name, + ) + continue yield Measurement( api_measurement.value, time_unix_nano=time_ns(), @@ -192,6 +200,13 @@ def add( super().add(amount, attributes=attributes, context=context) return + if not math.isfinite(amount): + _logger.warning( + "Add amount %s is not finite on Counter %s, ignoring measurement.", + amount, + self.name, + ) + return if amount < 0: _logger.warning( "Add amount must be non-negative on Counter %s.", self.name @@ -225,6 +240,13 @@ def add( super().add(amount, attributes=attributes, context=context) return + if not math.isfinite(amount): + _logger.warning( + "Add amount %s is not finite on UpDownCounter %s, ignoring measurement.", + amount, + self.name, + ) + return time_unix_nano = time_ns() self._measurement_consumer.consume_measurement( Measurement( @@ -294,6 +316,13 @@ def record( super().record(amount, attributes=attributes, context=context) return + if not math.isfinite(amount): + _logger.warning( + "Record amount %s is not finite on Histogram %s, ignoring measurement.", + amount, + self.name, + ) + return if amount < 0: _logger.warning( "Record amount must be non-negative on Histogram %s.", @@ -328,6 +357,13 @@ def set( super().set(amount, attributes=attributes, context=context) return + if not math.isfinite(amount): + _logger.warning( + "Set amount %s is not finite on Gauge %s, ignoring measurement.", + amount, + self.name, + ) + return time_unix_nano = time_ns() self._measurement_consumer.consume_measurement( Measurement( diff --git a/opentelemetry-sdk/tests/metrics/test_instrument.py b/opentelemetry-sdk/tests/metrics/test_instrument.py index c5375e3783..d8fd813e0a 100644 --- a/opentelemetry-sdk/tests/metrics/test_instrument.py +++ b/opentelemetry-sdk/tests/metrics/test_instrument.py @@ -51,6 +51,20 @@ def test_add_non_monotonic(self): counter.add(-1.0) mc.consume_measurement.assert_not_called() + def test_add_nan(self): + mc = Mock() + counter = _Counter("name", Mock(), mc) + with self.assertLogs(level=WARNING): + counter.add(float("nan")) + mc.consume_measurement.assert_not_called() + + def test_add_inf(self): + mc = Mock() + counter = _Counter("name", Mock(), mc) + with self.assertLogs(level=WARNING): + counter.add(float("inf")) + mc.consume_measurement.assert_not_called() + def test_disallow_direct_counter_creation(self): with self.assertRaises(TypeError): # pylint: disable=abstract-class-instantiated @@ -70,6 +84,20 @@ def test_add_non_monotonic(self): counter.add(-1.0) mc.consume_measurement.assert_called_once() + def test_add_nan(self): + mc = Mock() + counter = _UpDownCounter("name", Mock(), mc) + with self.assertLogs(level=WARNING): + counter.add(float("nan")) + mc.consume_measurement.assert_not_called() + + def test_add_inf(self): + mc = Mock() + counter = _UpDownCounter("name", Mock(), mc) + with self.assertLogs(level=WARNING): + counter.add(float("inf")) + mc.consume_measurement.assert_not_called() + def test_disallow_direct_up_down_counter_creation(self): with self.assertRaises(TypeError): # pylint: disable=abstract-class-instantiated @@ -300,6 +328,36 @@ def test_generator_multiple_generator_callback(self): ], ) + def test_nan_callback_value_is_dropped(self): + def nan_callback(options: CallbackOptions): + return [ + Observation(float("nan"), attributes=TEST_ATTRIBUTES), + Observation(1, attributes=TEST_ATTRIBUTES), + ] + + observable_gauge = _ObservableGauge( + "name", Mock(), Mock(), [nan_callback] + ) + with self.assertLogs(level=WARNING): + measurements = list(observable_gauge.callback(CallbackOptions())) + self.assertEqual(len(measurements), 1) + self.assertEqual(measurements[0].value, 1) + + def test_inf_callback_value_is_dropped(self): + def inf_callback(options: CallbackOptions): + return [ + Observation(float("inf"), attributes=TEST_ATTRIBUTES), + Observation(1, attributes=TEST_ATTRIBUTES), + ] + + observable_gauge = _ObservableGauge( + "name", Mock(), Mock(), [inf_callback] + ) + with self.assertLogs(level=WARNING): + measurements = list(observable_gauge.callback(CallbackOptions())) + self.assertEqual(len(measurements), 1) + self.assertEqual(measurements[0].value, 1) + def test_disallow_direct_observable_gauge_creation(self): with self.assertRaises(TypeError): # pylint: disable=abstract-class-instantiated @@ -392,6 +450,20 @@ def test_set(self): gauge.set(1.0) mc.consume_measurement.assert_called_once() + def test_set_nan(self): + mc = Mock() + gauge = _Gauge("name", Mock(), mc) + with self.assertLogs(level=WARNING): + gauge.set(float("nan")) + mc.consume_measurement.assert_not_called() + + def test_set_inf(self): + mc = Mock() + gauge = _Gauge("name", Mock(), mc) + with self.assertLogs(level=WARNING): + gauge.set(float("inf")) + mc.consume_measurement.assert_not_called() + def test_disallow_direct_counter_creation(self): with self.assertRaises(TypeError): # pylint: disable=abstract-class-instantiated @@ -487,6 +559,20 @@ def test_record_non_monotonic(self): hist.record(-1.0) mc.consume_measurement.assert_not_called() + def test_record_nan(self): + mc = Mock() + hist = _Histogram("name", Mock(), mc) + with self.assertLogs(level=WARNING): + hist.record(float("nan")) + mc.consume_measurement.assert_not_called() + + def test_record_inf(self): + mc = Mock() + hist = _Histogram("name", Mock(), mc) + with self.assertLogs(level=WARNING): + hist.record(float("inf")) + mc.consume_measurement.assert_not_called() + def test_disallow_direct_histogram_creation(self): with self.assertRaises(TypeError): # pylint: disable=abstract-class-instantiated