diff --git a/opentelemetry-api/src/opentelemetry/_metrics/__init__.py b/opentelemetry-api/src/opentelemetry/_metrics/__init__.py index a2f430c5f7d..b3a2ee5563c 100644 --- a/opentelemetry-api/src/opentelemetry/_metrics/__init__.py +++ b/opentelemetry-api/src/opentelemetry/_metrics/__init__.py @@ -26,7 +26,7 @@ from logging import getLogger from os import environ from threading import Lock -from typing import List, Optional, cast +from typing import List, Optional, Set, cast from opentelemetry._metrics.instrument import ( Counter, @@ -118,7 +118,8 @@ def __init__(self, name, version=None, schema_url=None): self._name = name self._version = version self._schema_url = schema_url - self._instrument_names = set() + self._instrument_ids: Set[str] = set() + self._instrument_ids_lock = Lock() @property def name(self): @@ -132,7 +133,27 @@ def version(self): def schema_url(self): return self._schema_url - # FIXME check that the instrument name has not been used already + def _check_instrument_id( + self, name: str, type_: type, unit: str, description: str + ) -> bool: + """ + Check if an instrument with the same name, type, unit and description + has been registered already. + """ + + result = False + + instrument_id = ",".join( + [name.strip().lower(), type_.__name__, unit, description] + ) + + with self._instrument_ids_lock: + if instrument_id in self._instrument_ids: + result = True + else: + self._instrument_ids.add(instrument_id) + + return result @abstractmethod def create_counter(self, name, unit="", description="") -> Counter: @@ -401,6 +422,15 @@ class NoOpMeter(Meter): def create_counter(self, name, unit="", description="") -> Counter: """Returns a no-op Counter.""" super().create_counter(name, unit=unit, description=description) + if self._check_instrument_id(name, DefaultCounter, unit, description): + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + Counter.__name__, + unit, + description, + ) return DefaultCounter(name, unit=unit, description=description) def create_up_down_counter( @@ -410,6 +440,17 @@ def create_up_down_counter( super().create_up_down_counter( name, unit=unit, description=description ) + if self._check_instrument_id( + name, DefaultUpDownCounter, unit, description + ): + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + UpDownCounter.__name__, + unit, + description, + ) return DefaultUpDownCounter(name, unit=unit, description=description) def create_observable_counter( @@ -419,6 +460,17 @@ def create_observable_counter( super().create_observable_counter( name, callback, unit=unit, description=description ) + if self._check_instrument_id( + name, DefaultObservableCounter, unit, description + ): + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + ObservableCounter.__name__, + unit, + description, + ) return DefaultObservableCounter( name, callback, @@ -429,6 +481,17 @@ def create_observable_counter( def create_histogram(self, name, unit="", description="") -> Histogram: """Returns a no-op Histogram.""" super().create_histogram(name, unit=unit, description=description) + if self._check_instrument_id( + name, DefaultHistogram, unit, description + ): + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + Histogram.__name__, + unit, + description, + ) return DefaultHistogram(name, unit=unit, description=description) def create_observable_gauge( @@ -438,6 +501,17 @@ def create_observable_gauge( super().create_observable_gauge( name, callback, unit=unit, description=description ) + if self._check_instrument_id( + name, DefaultObservableGauge, unit, description + ): + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + ObservableGauge.__name__, + unit, + description, + ) return DefaultObservableGauge( name, callback, @@ -452,6 +526,17 @@ def create_observable_up_down_counter( super().create_observable_up_down_counter( name, callback, unit=unit, description=description ) + if self._check_instrument_id( + name, DefaultObservableUpDownCounter, unit, description + ): + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + ObservableUpDownCounter.__name__, + unit, + description, + ) return DefaultObservableUpDownCounter( name, callback, diff --git a/opentelemetry-api/tests/metrics/test_meter.py b/opentelemetry-api/tests/metrics/test_meter.py index 6834df31f87..3b7f702dfba 100644 --- a/opentelemetry-api/tests/metrics/test_meter.py +++ b/opentelemetry-api/tests/metrics/test_meter.py @@ -13,9 +13,11 @@ # limitations under the License. # type: ignore +from logging import WARNING from unittest import TestCase +from unittest.mock import Mock -from opentelemetry._metrics import Meter +from opentelemetry._metrics import Meter, NoOpMeter # FIXME Test that the meter methods can be called concurrently safely. @@ -53,6 +55,42 @@ def create_observable_up_down_counter( class TestMeter(TestCase): + def test_repeated_instrument_names(self): + + try: + test_meter = NoOpMeter("name") + + test_meter.create_counter("counter") + test_meter.create_up_down_counter("up_down_counter") + test_meter.create_observable_counter("observable_counter", Mock()) + test_meter.create_histogram("histogram") + test_meter.create_observable_gauge("observable_gauge", Mock()) + test_meter.create_observable_up_down_counter( + "observable_up_down_counter", Mock() + ) + except Exception as error: + self.fail(f"Unexpected exception raised {error}") + + for instrument_name in [ + "counter", + "up_down_counter", + "histogram", + ]: + with self.assertLogs(level=WARNING): + getattr(test_meter, f"create_{instrument_name}")( + instrument_name + ) + + for instrument_name in [ + "observable_counter", + "observable_gauge", + "observable_up_down_counter", + ]: + with self.assertLogs(level=WARNING): + getattr(test_meter, f"create_{instrument_name}")( + instrument_name, Mock() + ) + def test_create_counter(self): """ Test that the meter provides a function to create a new Counter diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/__init__.py index 8a87bb1fa2c..fc71a15cf7b 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/__init__.py @@ -64,7 +64,20 @@ def __init__( self._instrumentation_info = instrumentation_info self._measurement_consumer = measurement_consumer - def create_counter(self, name, unit=None, description=None) -> APICounter: + def create_counter(self, name, unit="", description="") -> APICounter: + if self._check_instrument_id(name, Counter, unit, description): + # FIXME #2558 go through all views here and check if this + # instrument registration conflict can be fixed. If it can be, do + # not log the following warning. + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + APICounter.__name__, + unit, + description, + ) + return Counter( name, self._instrumentation_info, @@ -74,8 +87,21 @@ def create_counter(self, name, unit=None, description=None) -> APICounter: ) def create_up_down_counter( - self, name, unit=None, description=None + self, name, unit="", description="" ) -> APIUpDownCounter: + if self._check_instrument_id(name, UpDownCounter, unit, description): + # FIXME #2558 go through all views here and check if this + # instrument registration conflict can be fixed. If it can be, do + # not log the following warning. + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + APIUpDownCounter.__name__, + unit, + description, + ) + return UpDownCounter( name, self._instrumentation_info, @@ -85,9 +111,22 @@ def create_up_down_counter( ) def create_observable_counter( - self, name, callback, unit=None, description=None + self, name, callback, unit="", description="" ) -> APIObservableCounter: - + if self._check_instrument_id( + name, ObservableCounter, unit, description + ): + # FIXME #2558 go through all views here and check if this + # instrument registration conflict can be fixed. If it can be, do + # not log the following warning. + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + APIObservableCounter.__name__, + unit, + description, + ) instrument = ObservableCounter( name, self._instrumentation_info, @@ -101,9 +140,19 @@ def create_observable_counter( return instrument - def create_histogram( - self, name, unit=None, description=None - ) -> APIHistogram: + def create_histogram(self, name, unit="", description="") -> APIHistogram: + if self._check_instrument_id(name, Histogram, unit, description): + # FIXME #2558 go through all views here and check if this + # instrument registration conflict can be fixed. If it can be, do + # not log the following warning. + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + APIHistogram.__name__, + unit, + description, + ) return Histogram( name, self._instrumentation_info, @@ -113,8 +162,20 @@ def create_histogram( ) def create_observable_gauge( - self, name, callback, unit=None, description=None + self, name, callback, unit="", description="" ) -> APIObservableGauge: + if self._check_instrument_id(name, ObservableGauge, unit, description): + # FIXME #2558 go through all views here and check if this + # instrument registration conflict can be fixed. If it can be, do + # not log the following warning. + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + APIObservableGauge.__name__, + unit, + description, + ) instrument = ObservableGauge( name, @@ -130,8 +191,22 @@ def create_observable_gauge( return instrument def create_observable_up_down_counter( - self, name, callback, unit=None, description=None + self, name, callback, unit="", description="" ) -> APIObservableUpDownCounter: + if self._check_instrument_id( + name, ObservableUpDownCounter, unit, description + ): + # FIXME #2558 go through all views here and check if this + # instrument registration conflict can be fixed. If it can be, do + # not log the following warning. + _logger.warning( + "An instrument with name %s, type %s, unit %s and " + "description %s has been created already.", + name, + APIObservableUpDownCounter.__name__, + unit, + description, + ) instrument = ObservableUpDownCounter( name, @@ -280,6 +355,8 @@ def get_meter( info = InstrumentationInfo(name, version, schema_url) with self._meter_lock: if not self._meters.get(info): + # FIXME #2558 pass SDKConfig object to meter so that the meter + # has access to views. self._meters[info] = Meter( info, self._measurement_consumer, diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index 07d9894c969..d33002c9211 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -243,17 +243,17 @@ def test_register_asynchronous_instrument( meter_provider._measurement_consumer.register_asynchronous_instrument.assert_called_with( meter_provider.get_meter("name").create_observable_counter( - "name", Mock() + "name0", Mock() ) ) meter_provider._measurement_consumer.register_asynchronous_instrument.assert_called_with( meter_provider.get_meter("name").create_observable_up_down_counter( - "name", Mock() + "name1", Mock() ) ) meter_provider._measurement_consumer.register_asynchronous_instrument.assert_called_with( meter_provider.get_meter("name").create_observable_gauge( - "name", Mock() + "name2", Mock() ) ) @@ -298,6 +298,39 @@ class TestMeter(TestCase): def setUp(self): self.meter = Meter(Mock(), Mock()) + def test_repeated_instrument_names(self): + try: + self.meter.create_counter("counter") + self.meter.create_up_down_counter("up_down_counter") + self.meter.create_observable_counter("observable_counter", Mock()) + self.meter.create_histogram("histogram") + self.meter.create_observable_gauge("observable_gauge", Mock()) + self.meter.create_observable_up_down_counter( + "observable_up_down_counter", Mock() + ) + except Exception as error: + self.fail(f"Unexpected exception raised {error}") + + for instrument_name in [ + "counter", + "up_down_counter", + "histogram", + ]: + with self.assertLogs(level=WARNING): + getattr(self.meter, f"create_{instrument_name}")( + instrument_name + ) + + for instrument_name in [ + "observable_counter", + "observable_gauge", + "observable_up_down_counter", + ]: + with self.assertLogs(level=WARNING): + getattr(self.meter, f"create_{instrument_name}")( + instrument_name, Mock() + ) + def test_create_counter(self): counter = self.meter.create_counter( "name", unit="unit", description="description"