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

Clarify prometheus unit conversions #3066

Merged
merged 5 commits into from
Feb 1, 2023

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Jan 3, 2023

Addresses confusion in this comment open-telemetry/opentelemetry-java#4390 (comment) by more completely describing the translation steps required for translating between prometheus unit conventions and opentelemetry unit conventions.

This draws on https://prometheus.io/docs/practices/naming/#base-units and https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#units-and-base-units. It matches the current implementation of the conversion in the collector: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/translator/prometheus/normalize_name.go except that it maintains the current recommendation to convert to base units in Prometheus exporters.

cc @jack-berg @psx95 @jmacd @gouthamve @open-telemetry/wg-prometheus @bertysentry

@dashpole dashpole requested review from a team January 3, 2023 20:56
@dashpole dashpole changed the title clarify prometheus unit conversions Clarify prometheus unit conversions Jan 3, 2023
@bertysentry
Copy link
Contributor

Actually the description of the translation of Otel to Prometheus metrics is described here for the translator: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/translator/prometheus

Side note: when will we enable the pkg.translator.prometheus.NormalizeName feature gate by default?

Copy link
Member

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@fstab fstab left a comment

Choose a reason for hiding this comment

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

Hi 👋, I came across this PR and added a few comments. Thanks a lot for taking care of this.

Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

Looks good. Let's make sure we have Prometheus developers' approval before merging.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jan 28, 2023
@jmacd jmacd merged commit 69141c3 into open-telemetry:main Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants