diff --git a/.changelog/5565.fixed b/.changelog/5565.fixed new file mode 100644 index 0000000000..a0e4b6c58c --- /dev/null +++ b/.changelog/5565.fixed @@ -0,0 +1 @@ +`opentelemetry-sdk`: keep metric attribute values that Python considers equal but the data model does not, such as `True`, `1` and `1.0`, in separate metric streams diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py index 43240429c6..c6c4dbdd34 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py @@ -25,7 +25,10 @@ _logger = getLogger(__name__) -_HashedAttributes = str | bool | int | float | bytes | None | tuple["_HashedAttributes", ...] +# Every branch is tagged so that values Python considers equal but the OTel +# data model does not -- True/1/1.0, or a sequence of pairs and the mapping +# it resembles -- produce different aggregation keys. +_HashedAttributes = tuple[str, "str | bool | int | float | bytes | None | tuple[_HashedAttributes, ...]"] # pylint: disable=inconsistent-return-statements @@ -33,16 +36,21 @@ def _hash_attributes(value: Attributes | AnyValue) -> _HashedAttributes: # Attributes have been cleaned and validated when Measurement was instantiated, # so value is guaranteed to match one of the branches below at runtime. if isinstance(value, (NoneType, str, int, float, bool, bytes)): - return value + # bool is a subclass of int and 1 == 1.0, so the value alone is not + # enough to tell these apart. + return (type(value).__name__, value) if isinstance(value, Sequence): - return tuple(_hash_attributes(v) for v in value) + return ("sequence", tuple(_hash_attributes(v) for v in value)) if isinstance(value, Mapping): - return tuple( - (k, _hash_attributes(value[k])) - for k in sorted( - value, - key=lambda item: item if isinstance(item, str) else str(item), - ) + return ( + "mapping", + tuple( + (k, _hash_attributes(value[k])) + for k in sorted( + value, + key=lambda item: item if isinstance(item, str) else str(item), + ) + ), ) if TYPE_CHECKING: assert_never(value) diff --git a/opentelemetry-sdk/tests/metrics/test_attribute_hashing.py b/opentelemetry-sdk/tests/metrics/test_attribute_hashing.py new file mode 100644 index 0000000000..f75ea69756 --- /dev/null +++ b/opentelemetry-sdk/tests/metrics/test_attribute_hashing.py @@ -0,0 +1,106 @@ +# Copyright The OpenTelemetry Authors +# SPDX-License-Identifier: Apache-2.0 + +"""Attribute sets that differ in the OTel data model must not share a stream. + +`_hash_attributes` builds the aggregation key. Python compares `True == 1 == +1.0` and hashes them identically, but OTLP encodes them as `bool_value`, +`int_value` and `double_value` -- three different values. Folding them +together merges unrelated time series and mislabels the survivor. +""" + +import unittest + +from opentelemetry.sdk.metrics import MeterProvider +from opentelemetry.sdk.metrics._internal._view_instrument_match import ( + _hash_attributes, +) +from opentelemetry.sdk.metrics.export import InMemoryMetricReader + + +class TestHashAttributesDistinguishesTypes(unittest.TestCase): + def test_bool_int_and_float_are_distinct(self): + keys = [ + _hash_attributes({"k": True}), + _hash_attributes({"k": 1}), + _hash_attributes({"k": 1.0}), + ] + self.assertEqual(len(set(keys)), 3) + + def test_false_and_zero_are_distinct(self): + self.assertNotEqual(_hash_attributes({"k": False}), _hash_attributes({"k": 0})) + + def test_none_and_the_string_none_are_distinct(self): + self.assertNotEqual(_hash_attributes({"k": None}), _hash_attributes({"k": "None"})) + + def test_sequence_of_pairs_and_mapping_are_distinct(self): + """A list of pairs must not hash like the equivalent mapping.""" + self.assertNotEqual( + _hash_attributes({"k": (("x", 1),)}), + _hash_attributes({"k": {"x": 1}}), + ) + + def test_nested_types_are_distinguished(self): + self.assertNotEqual(_hash_attributes({"k": (True,)}), _hash_attributes({"k": (1,)})) + + def test_key_order_still_does_not_matter(self): + """Ordering must remain irrelevant; this is the property that matters most.""" + self.assertEqual( + _hash_attributes({"a": 1, "b": 2}), + _hash_attributes({"b": 2, "a": 1}), + ) + + def test_equal_attributes_still_share_a_key(self): + self.assertEqual(_hash_attributes({"a": "x"}), _hash_attributes({"a": "x"})) + + def test_result_is_hashable(self): + hash(_hash_attributes({"a": 1, "b": (1, 2), "c": {"d": None}})) + + +class TestDistinctAttributeTypesProduceDistinctStreams(unittest.TestCase): + def setUp(self): + self.reader = InMemoryMetricReader() + self.provider = MeterProvider(metric_readers=[self.reader]) + self.meter = self.provider.get_meter(__name__) + + def tearDown(self): + self.provider.shutdown() + + def _data_points(self): + data = self.reader.get_metrics_data() + return data.resource_metrics[0].scope_metrics[0].metrics[0].data.data_points + + def test_counter_keeps_bool_int_and_float_apart(self): + counter = self.meter.create_counter("counter") + counter.add(1, {"k": True}) + counter.add(1, {"k": 1}) + counter.add(1, {"k": 1.0}) + + points = self._data_points() + self.assertEqual(len(points), 3) + self.assertEqual([p.value for p in points], [1, 1, 1]) + self.assertEqual( + sorted(type(dict(p.attributes)["k"]).__name__ for p in points), + ["bool", "float", "int"], + ) + + def test_histogram_keeps_false_and_zero_apart(self): + histogram = self.meter.create_histogram("histogram") + histogram.record(1, {"flag": False}) + histogram.record(2, {"flag": 0}) + + points = self._data_points() + self.assertEqual(len(points), 2) + self.assertEqual( + sorted(type(dict(p.attributes)["flag"]).__name__ for p in points), + ["bool", "int"], + ) + + def test_same_attributes_still_aggregate_together(self): + counter = self.meter.create_counter("counter") + counter.add(1, {"k": "same"}) + counter.add(1, {"k": "same"}) + + points = self._data_points() + self.assertEqual(len(points), 1) + self.assertEqual(points[0].value, 2)