opentelemetry-api: fix already registered instrumentation check

Author: xrmx

opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py

@@ -45,7 +45,7 @@
 from logging import getLogger
 from os import environ
 from threading import Lock
-from typing import List, Optional, Sequence, Set, Tuple, Union, cast
+from typing import Dict, List, Optional, Sequence, Tuple, Union, cast
 
 from opentelemetry.environment_variables import OTEL_PYTHON_METER_PROVIDER
 from opentelemetry.metrics._internal.instrument import (
@@ -194,7 +194,7 @@ def __init__(
         self._name = name
         self._version = version
         self._schema_url = schema_url
-        self._instrument_ids: Set[str] = set()
+        self._instrument_ids: Dict[str, str] = {}
         self._instrument_ids_lock = Lock()
 
     @property
@@ -222,25 +222,25 @@ def _is_instrument_registered(
         self, name: str, type_: type, unit: str, description: str
     ) -> Tuple[bool, str]:
         """
-        Check if an instrument with the same name, type, unit and description
-        has been registered already.
+        Check if an instrument with the same name and type but different
+        unit or description has been registered already.
 
         Returns a tuple. The first value is `True` if the instrument has been
         registered already, `False` otherwise. The second value is the
         instrument id.
         """
 
-        instrument_id = ",".join(
-            [name.strip().lower(), type_.__name__, unit, description]
-        )
+        instrument_id = ",".join([name.strip().lower(), type_.__name__])
+        instrument_value = ",".join([unit, description])
 
         result = False
 
         with self._instrument_ids_lock:
-            if instrument_id in self._instrument_ids:
+            current_value = self._instrument_ids.get(instrument_id)
+            if current_value is not None and current_value != instrument_value:
                 result = True
             else:
-                self._instrument_ids.add(instrument_id)
+                self._instrument_ids[instrument_id] = instrument_value
 
         return (result, instrument_id)
 

opentelemetry-api/tests/metrics/test_meter.py

@@ -61,8 +61,42 @@ def create_observable_up_down_counter(
 
 
 class TestMeter(TestCase):
-    # pylint: disable=no-member
-    def test_repeated_instrument_names(self):
+    def test_no_warnings_on_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_gauge("gauge")
+            test_meter.create_observable_gauge("observable_gauge", Mock())
+            test_meter.create_observable_up_down_counter(
+                "observable_up_down_counter", Mock()
+            )
+        except Exception as error:  # pylint: disable=broad-exception-caught
+            self.fail(f"Unexpected exception raised {error}")
+
+        for instrument_name in [
+            "counter",
+            "up_down_counter",
+            "histogram",
+            "gauge",
+        ]:
+            with self.assertNoLogs(level=WARNING):
+                create_func = getattr(test_meter, f"create_{instrument_name}")
+                create_func(instrument_name)
+
+        for instrument_name in [
+            "observable_counter",
+            "observable_gauge",
+            "observable_up_down_counter",
+        ]:
+            with self.assertNoLogs(level=WARNING):
+                create_func = getattr(test_meter, f"create_{instrument_name}")
+                create_func(instrument_name, Mock())
+
+    def test_warnings_on_repeated_instrument_names_with_different_unit(self):
         try:
             test_meter = NoOpMeter("name")
 
@@ -85,18 +119,55 @@ def test_repeated_instrument_names(self):
             "gauge",
         ]:
             with self.assertLogs(level=WARNING):
-                getattr(test_meter, f"create_{instrument_name}")(
-                    instrument_name
-                )
+                create_func = getattr(test_meter, f"create_{instrument_name}")
+                create_func(instrument_name, unit="a unit")
+
+        for instrument_name in [
+            "observable_counter",
+            "observable_gauge",
+            "observable_up_down_counter",
+        ]:
+            with self.assertLogs(level=WARNING):
+                create_func = getattr(test_meter, f"create_{instrument_name}")
+                create_func(instrument_name, Mock(), unit="a unit")
+
+    def test_warnings_on_repeated_instrument_names_with_different_description(
+        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_gauge("gauge")
+            test_meter.create_observable_gauge("observable_gauge", Mock())
+            test_meter.create_observable_up_down_counter(
+                "observable_up_down_counter", Mock()
+            )
+        except Exception as error:  # pylint: disable=broad-exception-caught
+            self.fail(f"Unexpected exception raised {error}")
+
+        for instrument_name in [
+            "counter",
+            "up_down_counter",
+            "histogram",
+            "gauge",
+        ]:
+            with self.assertLogs(level=WARNING):
+                create_func = getattr(test_meter, f"create_{instrument_name}")
+                create_func(instrument_name, description="a description")
 
         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()
+                create_func = getattr(test_meter, f"create_{instrument_name}")
+                create_func(
+                    instrument_name, Mock(), description="a description"
                 )
 
     def test_create_counter(self):