-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update OM2.0 for UTF-8 metric names and labels #2746
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
Conversation
755a40d to
908e3a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, one nit comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good, I think need some examples with CI check on them
|
@ywwg I'd suggest to scan the "Design considerations" section, in particular https://prometheus.io/docs/specs/om/open_metrics_spec/#metric-naming-and-namespaces . I've done it as a follow-up to relaxed suffix rules, see #2751 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for extending!
I assume we will follow with same/similar wording in 2.0, right?
Can we outline e.g. in the description of this PR why we add UTF-8 capability to 1.1 instead of releasing this change with 2.0? Would be useful for bookkeeping.
|
we are going to go directly to 2.0 and skip 1.1 after all, so I'll adapt this change to 2.0 instead |
|
(I am not deleting 1.1 because we need to port some more changes from it into 2.0 and it's useful to keep it around for now) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to fix CI fail
|
removed diffs with 1.1 and cleaned things up a bit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I really think we should add some examples that are checked against the ABNF. It's ok if you want to do it in a next PR as well.
|
Yeah can we do that in a followup? I'll file an issue |
Part of prometheus/prometheus#16093 Signed-off-by: Owen Williams <[email protected]>
50a5023 to
ea1e18c
Compare
part of prometheus/prometheus#16093