Skip to content

Commit 22bfe82

Browse files
ocelotlocelotl
authored
Refactor instrument name, unit and description checks (#2849)
Fixes #2848
1 parent d8f32c9 commit 22bfe82

File tree

3 files changed

+143
-33
lines changed

3 files changed

+143
-33
lines changed

‎opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py‎

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,12 @@
2121
from re import compile as re_compile
2222
from typing import (
2323
Callable,
24+
Dict,
2425
Generator,
2526
Generic,
2627
Iterable,
2728
Optional,
2829
Sequence,
29-
Tuple,
3030
TypeVar,
3131
Union,
3232
)
@@ -74,20 +74,39 @@ def __init__(
7474
pass
7575

7676
@staticmethod
77-
def _check_name_and_unit(name: str, unit: str) -> Tuple[bool, bool]:
77+
def _check_name_unit_description(
78+
name: str, unit: str, description: str
79+
) -> Dict[str, Optional[str]]:
7880
"""
79-
Checks the following instrument name and unit for compliance with the
80-
spec.
81+
Checks the following instrument name, unit and description for
82+
compliance with the spec.
8183
82-
Returns a tuple of boolean value, the first one will be `True` if the
83-
name is valid, `False` otherwise. The second value will be `True` if
84-
the unit is valid, `False` otherwise.
84+
Returns a dict with keys "name", "unit" and "description", the
85+
corresponding values will be the checked strings or `None` if the value
86+
is invalid. If valid, the checked strings should be used instead of the
87+
original values.
8588
"""
8689

87-
return (
88-
_name_regex.fullmatch(name) is not None,
89-
_unit_regex.fullmatch(unit) is not None,
90-
)
90+
result: Dict[str, Optional[str]] = {}
91+
92+
if _name_regex.fullmatch(name) is not None:
93+
result["name"] = name
94+
else:
95+
result["name"] = None
96+
97+
if unit is None:
98+
unit = ""
99+
if _unit_regex.fullmatch(unit) is not None:
100+
result["unit"] = unit
101+
else:
102+
result["unit"] = None
103+
104+
if description is None:
105+
result["description"] = ""
106+
else:
107+
result["description"] = description
108+
109+
return result
91110

92111

93112
class _ProxyInstrument(ABC, Generic[InstrumentT]):

‎opentelemetry-api/tests/metrics/test_instruments.py‎

Lines changed: 97 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -563,27 +563,108 @@ def test_observable_up_down_counter_callback(self):
563563
Signature.empty,
564564
)
565565

566-
def test_name_regex(self):
566+
def test_name_check(self):
567567

568568
instrument = ChildInstrument("name")
569569

570-
self.assertTrue(instrument._check_name_and_unit("a" * 63, "unit")[0])
571-
self.assertTrue(instrument._check_name_and_unit("a.", "unit")[0])
572-
self.assertTrue(instrument._check_name_and_unit("a-", "unit")[0])
573-
self.assertTrue(instrument._check_name_and_unit("a_", "unit")[0])
574-
575-
self.assertFalse(instrument._check_name_and_unit("a" * 64, "unit")[0])
576-
self.assertFalse(instrument._check_name_and_unit("Ñ", "unit")[0])
577-
self.assertFalse(instrument._check_name_and_unit("_a", "unit")[0])
578-
self.assertFalse(instrument._check_name_and_unit("1a", "unit")[0])
579-
self.assertFalse(instrument._check_name_and_unit("", "unit")[0])
570+
self.assertEqual(
571+
instrument._check_name_unit_description(
572+
"a" * 63, "unit", "description"
573+
)["name"],
574+
"a" * 63,
575+
)
576+
self.assertEqual(
577+
instrument._check_name_unit_description(
578+
"a.", "unit", "description"
579+
)["name"],
580+
"a.",
581+
)
582+
self.assertEqual(
583+
instrument._check_name_unit_description(
584+
"a-", "unit", "description"
585+
)["name"],
586+
"a-",
587+
)
588+
self.assertEqual(
589+
instrument._check_name_unit_description(
590+
"a_", "unit", "description"
591+
)["name"],
592+
"a_",
593+
)
594+
595+
self.assertIsNone(
596+
instrument._check_name_unit_description(
597+
"a" * 64, "unit", "description"
598+
)["name"]
599+
)
600+
self.assertIsNone(
601+
instrument._check_name_unit_description(
602+
"Ñ", "unit", "description"
603+
)["name"]
604+
)
605+
self.assertIsNone(
606+
instrument._check_name_unit_description(
607+
"_a", "unit", "description"
608+
)["name"]
609+
)
610+
self.assertIsNone(
611+
instrument._check_name_unit_description(
612+
"1a", "unit", "description"
613+
)["name"]
614+
)
615+
self.assertIsNone(
616+
instrument._check_name_unit_description("", "unit", "description")[
617+
"name"
618+
]
619+
)
580620

581-
def test_unit_regex(self):
621+
def test_unit_check(self):
582622

583623
instrument = ChildInstrument("name")
584624

585-
self.assertTrue(instrument._check_name_and_unit("name", "a" * 63)[1])
586-
self.assertTrue(instrument._check_name_and_unit("name", "{a}")[1])
625+
self.assertEqual(
626+
instrument._check_name_unit_description(
627+
"name", "a" * 63, "description"
628+
)["unit"],
629+
"a" * 63,
630+
)
631+
self.assertEqual(
632+
instrument._check_name_unit_description(
633+
"name", "{a}", "description"
634+
)["unit"],
635+
"{a}",
636+
)
637+
638+
self.assertIsNone(
639+
instrument._check_name_unit_description(
640+
"name", "a" * 64, "description"
641+
)["unit"]
642+
)
643+
self.assertIsNone(
644+
instrument._check_name_unit_description(
645+
"name", "Ñ", "description"
646+
)["unit"]
647+
)
648+
self.assertEqual(
649+
instrument._check_name_unit_description(
650+
"name", None, "description"
651+
)["unit"],
652+
"",
653+
)
654+
655+
def test_description_check(self):
587656

588-
self.assertFalse(instrument._check_name_and_unit("name", "a" * 64)[1])
589-
self.assertFalse(instrument._check_name_and_unit("name", "Ñ")[1])
657+
instrument = ChildInstrument("name")
658+
659+
self.assertEqual(
660+
instrument._check_name_unit_description(
661+
"name", "unit", "description"
662+
)["description"],
663+
"description",
664+
)
665+
self.assertEqual(
666+
instrument._check_name_unit_description("name", "unit", None)[
667+
"description"
668+
],
669+
"",
670+
)

‎opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,13 +50,18 @@ def __init__(
5050
description: str = "",
5151
):
5252
# pylint: disable=no-member
53-
is_name_valid, is_unit_valid = self._check_name_and_unit(name, unit)
53+
result = self._check_name_unit_description(name, unit, description)
5454

55-
if not is_name_valid:
55+
if result["name"] is None:
5656
raise Exception(_ERROR_MESSAGE.format(name))
5757

58-
if not is_unit_valid:
58+
if result["unit"] is None:
5959
raise Exception(_ERROR_MESSAGE.format(unit))
60+
61+
name = result["name"]
62+
unit = result["unit"]
63+
description = result["description"]
64+
6065
self.name = name.lower()
6166
self.unit = unit
6267
self.description = description
@@ -76,13 +81,18 @@ def __init__(
7681
description: str = "",
7782
):
7883
# pylint: disable=no-member
79-
is_name_valid, is_unit_valid = self._check_name_and_unit(name, unit)
84+
result = self._check_name_unit_description(name, unit, description)
8085

81-
if not is_name_valid:
86+
if result["name"] is None:
8287
raise Exception(_ERROR_MESSAGE.format(name))
8388

84-
if not is_unit_valid:
89+
if result["unit"] is None:
8590
raise Exception(_ERROR_MESSAGE.format(unit))
91+
92+
name = result["name"]
93+
unit = result["unit"]
94+
description = result["description"]
95+
8696
self.name = name.lower()
8797
self.unit = unit
8898
self.description = description

0 commit comments

Comments
 (0)