Skip to content

fix(opentelemetry-exporter-prometheus)!: apply ratio suffix to gauges when necessary#6613

Closed
cjihrig wants to merge 2 commits intoopen-telemetry:mainfrom
cjihrig:prom-gauge
Closed

fix(opentelemetry-exporter-prometheus)!: apply ratio suffix to gauges when necessary#6613
cjihrig wants to merge 2 commits intoopen-telemetry:mainfrom
cjihrig:prom-gauge

Conversation

@cjihrig
Copy link
Copy Markdown
Contributor

@cjihrig cjihrig commented Apr 21, 2026

Which problem is this PR solving?

When converting OTLP metrics to Prometheus, _ratio should be appended to gauges whose unit is "1".

Here is the corresponding code in the Golang library (it is a Prometheus library used by the OTLP library). Here is the OTLP Golang unit test and corresponding test output snapshot.

There are a fairly significant amount of other similar changes missing from the JavaScript library that I'd like to work on, but I wanted to start with a small change to see how it would be received. However, because they can change the names of the output metrics, they would be breaking changes.

Fixes # N/A but found when looking into #6605

Short description of the changes

This commit appends the _ratio suffix to gauges as necessary.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • New and existing unit tests

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@cjihrig cjihrig requested a review from a team as a code owner April 21, 2026 01:49
Comment thread experimental/CHANGELOG.md Outdated

// Prometheus requires that metrics of the Counter kind have "_total" suffix
if (
!name.endsWith('_total') &&
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As a drive-by change, I switched the order of these checks to save the most expensive one for last.

@dyladan
Copy link
Copy Markdown
Member

dyladan commented Apr 21, 2026

Given that this is a breaking change would it be worth it to put this behind a configuration for some period of time, allowing users to retain backwards compatibility for some time if needed? This is an experimental component so breaking changes technically allowed, but it might be good to be a bit cautious here.

@cjihrig
Copy link
Copy Markdown
Contributor Author

cjihrig commented Apr 21, 2026

Given that this is a breaking change would it be worth it to put this behind a configuration for some period of time, allowing users to retain backwards compatibility for some time if needed?

I'll defer to you and the other maintainers on that. It's probably a good idea because I'd like to look into some of the other discrepancies such as replacing / with _per_, expanding abbreviations for units, etc. Any suggestions on the config option name? experimentalMetricNames?

@cjihrig
Copy link
Copy Markdown
Contributor Author

cjihrig commented Apr 24, 2026

Any suggestions on the config option name? experimentalMetricNames?

Ping @dyladan

@dyladan
Copy link
Copy Markdown
Member

dyladan commented Apr 24, 2026

I would not encode experimental into the name of the config because it would be weird when it goes to default on. Also the spec is not experimental I believe. I'd give it an obvious name like noDuplicateRatioSuffix.

I'd like to look into some of the other discrepancies such as replacing / with per, expanding abbreviations for units, etc

I'd give these configs with obvious names too. Documenting them together should be sufficient to indicate they are meant to all be enabled together. It also gives people the ability to control when each breaking change occurs.


We're going to be releasing a 3.x soon anyway so it will be a good opportunity to change the settings to default on.

@ArthurSens
Copy link
Copy Markdown
Member

ArthurSens commented Apr 26, 2026

Hey 👋

This conversion was recently removed from the spec 😬
open-telemetry/opentelemetry-specification#4966


Also the spec is not experimental I believe.

Right now it is experimental, the Prometheus SIG is working towards stabilizing it though :)

@cjihrig cjihrig closed this Apr 26, 2026
@cjihrig cjihrig deleted the prom-gauge branch April 26, 2026 22:18
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.

3 participants