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

Add unit suffixes to prometheus metric names #3352

Merged
merged 3 commits into from
Oct 19, 2022

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Oct 18, 2022

Fixes #3288

This adds a new option, WithoutUnits() that disables this behavior for all metrics, and maintains the status quo.

I've opted to only rename units we explicitly support in our units package, rather than a more complete set like the collector does. I've also opted not to try and convert milliseconds to seconds, as the conversion (dividing by 1000) won't be straightforward with exponential histograms.

Open Question: Is 1 only applicable to ratios and fractions, or does it apply to any unitless metric? I was told that it only applies to ratios and fractions in the collector version of this. The spec says:

Instruments for utilization metrics (that measure the fraction out of a total) are dimensionless and SHOULD use the default unit 1 (the unity).
Instruments that measure an integer count of something SHOULD only use annotations with curly braces to give additional meaning without the leading default unit (1). For example, use {packets}, {errors}, {faults}, etc.

But I can't tell if there are metrics which don't fall into either category and can use 1 without being a ratio.

@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Merging #3352 (a7c8fee) into main (1d9d4b2) will decrease coverage by 0.0%.
The diff coverage is 80.7%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3352     +/-   ##
=======================================
- Coverage   77.9%   77.9%   -0.1%     
=======================================
  Files        164     164             
  Lines      11348   11363     +15     
=======================================
+ Hits        8850    8861     +11     
- Misses      2301    2305      +4     
  Partials     197     197             
Impacted Files Coverage Δ
exporters/prometheus/exporter.go 84.0% <76.1%> (+0.7%) ⬆️
exporters/prometheus/config.go 100.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go 81.1% <0.0%> (-0.9%) ⬇️
exporters/jaeger/jaeger.go 90.3% <0.0%> (-0.9%) ⬇️

CHANGELOG.md Outdated Show resolved Hide resolved
exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
exporters/prometheus/config.go Show resolved Hide resolved
@dashpole dashpole force-pushed the unit_in_name branch 2 times, most recently from ba437f1 to 68a17ab Compare October 19, 2022 18:24
dashpole and others added 3 commits October 19, 2022 18:29
CHANGELOG.md Show resolved Hide resolved
@MrAlias MrAlias added this to the Metric v0.33.0 milestone Oct 19, 2022
@Aneurysm9 Aneurysm9 merged commit 510910e into open-telemetry:main Oct 19, 2022
@dashpole dashpole deleted the unit_in_name branch October 19, 2022 19:28
@MrAlias MrAlias mentioned this pull request Oct 19, 2022
@ttys3
Copy link

ttys3 commented May 24, 2023

using _ratio is really bad if you use an promethus exporter

requests_total will become: requests_ratio_total Omz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:exporter:prometheus Related to the Prometheus exporter package
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Prometheus exporter metric names should include unit suffixes.
5 participants