From ff034b25a4a9a589fc5126e1a14ea7ba33ab151a Mon Sep 17 00:00:00 2001 From: ersalil Date: Wed, 10 Jun 2026 02:58:36 +0530 Subject: [PATCH 1/4] fix(metrics): provide distinct, accurate error messages for instrument name and unit validation; add tests --- .../sdk/metrics/_internal/instrument.py | 15 ++++++---- .../metrics/test_instrument_error_message.py | 28 +++++++++++++++++++ 2 files changed, 38 insertions(+), 5 deletions(-) create mode 100644 opentelemetry-sdk/tests/metrics/test_instrument_error_message.py diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py index 946cdf1df17..8b558e6beb2 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py @@ -45,7 +45,12 @@ _logger = getLogger(__name__) -_ERROR_MESSAGE = ( +_NAME_ERROR_MESSAGE = ( + "Instrument name must be an ASCII string, start with a letter, " + "contain only letters, digits, '_', '.', '-', '/' and be at most 255 characters; got {}" +) + +_UNIT_ERROR_MESSAGE = ( "Expected ASCII string of maximum length 63 characters but got {}" ) @@ -74,11 +79,11 @@ def __init__( if result["name"] is None: # pylint: disable=broad-exception-raised - raise Exception(_ERROR_MESSAGE.format(name)) + raise Exception(_NAME_ERROR_MESSAGE.format(name)) if result["unit"] is None: # pylint: disable=broad-exception-raised - raise Exception(_ERROR_MESSAGE.format(unit)) + raise Exception(_UNIT_ERROR_MESSAGE.format(unit)) name = result["name"] unit = result["unit"] @@ -113,11 +118,11 @@ def __init__( if result["name"] is None: # pylint: disable=broad-exception-raised - raise Exception(_ERROR_MESSAGE.format(name)) + raise Exception(_NAME_ERROR_MESSAGE.format(name)) if result["unit"] is None: # pylint: disable=broad-exception-raised - raise Exception(_ERROR_MESSAGE.format(unit)) + raise Exception(_UNIT_ERROR_MESSAGE.format(unit)) name = result["name"] unit = result["unit"] diff --git a/opentelemetry-sdk/tests/metrics/test_instrument_error_message.py b/opentelemetry-sdk/tests/metrics/test_instrument_error_message.py new file mode 100644 index 00000000000..65818710042 --- /dev/null +++ b/opentelemetry-sdk/tests/metrics/test_instrument_error_message.py @@ -0,0 +1,28 @@ +import unittest + +from opentelemetry.sdk.util.instrumentation import InstrumentationScope + +from opentelemetry.sdk.metrics._internal.instrument import _Synchronous + + +class TestInstrumentErrorMessage(unittest.TestCase): + def test_invalid_name_error_message(self): + scope = InstrumentationScope("test") + with self.assertRaises(Exception) as cm: + _Synchronous("1invalid", scope, object()) + msg = str(cm.exception) + self.assertIn("start with a letter", msg) + self.assertIn("255", msg) + + def test_invalid_unit_error_message(self): + scope = InstrumentationScope("test") + # name valid, unit invalid (non-ASCII) + with self.assertRaises(Exception) as cm: + _Synchronous("validname", scope, object(), unit="Ñ") + msg = str(cm.exception) + self.assertIn("maximum length 63", msg) + self.assertIn("ASCII", msg) + + +if __name__ == "__main__": + unittest.main() From ad23ad0cd83f5a2020ff5471b694e2e06bb9a491 Mon Sep 17 00:00:00 2001 From: ersalil Date: Wed, 10 Jun 2026 03:09:28 +0530 Subject: [PATCH 2/4] chore(changelog): add fragment for PR #5283 --- .changelog/5283.changed | 1 + 1 file changed, 1 insertion(+) create mode 100644 .changelog/5283.changed diff --git a/.changelog/5283.changed b/.changelog/5283.changed new file mode 100644 index 00000000000..f7b3a94391d --- /dev/null +++ b/.changelog/5283.changed @@ -0,0 +1 @@ +metrics: clarify instrument name validation error message to reflect spec constraints (start with a letter, allowed chars, max length 255) From f3b3f07b9e33e401ac4e9044174e7cb178e0f65b Mon Sep 17 00:00:00 2001 From: Salil Agrawal <56884743+ersalil@users.noreply.github.com> Date: Thu, 11 Jun 2026 10:11:15 +0530 Subject: [PATCH 3/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../tests/metrics/test_instrument_error_message.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/opentelemetry-sdk/tests/metrics/test_instrument_error_message.py b/opentelemetry-sdk/tests/metrics/test_instrument_error_message.py index 65818710042..9e3cc778c3b 100644 --- a/opentelemetry-sdk/tests/metrics/test_instrument_error_message.py +++ b/opentelemetry-sdk/tests/metrics/test_instrument_error_message.py @@ -1,9 +1,7 @@ import unittest -from opentelemetry.sdk.util.instrumentation import InstrumentationScope - from opentelemetry.sdk.metrics._internal.instrument import _Synchronous - +from opentelemetry.sdk.util.instrumentation import InstrumentationScope class TestInstrumentErrorMessage(unittest.TestCase): def test_invalid_name_error_message(self): From e62c5baa573967c1612d76109518fc2f988998b0 Mon Sep 17 00:00:00 2001 From: ersalil Date: Tue, 30 Jun 2026 20:41:02 +0530 Subject: [PATCH 4/4] fix(metrics): address PR review feedback - changelog: use full package name and cover both name and unit validation - instrument.py: use {!r} when formatting offending value; make unit message reference the unit field for parity with the name message - tests: assert precise "at most 255 characters" substring and drop the unnecessary unittest.main() entrypoint Co-Authored-By: Claude Opus 4.8 (1M context) --- .changelog/5283.changed | 2 +- .../src/opentelemetry/sdk/metrics/_internal/instrument.py | 4 ++-- .../tests/metrics/test_instrument_error_message.py | 7 ++----- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/.changelog/5283.changed b/.changelog/5283.changed index f7b3a94391d..e347df27edc 100644 --- a/.changelog/5283.changed +++ b/.changelog/5283.changed @@ -1 +1 @@ -metrics: clarify instrument name validation error message to reflect spec constraints (start with a letter, allowed chars, max length 255) +`opentelemetry-sdk`: provide distinct, accurate error messages for instrument name and unit validation diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py index 8b558e6beb2..a21daadafb1 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py @@ -47,11 +47,11 @@ _NAME_ERROR_MESSAGE = ( "Instrument name must be an ASCII string, start with a letter, " - "contain only letters, digits, '_', '.', '-', '/' and be at most 255 characters; got {}" + "contain only letters, digits, '_', '.', '-', '/' and be at most 255 characters; got {!r}" ) _UNIT_ERROR_MESSAGE = ( - "Expected ASCII string of maximum length 63 characters but got {}" + "Instrument unit must be an ASCII string of maximum length 63 characters; got {!r}" ) diff --git a/opentelemetry-sdk/tests/metrics/test_instrument_error_message.py b/opentelemetry-sdk/tests/metrics/test_instrument_error_message.py index 9e3cc778c3b..f9f03634271 100644 --- a/opentelemetry-sdk/tests/metrics/test_instrument_error_message.py +++ b/opentelemetry-sdk/tests/metrics/test_instrument_error_message.py @@ -3,6 +3,7 @@ from opentelemetry.sdk.metrics._internal.instrument import _Synchronous from opentelemetry.sdk.util.instrumentation import InstrumentationScope + class TestInstrumentErrorMessage(unittest.TestCase): def test_invalid_name_error_message(self): scope = InstrumentationScope("test") @@ -10,7 +11,7 @@ def test_invalid_name_error_message(self): _Synchronous("1invalid", scope, object()) msg = str(cm.exception) self.assertIn("start with a letter", msg) - self.assertIn("255", msg) + self.assertIn("at most 255 characters", msg) def test_invalid_unit_error_message(self): scope = InstrumentationScope("test") @@ -20,7 +21,3 @@ def test_invalid_unit_error_message(self): msg = str(cm.exception) self.assertIn("maximum length 63", msg) self.assertIn("ASCII", msg) - - -if __name__ == "__main__": - unittest.main()