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

prometheusexporter: Automatic normalization of metrics from Otel to Prometheus naming convention #10028

Conversation

bertysentry
Copy link
Contributor

@bertysentry bertysentry commented May 15, 2022

Description: In prometheusexporter and prometheusremotewriteexporter, automatic normalization of metrics from OpenTelemetry semantic convention to Prometheus naming convention

The renaming is in alpha stage and thus disabled by default (behind a feature gate). Enable with --feature-gates=pkg.translator.prometheus.NormalizeName option.

Shared code between prometheusexporter and prometheusremotewriteexporter has been moved to a new module: pkg/translator/prometheus. This module provides 2 main functions:

  • func NormalizeLabel(label string) string for labels normalization
  • func BuildPromCompliantName(metric pmetric.Metric, namespace string) string for metric names normalization

All normalization operations are detailed in pkg/translator/prometheus/README.md.

Note
It is recommended to import pkg/translator/prometheus as prometheustranslator to avoid conflict with the "real" Prometheus module, from Prometheus.

Link to tracking Issue: #8950

Testing: See unit tests in the pkg/translator/prometheus module

Documentation: Updated README.md in prometheusexporter and prometheusremotewriteexporter

@Aneurysm9
Copy link
Member

This looks reasonable to me, though I'm still not clear on whether _total should be appended to counter names or not. I think some prometheus client libraries do that and some don't.

What happens if metric names already contain a suffix or go through multiple round trips? Will the suffix be omitted if it already exists?

@bertysentry
Copy link
Contributor Author

@Aneurysm9 Thank you for the comment!

_total must be appended to Counter metrics as per Prometheus naming standard.

Good point about round-trip conversion: units must not be appended when already present in the metric name (this goes for units, _total and _ratio suffixes).

Also, the issue #8950 mentions the need for the Prometheus receiver to perform the reverse "conversion", like in the example below:

system_io_bytes_total ==> system.io Counter with Units By

This should be covered in a separate PR.

Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

Whoops, forgot to click send...

exporter/prometheusexporter/README.md Outdated Show resolved Hide resolved
exporter/prometheusexporter/README.md Outdated Show resolved Hide resolved
exporter/prometheusexporter/README.md Outdated Show resolved Hide resolved
exporter/prometheusexporter/README.md Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor

readme LGTM

@bertysentry bertysentry changed the title Add option for automatic renaming of metrics from Otel to Prometheus naming convention Automatic renaming of metrics from Otel to Prometheus naming convention May 25, 2022
@bertysentry bertysentry changed the title Automatic renaming of metrics from Otel to Prometheus naming convention prometheusexporter: Automatic renaming of metrics from Otel to Prometheus naming convention May 25, 2022
@bertysentry bertysentry force-pushed the feature/issue-8950-handle-units-prometheus branch from aff427b to 43303f7 Compare May 26, 2022 15:54
@bertysentry
Copy link
Contributor Author

@Aneurysm9 @dashpole @pmm-sumo Code pushed, according to our discussion.

Next step: update prometheusremotewriteexporter the same way.

exporter/prometheusexporter/metric_rename.go Outdated Show resolved Hide resolved
exporter/prometheusexporter/metric_rename.go Outdated Show resolved Hide resolved
exporter/prometheusexporter/metric_rename.go Outdated Show resolved Hide resolved
exporter/prometheusexporter/metric_rename.go Outdated Show resolved Hide resolved
exporter/prometheusexporter/metric_rename.go Outdated Show resolved Hide resolved
exporter/prometheusexporter/metric_rename.go Outdated Show resolved Hide resolved
exporter/prometheusexporter/metric_rename.go Outdated Show resolved Hide resolved
exporter/prometheusexporter/metric_rename.go Outdated Show resolved Hide resolved
@bertysentry
Copy link
Contributor Author

@dashpole Complete rewrite to properly handle all metrics (and their units) in current Otel receivers. Added many unit tests to cover these cases.

Designed to be fast and robust, while covering most reasonable cases (and 100% of current Otel receivers).

@bertysentry
Copy link
Contributor Author

@Aneurysm9 @pmm-sumo Please review! 😊

exporter/prometheusexporter/README.md Outdated Show resolved Hide resolved
exporter/prometheusexporter/metric_rename.go Outdated Show resolved Hide resolved
exporter/prometheusexporter/metric_rename.go Outdated Show resolved Hide resolved
@bertysentry bertysentry force-pushed the feature/issue-8950-handle-units-prometheus branch from 5703356 to ffdbb11 Compare June 2, 2022 18:42
@bertysentry
Copy link
Contributor Author

Code updated following suggestions, and rebased to solve conflict.

@bertysentry
Copy link
Contributor Author

@dashpole @pmm-sumo @Aneurysm9 If you got some time, this PR needs approvals 😉

@bertysentry bertysentry requested a review from pmm-sumo June 6, 2022 22:42
@bertysentry
Copy link
Contributor Author

@Aneurysm9 @pmm-sumo I need this PR to be merged so I can start working on prometheusremotewriteexporter to implement the same mechanism. Is there anyone else you would like to review these changes? Thank you!

@bertysentry bertysentry force-pushed the feature/issue-8950-handle-units-prometheus branch from 7baad4d to 3839a11 Compare June 8, 2022 18:09
@bertysentry
Copy link
Contributor Author

Rebased on main to solve recently introduced conflict in CHANGELOG.md

@bertysentry bertysentry force-pushed the feature/issue-8950-handle-units-prometheus branch from 6b1bb37 to e38cef2 Compare June 13, 2022 11:24
@bertysentry
Copy link
Contributor Author

Restructured the code so that common code for normalizing metric names and labels is now in module /pkg/translator/prometheus. Both prometheusexporter and prometheusremotewriteexporter now leverage these functions, so the behavior is consistent.

@dashpole Your review and comments are appreciated 😊
@Aneurysm9 and @pmm-sumo This PR changes a few things in the Prometheus exporters. Your comments are welcome.

I will look into the Prometheus compliance tests that are now failing.

@bertysentry
Copy link
Contributor Author

@dmitryax @alolita @Aneurysm9 This metric normalization is required as per OpenTelemetry's official specification. Please review (and merge if okay). I'm concerned these changes could cause conflicts with other PRs that update the Prometheus-specific code.

If review is not possible at this time (which I totally understand), could you please redirect me to other potential reviewers? Thank you!

@bertysentry
Copy link
Contributor Author

I am sorry to insist, but this PR fixes an important issue in the Prometheus exporters. How can I help to get this merged?

@bertysentry bertysentry force-pushed the feature/issue-8950-handle-units-prometheus branch from 52e4424 to 06f14a8 Compare June 22, 2022 10:44
@bertysentry
Copy link
Contributor Author

I'm begging here: I need a review of this PR, as I can't keep resolving conflicts with upstream.
Please remember that changes in this PR are behind a feature gate, so this PR is not going to cause havoc on the project, but it refactors some duplicate code between the exporters for Prometheus (hence the many changes in this PR).
Please @jpkrohling @Aneurysm9 your help is much appreciated. Thanks!

…metrics to follow Prometheus naming convention

* Updated **prometheusremotewriteexporter** to normalize metric names
* Moved common code to new module /pkg/translator/prometheus
* Added documentation
@bertysentry bertysentry force-pushed the feature/issue-8950-handle-units-prometheus branch from 5daffa8 to d4757be Compare June 22, 2022 17:30
@bertysentry
Copy link
Contributor Author

Also, many tests are now failing after rebasing on main. Failures are completely unrelated to the changes in this PR. What is happening? Is there anyone who can help?

@djaglowski
Copy link
Member

@bertysentry, CI failed on an unrelated flaky test (#11451). I've restarted the workflow.

@bertysentry
Copy link
Contributor Author

Daily burden of go.mod conflict resolution...
image

Can anyone give me a hint on how to get these changes approved and merged? It's critical to change the behavior of the Prometheus exporters as soon as possible so they follow Otel's specifications and Prometheus' conventions. The longer we wait, the worse it gets to end users.
Feel free to contact me on CNCF's Slack if you want to discuss the changes. @Aneurysm9 @pmm-sumo @jpkrohling
Thank you!

@djaglowski
Copy link
Member

Hi @bertysentry, I tried to resolve the conflicts and push but you have not allowed maintainers to push to your branch. If you want to click the box (bottom of the righthand panel), I'll push. Then hopefully we can get this merged today.

@bertysentry
Copy link
Contributor Author

@djaglowski Awesome news! Weirdly, I can't see the option to allow maintainers to push to the branch of my PR:

image

I gave you direct access to my forked repo, so you should be able to push (you should have received an invite). Thank you again for your help!

@djaglowski djaglowski merged commit b828db4 into open-telemetry:main Jun 23, 2022
@bertysentry bertysentry deleted the feature/issue-8950-handle-units-prometheus branch June 23, 2022 16:34
@bertysentry
Copy link
Contributor Author

Thank you @djaglowski for your help, much appreciated!

@djaglowski
Copy link
Member

Glad to help @bertysentry.

I should have noticed this before, but since this PR introduced a new module, we really ought to have a code owner for it. Would you, @bertysentry and/or @dashpole, be willing to be the code owner for pkg/translator/prometheus?

@dashpole
Copy link
Contributor

I'm happy to be a codeowner

@bertysentry
Copy link
Contributor Author

Happy to!

@djaglowski
Copy link
Member

Thank you both! I'll make the PR

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.

6 participants