Skip to content

naming: Update metric and label name restrictions and recommendations with the latest context #2626

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 24, 2025
Merged

naming: Update metric and label name restrictions and recommendations with the latest context #2626

merged 3 commits into from
Apr 24, 2025

Conversation

bwplotka
Copy link
Member

Notable changes

  • Cleaned up should/must/may to upper case for clarity (as per RFC style we do in specs).
  • Mentioned UTF-8 support.
  • Lifted limited character set from MUST to SHOULD with a clear about consequences and downsides.
  • Mentioned alternative notations.
  • Removed experimental mentions about native histograms, made it part of the main sample definition.
  • Added section on WHY Prometheus metric SHOULD contain type and unit suffix, even with our type and unit labels one day.

Motivation

@bwplotka
naming: Update metric and label name recommendations with the latest …
c095aa6 
…context.

### Notable changes

* Cleaned up should/must/may to upper case for clarity (as per RFC style we do in specs).
* Mentioned UTF-8 support.
* Lifted limited character set from MUST to SHOULD with a clear about consequences and downsides.
* Mentioned alternative notations.
* Removed experimental mentions about native histograms, made it part of the main sample definition.
* Added section on WHY Prometheus metric SHOULD contain type and unit suffix, even with our type and unit labels one day.

### Motivation

* [OpenMetrics WG 2.0 Action item](https://docs.google.com/document/d/1FCD-38Xz1-9b3ExgHOeDTQUKUatzgj5KbCND9t-abZY/edit?tab=t.lvx6fags1fga#heading=h.3xjg8jcpctk4
) and blocker to relax MUST to SHOULD type and unit suffix in OpenMetrics 2.0.
* Similarly, to have something clear to reference to highlight huge downsides of skipping suffixes for OTLP (prometheus/prometheus#16441)
* To help with explaining this problem to Prometheus users in general (on recent KubeCons we had to explain this hundrends of times).


Signed-off-by: bwplotka <[email protected]>
@bwplotka bwplotka mentioned this pull request Apr 17, 2025
Merged
bboreham
bboreham reviewed Apr 17, 2025
View reviewed changes
Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally fine. Some thoughts below.

krajorama
krajorama reviewed Apr 17, 2025
View reviewed changes
beorn7
beorn7 requested changes Apr 17, 2025
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this.

ArthurSens
ArthurSens reviewed Apr 17, 2025
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides all that has already been said, found one other thing

bwplotka and others added 2 commits April 23, 2025 09:25
@bwplotka @bboreham
@beorn7 @krajorama
Apply suggestions from code review
61a913b 
Co-authored-by: Bryan Boreham <[email protected]>
Co-authored-by: Björn Rabenstein <[email protected]>
Co-authored-by: George Krajcsovits <[email protected]>
Signed-off-by: Bartlomiej Plotka <[email protected]>
@bwplotka
Address comments.
9219490 
Signed-off-by: bwplotka <[email protected]>
@bwplotka
Copy link
Member Author

Thank you so much for all the great feedback @bboreham @beorn7 @ArthurSens @krajorama

Should be good for "final" look! 💪🏽

ArthurSens
ArthurSens approved these changes Apr 23, 2025
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@bwplotka bwplotka merged commit 9b0d0c5 into main Apr 24, 2025
5 checks passed
@bwplotka bwplotka deleted the naming branch April 24, 2025 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beorn7 beorn7 beorn7 approved these changes

@bboreham bboreham bboreham approved these changes

@ArthurSens ArthurSens ArthurSens approved these changes

@juliusv juliusv Awaiting requested review from juliusv

@RichiH RichiH Awaiting requested review from RichiH

@ywwg ywwg Awaiting requested review from ywwg

@krajorama krajorama Awaiting requested review from krajorama

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bwplotka @beorn7 @bboreham @krajorama @ArthurSens