-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
prometheusexporter: Automatic normalization of metrics from Otel to Prometheus naming convention #10028
Conversation
This looks reasonable to me, though I'm still not clear on whether What happens if metric names already contain a suffix or go through multiple round trips? Will the suffix be omitted if it already exists? |
@Aneurysm9 Thank you for the comment!
Good point about round-trip conversion: units must not be appended when already present in the metric name (this goes for units, Also, the issue #8950 mentions the need for the Prometheus receiver to perform the reverse "conversion", like in the example below:
This should be covered in a separate PR. |
There was a problem hiding this 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...
readme LGTM |
aff427b
to
43303f7
Compare
@Aneurysm9 @dashpole @pmm-sumo Code pushed, according to our discussion. Next step: update prometheusremotewriteexporter the same way. |
@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). |
@Aneurysm9 @pmm-sumo Please review! 😊 |
5703356
to
ffdbb11
Compare
Code updated following suggestions, and rebased to solve conflict. |
@dashpole @pmm-sumo @Aneurysm9 If you got some time, this PR needs approvals 😉 |
@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! |
7baad4d
to
3839a11
Compare
Rebased on main to solve recently introduced conflict in CHANGELOG.md |
6b1bb37
to
e38cef2
Compare
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 😊 I will look into the Prometheus compliance tests that are now failing. |
@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! |
I am sorry to insist, but this PR fixes an important issue in the Prometheus exporters. How can I help to get this merged? |
52e4424
to
06f14a8
Compare
I'm begging here: I need a review of this PR, as I can't keep resolving conflicts with upstream. |
…metrics to follow Prometheus naming convention * Updated **prometheusremotewriteexporter** to normalize metric names * Moved common code to new module /pkg/translator/prometheus * Added documentation
5daffa8
to
d4757be
Compare
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? |
@bertysentry, CI failed on an unrelated flaky test (#11451). I've restarted the workflow. |
Daily burden of 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. |
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. |
@djaglowski Awesome news! Weirdly, I can't see the option to allow maintainers to push to the branch of my PR: 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! |
Thank you @djaglowski for your help, much appreciated! |
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 |
I'm happy to be a codeowner |
Happy to! |
Thank you both! I'll make the PR |
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 normalizationfunc BuildPromCompliantName(metric pmetric.Metric, namespace string) string
for metric names normalizationAll normalization operations are detailed in
pkg/translator/prometheus/README.md
.Link to tracking Issue: #8950
Testing: See unit tests in the
pkg/translator/prometheus
moduleDocumentation: Updated README.md in prometheusexporter and prometheusremotewriteexporter