Repository navigation
fix(sdk): exact-match the View unit selection criterion - #5816
RichardoMrMu wants to merge 5 commits into
Conversation
The Metrics SDK specification defines unit selection as an exact, case-sensitive comparison and gives no wildcard semantics for units. Matching instrument_unit with fnmatchcase therefore misread UCUM annotated units such as "[degF]" as character classes, so a View for that unit never matched. Compare the unit literally and cover the bracketed units and wildcard characters with regression tests. Assisted-by: Doubao
Assisted-by: Doubao
Pull request dashboard statusWaiting on maintainers · refreshed 2026-11-02 12:03 UTC Merge when ready. Status above doesn't look right?
|
knowprasad
left a comment
knowprasad
left a comment
There was a problem hiding this comment.
Nice catch on the fnmatchcase issue with UCUM brackets. Makes total sense since the metrics spec only specifies wildcard matching on instrument names anyway, so exact string comparison for units is much cleaner.
Pulled the branch and verified the new unit tests pass locally alongside the rest of the metrics suite.
LGTM!
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the change to exact, case-sensitive View unit matching.
No open issues 🎉
What changed in this PR
Fixes Metrics SDK View selection to match instrument units exactly and case-sensitively, as required by the specification. Instrument-name matching remains unchanged.
Changes:
- Replaces wildcard-based unit matching with literal equality.
- Adds regression tests for bracketed UCUM units and literal wildcard characters.
- Documents the fix in a changelog entry.
| File | Description |
|---|---|
opentelemetry-sdk/tests/metrics/test_view.py |
Adds regression coverage for exact unit matching. |
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/view.py |
Uses literal equality for unit selection. |
.changelog/5816.fixed |
Records the corrected View unit-matching behavior. |
Review effort: Balanced
| # across platforms. | ||
| if not fnmatchcase(instrument.unit, self._instrument_unit): | ||
| # The unit selection criterion is an exact, case-sensitive match: | ||
| # the Metrics SDK specification defines no wildcard matching for |
There was a problem hiding this comment.
There is no point in even referencing fnmatch here, we should shorten this to something like: "Unit comparison is performed in an exact case-sensitive manner".
github-project-automation
Bot
moved this from Approved PRs
to Reviewed PRs that need fixes
in Python PR digest
Address review feedback: drop the fnmatch reference and condense the four-line comment to a single sentence stating unit comparison is an exact, case-sensitive match. No behavior change. Assisted-by: Doubao
Description
A View selected by
instrument_unitmatched the unit throughfnmatchcase, copied from the instrument-name path. The Metrics SDK specification definesunitas an exact, case-sensitive criterion with no wildcard matching (only instrumentnameMAY match*/?). UCUM annotated units use square brackets - e.g.[degF],[degC],[in_i]- andfnmatchcaseread those as character classes, soView(instrument_unit="[degF]")never matched an instrument whose unit is[degF];*/?in a unit pattern also behaved as wildcards against the spec.Compare the unit literally in
View._match(case-sensitive, as required by UCUM). Instrument-name matching is unchanged.Type of change
Verification
test_instrument_unit_ucum_annotated_units_match_exactlyandtest_instrument_unit_wildcard_characters_are_literal; both fail on main ([degF]does not match;request_*matchesrequest_count) and pass with the fix.opentelemetry-sdk/tests/metrics: 339 passed, 11 skipped, 0 failed.ruff checkandruff format --checkclean on both files.Does this PR require a contrib change?