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

Update prometheus metric exporter to comply with data model specification #2938

Closed
srikanthccv opened this issue Sep 20, 2022 · 7 comments · Fixed by #3924
Closed

Update prometheus metric exporter to comply with data model specification #2938

srikanthccv opened this issue Sep 20, 2022 · 7 comments · Fixed by #3924

Comments

@srikanthccv
Copy link
Member

The exporter should perform this such as

  1. The Name of an OTLP metric MUST be added as the OpenMetrics MetricFamily Name, with unit and type suffixes added as described below. The metric name is required to match the regex: a-zA-Z_:*. Invalid characters in the metric name MUST be replaced with the _ character. Multiple consecutive _ characters MUST be replaced with a single _ character.
    • Additionally, the unit MUST be added as a suffix to the metric name, and SHOULD be converted to base units recommended by OpenMetrics when possible. The unit suffix comes before any type-specific suffixes.
  2. The data point type of an OTLP metric MUST be added as OpenMetrics TYPE metadata.

Link to spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/data-model.md#otlp-metric-points-to-prometheus

@srikanthccv srikanthccv added bug Something isn't working metrics exporters prometheus and removed bug Something isn't working labels Sep 20, 2022
@dashpole
Copy link

dashpole commented Dec 2, 2022

New spec link: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#otlp-metric-points-to-prometheus

Looking through the current implementation, it looks like the name is already sanitized. Unit suffixes are already added by the upstream library. TYPE metadata is already added by upstream as well.

Remaining items (from the comment above):

  • repeated _ are not replaced with a single _.
  • I would hold off on converting to base units (e.g. milliseconds to seconds) for now, but we should convert from abbreviations to full words when possible (e.g. ms to milliseconds).

@nstawski
Copy link
Contributor

I would like to take this issue.

@XuanWang-Amos
Copy link

Hi, is there any update on the progress? We're running into some issue while exporting metrics to prometheus because they don't allow {} in unit, based on current specification, unit within brackets should be dropped.

@aabmass aabmass assigned aabmass and unassigned nstawski May 20, 2024
@aabmass
Copy link
Member

aabmass commented May 20, 2024

@dashpole since this is a breaking change to metric names, I'd like to minimize the disruption by fixing as much as possible at once. I see the spec also has

But I don't see it in the Go impl. Should I be implementing this case as well?

@dashpole
Copy link

You can skip it if it isn't in other SDK implementations.

@dashpole
Copy link

@aabmass
Copy link
Member

aabmass commented May 22, 2024

I implemented the "per" conversion in this PR as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants