diff --git a/.changelog/5332.added b/.changelog/5332.added new file mode 100644 index 00000000000..d8e9a2dd664 --- /dev/null +++ b/.changelog/5332.added @@ -0,0 +1 @@ +`opentelemetry-sdk`: add support for parameter `exclude_attribute_keys` in `View` diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/_meter_provider.py b/opentelemetry-configuration/src/opentelemetry/configuration/_meter_provider.py index 39e78e9f25c..9f5615a7c87 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/_meter_provider.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/_meter_provider.py @@ -235,14 +235,13 @@ def _create_view(config: ViewConfig) -> View: if instrument_type is None: raise ConfigurationError(f"Unknown instrument type: {selector.instrument_type!r}") - attribute_keys: set[str] | None = None + attribute_keys: list[str] | None = None + exclude_attribute_keys: list[str] | None = None if stream.attribute_keys is not None: - if stream.attribute_keys.excluded: - _logger.warning( - "attribute_keys.excluded is not supported by the Python SDK View; the exclusion list will be ignored." - ) + if stream.attribute_keys.excluded is not None: + exclude_attribute_keys = stream.attribute_keys.excluded if stream.attribute_keys.included is not None: - attribute_keys = set(stream.attribute_keys.included) + attribute_keys = stream.attribute_keys.included aggregation = None if stream.aggregation is not None: @@ -259,6 +258,7 @@ def _create_view(config: ViewConfig) -> View: description=stream.description, attribute_keys=attribute_keys, aggregation=aggregation, + exclude_attribute_keys=exclude_attribute_keys, aggregation_cardinality_limit=stream.aggregation_cardinality_limit, ) diff --git a/opentelemetry-configuration/tests/test_meter_provider.py b/opentelemetry-configuration/tests/test_meter_provider.py index 25e5aa3f7be..e1baa57a7e8 100644 --- a/opentelemetry-configuration/tests/test_meter_provider.py +++ b/opentelemetry-configuration/tests/test_meter_provider.py @@ -964,11 +964,75 @@ def test_stream_attribute_keys_included(self): ) self.assertEqual(view._attribute_keys, {"key1", "key2"}) - def test_stream_attribute_keys_excluded_logs_warning(self): + def test_stream_attribute_keys_excluded_is_applied(self): config = self._make_view_config(stream_kwargs={"attribute_keys": IncludeExclude(excluded=["key1"])}) - with self.assertLogs("opentelemetry.configuration._meter_provider", level="WARNING") as log: - create_meter_provider(config) - self.assertTrue(any("excluded" in msg for msg in log.output)) + meter_provider = create_meter_provider(config) + views = meter_provider._sdk_config.views + self.assertEqual(len(views), 1) + view = views[0] + self.assertEqual(view._exclude_attribute_keys, frozenset({"key1"})) + self.assertIsNone(view._attribute_keys) + + def test_stream_attribute_keys_exact_overlap_is_allowed(self): + view = self._get_view( + self._make_view_config( + stream_kwargs={ + "attribute_keys": IncludeExclude( + included=["region"], + excluded=["region"], + ) + } + ) + ) + + self.assertEqual( + view._attribute_keys, + frozenset({"region"}), + ) + self.assertEqual( + view._exclude_attribute_keys, + frozenset({"region"}), + ) + + def test_stream_attribute_key_pattern_intersection_is_allowed( + self, + ): + view = self._get_view( + self._make_view_config( + stream_kwargs={ + "attribute_keys": IncludeExclude( + included=["http.*"], + excluded=["*.status"], + ) + } + ) + ) + + self.assertEqual( + view._attribute_keys, + frozenset({"http.*"}), + ) + self.assertEqual( + view._exclude_attribute_keys, + frozenset({"*.status"}), + ) + + def test_stream_empty_exclusion_list_is_allowed(self): + view = self._get_view( + self._make_view_config( + stream_kwargs={ + "attribute_keys": IncludeExclude( + excluded=[], + ) + } + ) + ) + + self.assertIsNone(view._attribute_keys) + self.assertEqual( + view._exclude_attribute_keys, + frozenset(), + ) def test_stream_aggregation_drop(self): view = self._get_view(self._make_view_config(stream_kwargs={"aggregation": AggregationConfig(drop={})})) 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 e9ac5eecce5..7ec6f71ca4e 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 @@ -20,7 +20,10 @@ from opentelemetry.sdk.metrics._internal.instrument import _Instrument from opentelemetry.sdk.metrics._internal.measurement import Measurement from opentelemetry.sdk.metrics._internal.point import DataPointT -from opentelemetry.sdk.metrics._internal.view import View +from opentelemetry.sdk.metrics._internal.view import ( + View, + _attribute_key_matches, +) from opentelemetry.util.types import AnyValue, Attributes _logger = getLogger(__name__) @@ -141,8 +144,20 @@ def consume_measurement(self, measurement: Measurement, should_sample_exemplar: # The user can still modify mutable attribute values (lists/dicts) # leading to unexpected behavior, but deep copying is expensive. attributes = dict(measurement.attributes) + if self._view._attribute_keys is not None: - attributes = {k: v for k, v in attributes.items() if k in self._view._attribute_keys} + attributes = { + key: value + for key, value in attributes.items() + if any(_attribute_key_matches(key, pattern) for pattern in self._view._attribute_keys) + } + + if self._view._exclude_attribute_keys is not None: + attributes = { + key: value + for key, value in attributes.items() + if not any(_attribute_key_matches(key, pattern) for pattern in self._view._exclude_attribute_keys) + } aggr_key = _hash_attributes(attributes) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/view.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/view.py index 23f415be1e9..d95fc380895 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/view.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/view.py @@ -1,8 +1,8 @@ # Copyright The OpenTelemetry Authors # SPDX-License-Identifier: Apache-2.0 - -from collections.abc import Callable +import re +from collections.abc import Callable, Iterable from fnmatch import fnmatchcase from logging import getLogger @@ -24,6 +24,26 @@ _logger = getLogger(__name__) +def _normalize_attribute_keys(keys: Iterable[str] | None) -> frozenset[str] | None: + """Normalize attribute-key iterables to immutable sets for internal use.""" + if keys is None: + return None + if isinstance(keys, str): + return frozenset({keys}) + return frozenset(keys) + + +def _attribute_key_matches(key: str, pattern: str) -> bool: + """Match attribute keys using only ``*`` and ``?`` as wildcards.""" + if pattern == key: + return True + + expression = "".join( + ".*" if char == "*" else "." if char == "?" else re.escape(char) for char in pattern + ) + return re.fullmatch(expression, key) is not None + + def _default_reservoir_factory( aggregation_type: type[_Aggregation], ) -> ExemplarReservoirBuilder: @@ -75,9 +95,13 @@ class View: description of the metric stream. If `None`, the description of the instrument will be used. - attribute_keys: This is a metric stream customizing attribute: this is - a set of attribute keys. If not `None` then only the measurement attributes that - are in ``attribute_keys`` will be used to identify the metric stream. + attribute_keys: This is a metric stream customizing attribute: a set-like + collection of attribute keys to keep. If not `None`, only the measurement + attributes whose keys are in ``attribute_keys`` are used to identify the + metric stream. + + exclude_attribute_keys: This is a metric stream customizing attribute: a set-like + collection of attribute keys to drop after any include list is applied. aggregation: This is a metric stream customizing attribute: the aggregation instance to use when data is aggregated for the @@ -112,10 +136,11 @@ def __init__( meter_schema_url: str | None = None, name: str | None = None, description: str | None = None, - attribute_keys: set[str] | None = None, + attribute_keys: Iterable[str] | None = None, aggregation: Aggregation | None = None, exemplar_reservoir_factory: Callable[[type[_Aggregation]], ExemplarReservoirBuilder] | None = None, instrument_unit: str | None = None, + exclude_attribute_keys: Iterable[str] | None = None, aggregation_cardinality_limit: int | None = None, ): if aggregation_cardinality_limit is not None and ( @@ -150,8 +175,8 @@ def __init__( raise Exception(f"View {name} declared with wildcard characters in instrument_name") # _name, _description, _aggregation, _exemplar_reservoir_factory, - # _attribute_keys and _aggregation_cardinality_limit will be accessed - # when instantiating a _ViewInstrumentMatch. + # _attribute_keys, _exclude_attribute_keys and _aggregation_cardinality_limit + # will be accessed when instantiating a _ViewInstrumentMatch. self._name = name self._instrument_type = instrument_type self._instrument_name = instrument_name @@ -161,7 +186,8 @@ def __init__( self._meter_schema_url = meter_schema_url self._description = description - self._attribute_keys = attribute_keys + self._attribute_keys = _normalize_attribute_keys(attribute_keys) + self._exclude_attribute_keys = _normalize_attribute_keys(exclude_attribute_keys) self._aggregation = aggregation or self._default_aggregation self._exemplar_reservoir_factory = exemplar_reservoir_factory or _default_reservoir_factory self._aggregation_cardinality_limit = aggregation_cardinality_limit diff --git a/opentelemetry-sdk/tests/metrics/test_view.py b/opentelemetry-sdk/tests/metrics/test_view.py index f76c01083ca..2ab632a5abf 100644 --- a/opentelemetry-sdk/tests/metrics/test_view.py +++ b/opentelemetry-sdk/tests/metrics/test_view.py @@ -7,6 +7,9 @@ from unittest.mock import Mock from opentelemetry.sdk.metrics import Counter, MeterProvider +from opentelemetry.sdk.metrics._internal.view import ( + _attribute_key_matches, +) from opentelemetry.sdk.metrics.view import View @@ -164,6 +167,113 @@ def test_view_name(self): self.assertEqual(view._name, "name") self.assertEqual(view._instrument_name, "instrument_name_1") + def test_attribute_keys_and_exclude_attribute_keys_overlap(self): + test_cases = [ + { + "name": "exact overlap", + "attribute_keys": ("method", "status_code"), + "exclude_attribute_keys": ("method", "user_id"), + }, + { + "name": "intersecting wildcard patterns", + "attribute_keys": ("http.*",), + "exclude_attribute_keys": ("*.status",), + }, + { + "name": "intersecting question mark patterns", + "attribute_keys": ("a?",), + "exclude_attribute_keys": ("?b",), + }, + { + "name": "literal brackets do not overlap", + "attribute_keys": ("header0",), + "exclude_attribute_keys": ("header[0]",), + }, + ] + + for case in test_cases: + with self.subTest(case=case["name"]): + view = View( + instrument_name="instrument_name", + attribute_keys=case["attribute_keys"], + exclude_attribute_keys=case[ + "exclude_attribute_keys" + ], + ) + + self.assertEqual( + view._attribute_keys, + frozenset(case["attribute_keys"]), + ) + self.assertEqual( + view._exclude_attribute_keys, + frozenset(case["exclude_attribute_keys"]), + ) + + def test_attribute_key_matching_semantics(self): + test_cases = [ + ("exact match", "region", "region", True), + ("star wildcard", "http.status", "http.*", True), + ("question mark wildcard", "ab", "a?", True), + ( + "question mark one character only", + "abc", + "a?", + False, + ), + ( + "literal bracket match", + "header[0]", + "header[0]", + True, + ), + ( + "brackets are not character class", + "header0", + "header[0]", + False, + ), + ( + "literal backslash match", + r"header\0", + r"header\0", + True, + ), + ( + "backslash is not discarded", + "header0", + r"header\0", + False, + ), + ( + "regex dot remains literal", + "metric.value", + "metric.value", + True, + ), + ( + "literal dot mismatch", + "metricXvalue", + "metric.value", + False, + ), + ] + + for name, key, pattern, expected in test_cases: + with self.subTest(case=name): + self.assertIs( + _attribute_key_matches(key, pattern), + expected, + ) + + def test_attribute_keys_and_exclude_attribute_keys_disjoint(self): + view = View( + instrument_name="instrument_name", + attribute_keys=("method", "status_code"), + exclude_attribute_keys=("user_id",), + ) + + self.assertIsNotNone(view) def test_view_name_without_instrument_name(self): cases = [ ("custom_name", {"instrument_type": Mock}), diff --git a/opentelemetry-sdk/tests/metrics/test_view_instrument_match.py b/opentelemetry-sdk/tests/metrics/test_view_instrument_match.py index 8985a339d34..aa68d2bd9a9 100644 --- a/opentelemetry-sdk/tests/metrics/test_view_instrument_match.py +++ b/opentelemetry-sdk/tests/metrics/test_view_instrument_match.py @@ -73,6 +73,332 @@ def setUpClass(cls): views=[], ) + def test_consume_measurement_with_exclude_attribute_keys(self): + test_cases = [ + { + "name": "excluded attribute removed", + "exclude_attribute_keys": {"f"}, + "measurement_attributes": {"c": "d", "f": "g"}, + "expected_attributes": {"c": "d"}, + }, + { + "name": "wildcard exclusion", + "exclude_attribute_keys": {"k8s.*"}, + "measurement_attributes": { + "k8s.node": "node1", + "k8s.cluster": "cluster1", + "service.name": "svc", + }, + "expected_attributes": { + "service.name": "svc", + }, + }, + { + "name": "question mark exclusion", + "exclude_attribute_keys": {"node?"}, + "measurement_attributes": { + "node1": "a", + "node2": "b", + "node12": "c", + }, + "expected_attributes": { + "node12": "c", + }, + }, + { + "name": "wildcard exact exclusion", + "exclude_attribute_keys": {"*.node"}, + "measurement_attributes": { + "k8s.node": "node1", + "k8s.cluster": "cluster1", + }, + "expected_attributes": { + "k8s.cluster": "cluster1", + }, + }, + { + "name": "empty exclusion preserves attributes", + "exclude_attribute_keys": set(), + "measurement_attributes": { + "region": "us", + "service.name": "svc", + }, + "expected_attributes": { + "region": "us", + "service.name": "svc", + }, + }, + { + "name": "bracketed exclusion is literal", + "exclude_attribute_keys": {"header[0]"}, + "measurement_attributes": { + "header[0]": "remove", + "header0": "keep", + }, + "expected_attributes": { + "header0": "keep", + }, + }, + { + "name": "backslash exclusion is literal", + "exclude_attribute_keys": {r"header\0"}, + "measurement_attributes": { + r"header\0": "remove", + "header0": "keep", + }, + "expected_attributes": { + "header0": "keep", + }, + }, + { + "name": "none attributes become empty attributes", + "exclude_attribute_keys": {"f"}, + "measurement_attributes": None, + "expected_attributes": {}, + }, + ] + + for case in test_cases: + with self.subTest(case=case["name"]): + instrument1 = Mock(name="instrument1") + instrument1.instrumentation_scope = self.mock_instrumentation_scope + + view_instrument_match = _ViewInstrumentMatch( + view=View( + instrument_name="instrument1", + name="name", + aggregation=self.mock_aggregation_factory, + exclude_attribute_keys=case["exclude_attribute_keys"], + ), + instrument=instrument1, + instrument_class_aggregation=MagicMock(**{"__getitem__.return_value": DefaultAggregation()}), + ) + + view_instrument_match.consume_measurement( + Measurement( + value=0, + time_unix_nano=time_ns(), + instrument=instrument1, + context=Context(), + attributes=case["measurement_attributes"], + ) + ) + + self.assertEqual( + view_instrument_match._attributes_aggregation, + { + _hash_attributes(case["expected_attributes"]): (self.mock_created_aggregation), + }, + ) + def test_consume_measurement_without_filter_preserves_attributes(self): + instrument = Mock(name="instrument1") + instrument.instrumentation_scope = ( + self.mock_instrumentation_scope + ) + view_instrument_match = _ViewInstrumentMatch( + view=View( + instrument_name="instrument1", + name="name", + aggregation=self.mock_aggregation_factory, + ), + instrument=instrument, + instrument_class_aggregation=MagicMock( + **{"__getitem__.return_value": DefaultAggregation()} + ), + ) + + for region in ("us", "eu"): + view_instrument_match.consume_measurement( + Measurement( + value=1, + time_unix_nano=time_ns(), + instrument=instrument, + context=Context(), + attributes={"region": region}, + ) + ) + + self.assertEqual( + set(view_instrument_match._attributes_aggregation), + { + _hash_attributes({"region": "us"}), + _hash_attributes({"region": "eu"}), + }, + ) + + def test_consume_measurement_applies_exclusion_after_inclusion( + self, + ): + instrument = Mock(name="instrument1") + instrument.instrumentation_scope = ( + self.mock_instrumentation_scope + ) + view_instrument_match = _ViewInstrumentMatch( + view=View( + instrument_name="instrument1", + name="name", + aggregation=self.mock_aggregation_factory, + attribute_keys={"http.*"}, + exclude_attribute_keys={"*.status"}, + ), + instrument=instrument, + instrument_class_aggregation=MagicMock( + **{"__getitem__.return_value": DefaultAggregation()} + ), + ) + + view_instrument_match.consume_measurement( + Measurement( + value=1, + time_unix_nano=time_ns(), + instrument=instrument, + context=Context(), + attributes={ + "http.method": "GET", + "http.status": "200", + "region": "us", + }, + ) + ) + + self.assertEqual( + view_instrument_match._attributes_aggregation, + { + _hash_attributes( + {"http.method": "GET"} + ): self.mock_created_aggregation, + }, + ) + + def test_consume_measurement_exclusion_wins_exact_overlap( + self, + ): + instrument = Mock(name="instrument1") + instrument.instrumentation_scope = ( + self.mock_instrumentation_scope + ) + view_instrument_match = _ViewInstrumentMatch( + view=View( + instrument_name="instrument1", + name="name", + aggregation=self.mock_aggregation_factory, + attribute_keys={"region"}, + exclude_attribute_keys={"region"}, + ), + instrument=instrument, + instrument_class_aggregation=MagicMock( + **{"__getitem__.return_value": DefaultAggregation()} + ), + ) + + view_instrument_match.consume_measurement( + Measurement( + value=1, + time_unix_nano=time_ns(), + instrument=instrument, + context=Context(), + attributes={ + "region": "us", + "host": "server1", + }, + ) + ) + + self.assertEqual( + view_instrument_match._attributes_aggregation, + { + _hash_attributes( + {} + ): self.mock_created_aggregation, + }, + ) + + def test_consume_measurement_bracketed_inclusion_is_literal( + self, + ): + instrument = Mock(name="instrument1") + instrument.instrumentation_scope = ( + self.mock_instrumentation_scope + ) + view_instrument_match = _ViewInstrumentMatch( + view=View( + instrument_name="instrument1", + name="name", + aggregation=self.mock_aggregation_factory, + attribute_keys={"header[0]"}, + ), + instrument=instrument, + instrument_class_aggregation=MagicMock( + **{"__getitem__.return_value": DefaultAggregation()} + ), + ) + + view_instrument_match.consume_measurement( + Measurement( + value=1, + time_unix_nano=time_ns(), + instrument=instrument, + context=Context(), + attributes={ + "header[0]": "literal", + "header0": "lookalike", + }, + ) + ) + + self.assertEqual( + view_instrument_match._attributes_aggregation, + { + _hash_attributes( + {"header[0]": "literal"} + ): self.mock_created_aggregation, + }, + ) + + def test_consume_measurement_question_mark_pattern_overlap( + self, + ): + instrument = Mock(name="instrument1") + instrument.instrumentation_scope = ( + self.mock_instrumentation_scope + ) + view_instrument_match = _ViewInstrumentMatch( + view=View( + instrument_name="instrument1", + name="name", + aggregation=self.mock_aggregation_factory, + attribute_keys={"a?"}, + exclude_attribute_keys={"?b"}, + ), + instrument=instrument, + instrument_class_aggregation=MagicMock( + **{"__getitem__.return_value": DefaultAggregation()} + ), + ) + + view_instrument_match.consume_measurement( + Measurement( + value=1, + time_unix_nano=time_ns(), + instrument=instrument, + context=Context(), + attributes={ + "ab": "exclude", + "ac": "keep", + "abc": "not-included", + }, + ) + ) + + self.assertEqual( + view_instrument_match._attributes_aggregation, + { + _hash_attributes( + {"ac": "keep"} + ): self.mock_created_aggregation, + }, + ) + def test_consume_measurement(self): instrument1 = Mock(name="instrument1") instrument1.instrumentation_scope = self.mock_instrumentation_scope @@ -119,6 +445,44 @@ def test_consume_measurement(self): }, ) + # wildcard attribute_keys should match multiple attributes + view_instrument_match = _ViewInstrumentMatch( + view=View( + instrument_name="instrument1", + name="name", + aggregation=self.mock_aggregation_factory, + attribute_keys={"k8s.*"}, + ), + instrument=instrument1, + instrument_class_aggregation=MagicMock(**{"__getitem__.return_value": DefaultAggregation()}), + ) + + view_instrument_match.consume_measurement( + Measurement( + value=0, + time_unix_nano=time_ns(), + instrument=instrument1, + context=Context(), + attributes={ + "k8s.node": "node1", + "k8s.cluster": "cluster1", + "service.name": "svc", + }, + ) + ) + + self.assertEqual( + view_instrument_match._attributes_aggregation, + { + _hash_attributes( + { + "k8s.node": "node1", + "k8s.cluster": "cluster1", + } + ): self.mock_created_aggregation, + }, + ) + # setup new instrument match without `attribute_keys` set, defaults to keeping # all attributes. view_instrument_match = _ViewInstrumentMatch(