Check instrument names and units (#2648)

* Check instrument names and units Fixes #2647 * Update opentelemetry-api/src/opentelemetry/_metrics/instrument.py Co-authored-by: Srikanth Chekuri <[email protected]> * Add missing type * Add missing type * Fix error message * Update opentelemetry-api/src/opentelemetry/_metrics/_internal/instrument.py Co-authored-by: Srikanth Chekuri <[email protected]> Co-authored-by: Srikanth Chekuri <[email protected]>
open-telemetry · Apr 29, 2022 · 5456988 · 5456988
1 parent 2972ebc
commit 5456988
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 14 deletions.
diff --git a/opentelemetry-api/src/opentelemetry/_metrics/_internal/instrument.py b/opentelemetry-api/src/opentelemetry/_metrics/_internal/instrument.py
@@ -17,13 +17,16 @@
 
 from abc import ABC, abstractmethod
 from logging import getLogger
+from re import ASCII
+from re import compile as re_compile
 from typing import (
     Callable,
     Generator,
     Generic,
     Iterable,
     Optional,
     Sequence,
+    Tuple,
     TypeVar,
     Union,
 )
@@ -42,6 +45,9 @@
 
 _logger = getLogger(__name__)
 
+_name_regex = re_compile(r"[a-zA-Z][-.\w]{0,62}", ASCII)
+_unit_regex = re_compile(r"\w{0,63}", ASCII)
+
 
 class Instrument(ABC):
     """Abstract class that serves as base for all instruments."""
@@ -55,9 +61,21 @@ def __init__(
     ) -> None:
         pass
 
-        # FIXME check that the instrument name is valid
-        # FIXME check that the unit is 63 characters or shorter
-        # FIXME check that the unit contains only ASCII characters
+    @staticmethod
+    def _check_name_and_unit(name: str, unit: str) -> Tuple[bool, bool]:
+        """
+        Checks the following instrument name and unit for compliance with the
+        spec.
+
+        Returns a tuple of boolean value, the first one will be `True` if the
+        name is valid, `False` otherwise. The second value will be `True` if
+        the unit is valid, `False` otherwise.
+        """
+
+        return (
+            _name_regex.fullmatch(name) is not None,
+            _unit_regex.fullmatch(unit) is not None,
+        )
 
 
 class _ProxyInstrument(ABC, Generic[InstrumentT]):
@@ -124,7 +142,6 @@ def add(
         amount: Union[int, float],
         attributes: Optional[Attributes] = None,
     ) -> None:
-        # FIXME check that the amount is non negative
         pass
 
 

diff --git a/opentelemetry-api/tests/metrics/test_instruments.py b/opentelemetry-api/tests/metrics/test_instruments.py
@@ -562,3 +562,25 @@ def test_observable_up_down_counter_callback(self):
             ].default,
             Signature.empty,
         )
+
+    def test_name_regex(self):
+
+        instrument = ChildInstrument("name")
+
+        self.assertTrue(instrument._check_name_and_unit("a" * 63, "unit")[0])
+        self.assertTrue(instrument._check_name_and_unit("a.", "unit")[0])
+        self.assertTrue(instrument._check_name_and_unit("a-", "unit")[0])
+        self.assertTrue(instrument._check_name_and_unit("a_", "unit")[0])
+        self.assertFalse(instrument._check_name_and_unit("a" * 64, "unit")[0])
+        self.assertFalse(instrument._check_name_and_unit("Ñ", "unit")[0])
+        self.assertFalse(instrument._check_name_and_unit("_a", "unit")[0])
+        self.assertFalse(instrument._check_name_and_unit("1a", "unit")[0])
+        self.assertFalse(instrument._check_name_and_unit("", "unit")[0])
+
+    def test_unit_regex(self):
+
+        instrument = ChildInstrument("name")
+
+        self.assertTrue(instrument._check_name_and_unit("name", "a" * 63)[1])
+        self.assertFalse(instrument._check_name_and_unit("name", "a" * 64)[1])
+        self.assertFalse(instrument._check_name_and_unit("name", "Ñ")[1])
diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/instrument.py b/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/instrument.py
@@ -38,6 +38,11 @@
 _logger = getLogger(__name__)
 
 
+_ERROR_MESSAGE = (
+    "Expected ASCII string of maximum length 63 characters but got {}"
+)
+
+
 class _Synchronous:
     def __init__(
         self,
@@ -47,7 +52,15 @@ def __init__(
         unit: str = "",
         description: str = "",
     ):
-        self.name = name
+        # pylint: disable=no-member
+        is_name_valid, is_unit_valid = self._check_name_and_unit(name, unit)
+
+        if not is_name_valid:
+            raise Exception(_ERROR_MESSAGE.format(name))
+
+        if not is_unit_valid:
+            raise Exception(_ERROR_MESSAGE.format(unit))
+        self.name = name.lower()
         self.unit = unit
         self.description = description
         self.instrumentation_scope = instrumentation_scope
@@ -65,7 +78,15 @@ def __init__(
         unit: str = "",
         description: str = "",
     ):
-        self.name = name
+        # pylint: disable=no-member
+        is_name_valid, is_unit_valid = self._check_name_and_unit(name, unit)
+
+        if not is_name_valid:
+            raise Exception(_ERROR_MESSAGE.format(name))
+
+        if not is_unit_valid:
+            raise Exception(_ERROR_MESSAGE.format(unit))
+        self.name = name.lower()
         self.unit = unit
         self.description = description
         self.instrumentation_scope = instrumentation_scope

diff --git a/opentelemetry-sdk/tests/metrics/test_aggregation.py b/opentelemetry-sdk/tests/metrics/test_aggregation.py
@@ -875,7 +875,7 @@ def setUpClass(cls):
     def test_counter(self):
 
         aggregation = self.default_aggregation._create_aggregation(
-            Counter(Mock(), Mock(), Mock())
+            Counter("name", Mock(), Mock())
         )
         self.assertIsInstance(aggregation, _SumAggregation)
         self.assertTrue(aggregation._instrument_is_monotonic)
@@ -886,7 +886,7 @@ def test_counter(self):
     def test_up_down_counter(self):
 
         aggregation = self.default_aggregation._create_aggregation(
-            UpDownCounter(Mock(), Mock(), Mock())
+            UpDownCounter("name", Mock(), Mock())
         )
         self.assertIsInstance(aggregation, _SumAggregation)
         self.assertFalse(aggregation._instrument_is_monotonic)
@@ -897,7 +897,7 @@ def test_up_down_counter(self):
     def test_observable_counter(self):
 
         aggregation = self.default_aggregation._create_aggregation(
-            ObservableCounter(Mock(), Mock(), Mock(), callbacks=[Mock()])
+            ObservableCounter("name", Mock(), Mock(), callbacks=[Mock()])
         )
         self.assertIsInstance(aggregation, _SumAggregation)
         self.assertTrue(aggregation._instrument_is_monotonic)
@@ -909,7 +909,7 @@ def test_observable_counter(self):
     def test_observable_up_down_counter(self):
 
         aggregation = self.default_aggregation._create_aggregation(
-            ObservableUpDownCounter(Mock(), Mock(), Mock(), callbacks=[Mock()])
+            ObservableUpDownCounter("name", Mock(), Mock(), callbacks=[Mock()])
         )
         self.assertIsInstance(aggregation, _SumAggregation)
         self.assertFalse(aggregation._instrument_is_monotonic)
@@ -922,9 +922,7 @@ def test_histogram(self):
 
         aggregation = self.default_aggregation._create_aggregation(
             Histogram(
-                Mock(),
-                Mock(),
-                Mock(),
+                "name",
                 Mock(),
                 Mock(),
             )
@@ -935,7 +933,7 @@ def test_observable_gauge(self):
 
         aggregation = self.default_aggregation._create_aggregation(
             ObservableGauge(
-                Mock(),
+                "name",
                 Mock(),
                 Mock(),
                 callbacks=[Mock()],

diff --git a/opentelemetry-sdk/tests/metrics/test_instrument.py b/opentelemetry-sdk/tests/metrics/test_instrument.py
@@ -28,6 +28,10 @@
 
 
 class TestCounter(TestCase):
+    def testname(self):
+        self.assertEqual(Counter("name", Mock(), Mock()).name, "name")
+        self.assertEqual(Counter("Name", Mock(), Mock()).name, "name")
+
     def test_add(self):
         mc = Mock()
         counter = Counter("name", Mock(), mc)
@@ -91,6 +95,10 @@ def generator_callback_1():
 
 
 class TestObservableGauge(TestCase):
+    def testname(self):
+        self.assertEqual(ObservableGauge("name", Mock(), Mock()).name, "name")
+        self.assertEqual(ObservableGauge("Name", Mock(), Mock()).name, "name")
+
     def test_callable_callback_0(self):
         observable_gauge = ObservableGauge(
             "name", Mock(), Mock(), [callable_callback_0]