Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metric units should use UCUM case sensitive variant #3306

Merged
merged 3 commits into from
Mar 8, 2023

Conversation

jack-berg
Copy link
Member

Related to #705.

@jack-berg jack-berg requested review from a team March 8, 2023 16:47
@reyang
Copy link
Member

reyang commented Mar 8, 2023

Although not introduced by this PR, I just want to call out that we might run into trouble with the current spec:

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument-unit

image

image

@jack-berg
Copy link
Member Author

@reyang I don't understand - single quote is a valid ascii character, no?

@reyang
Copy link
Member

reyang commented Mar 8, 2023

@reyang I don't understand - single quote is a valid ascii character, no?

"single quote is a valid ascii character" - right. The problem I'm seeing - UCUM c/s doesn't say anything about the allowed characters, we don't know if we'll see non-ASCII chars.

@MrAlias
Copy link
Contributor

MrAlias commented Mar 8, 2023

Although not introduced by this PR, I just want to call out that we might run into trouble with the current spec:

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument-unit

image

image

Aren't these just two single quote characters (ASCII code 39)?

@MrAlias
Copy link
Contributor

MrAlias commented Mar 8, 2023

@reyang
Copy link
Member

reyang commented Mar 8, 2023

20230308_090755

Ah, I see, that removes the concern. Thanks!

@reyang reyang merged commit b674b49 into open-telemetry:main Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants