fix(metrics): accurate error messages for instrument name and unit validation#5283
fix(metrics): accurate error messages for instrument name and unit validation#5283ersalil wants to merge 3 commits into
Conversation
…t name and unit validation; add tests
|
|
|
@ersalil can you please confirm the actual code is doing the validation you expect, and if you're able to link to the spec or spec matrix for this, it would be helpful. |
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK metrics instrument construction path to emit distinct, spec-accurate validation error messages for invalid instrument names vs invalid units, and adds regression coverage to prevent message mix-ups in the future.
Changes:
- Introduced dedicated constants for name vs unit validation error messages and used them in synchronous/asynchronous instrument initialization.
- Added regression tests asserting key spec constraints are reflected in the raised exception messages.
- Added a changelog fragment documenting the name error message clarification.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.py |
Split validation error messaging so name/unit failures raise different messages. |
opentelemetry-sdk/tests/metrics/test_instrument_error_message.py |
New regression tests to assert name/unit validation errors contain correct spec wording. |
.changelog/5283.changed |
Changelog entry describing the updated name validation error message. |
Comments suppressed due to low confidence (1)
.changelog/5283.changed:2
- This changelog fragment has an extra blank line; other fragments in
.changelog/*.changedare single-line entries. Please remove the trailing empty line so the fragment is exactly one line.
metrics: clarify instrument name validation error message to reflect spec constraints (start with a letter, allowed chars, max length 255)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@aabmass Thanks for the review! Here's comprehensive validation: Validation CodeThe validation happens in the API layer: _name_regex = re_compile(r"[a-zA-Z][-_./a-zA-Z0-9]{0,254}")
_unit_regex = re_compile(r"[\x00-\x7F]{0,63}")Spec ReferenceOpenTelemetry Metrics API Specification confirms: Instrument Name: Must start with letter, contain ASCII alphanumerics + _, ., -, /, max 255 chars Test ResultsUnit TestsLocal verification, name vs unit error-message outputsTest 1: Invalid name (starts with number) Test 2: Invalid unit (non-ASCII) Test 3: Unit exceeds 63 chars All tests passed. Distinguishes name vs unit validation failures |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| if __name__ == "__main__": | ||
| unittest.main() |
| _Synchronous("1invalid", scope, object()) | ||
| msg = str(cm.exception) | ||
| self.assertIn("start with a letter", msg) | ||
| self.assertIn("255", msg) |
| _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 {}" | ||
| ) |
Summary
Use two distinct validation error messages for metrics instrument creation so name and unit failures reflect their different spec constraints.
Changes
_NAME_ERROR_MESSAGEand_UNIT_ERROR_MESSAGEinopentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/instrument.pyopentelemetry-sdk/tests/metrics/test_instrument_error_message.pyTests
test_invalid_name_error_messageverifies invalid names mention “start with a letter” and max length255test_invalid_unit_error_messageverifies invalid units mention ASCII and max length63Notes
Exceptionto avoid changing behavior in this small fix; happy to change toValueErrorif maintainers prefer