[chore] Migrate to github.com/prometheus/otlptranslator#39827
Merged
songy23 merged 6 commits intoJul 18, 2025
Merged
Conversation
github-actions
Bot
added
receiver/prometheus
github-actions
Bot
requested review from
Aneurysm9,
aabmass,
braydonk,
dashpole,
gouthamve,
jsuereth,
krajorama,
mar4uk,
psx95,
rapphil and
ridwanmsharif
9 tasks
aknuds1
reviewed
May 3, 2025
Member
Author
It's not a requirement, it's an idea to avoid code duplication in the OTel collector. If we don't move it to otlptranslator, do you have any other suggestions? |
Contributor
Contributor
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Contributor
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Contributor
|
@ArthurSens Do you need help picking this up? |
ArthurSens
changed the title
Member
Author
|
urgh, |
ArthurSens
force-pushed
the
switch-translator-libraries-2
branch
from
20c88d5 to
3fcf6d1
Compare
ywwg
suggested changes
Jul 15, 2025
Contributor
ywwg
left a comment
ywwg
left a comment
There was a problem hiding this comment.
looking great! My main worry is the overhead of creating all the new metricNamer objects -- are there places where it can be built once for many metrics? And do we have benchmarks we can run to see if it's actually a problem?
Closed
1 task
ArthurSens
force-pushed
the
switch-translator-libraries-2
branch
from
5e4bb49 to
30b7445
Compare
ywwg
approved these changes
Jul 16, 2025
Contributor
ywwg
left a comment
ywwg
left a comment
There was a problem hiding this comment.
looks great! Just one small nit
| } | ||
|
|
||
| func newCollector(config *Config, logger *zap.Logger) *collector { | ||
| labelNamer := otlptranslator.LabelNamer{} |
Contributor
There was a problem hiding this comment.
looks like this one can also be inline in the return struct
Member
Author
There was a problem hiding this comment.
it can't because we're using it to clean up the namespace 😬
Contributor
|
@dashpole since you offered help earlier, would you please review and approve if the changes look ok? |
dashpole
approved these changes
Jul 17, 2025
Contributor
|
great work! |
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
ArthurSens
force-pushed
the
switch-translator-libraries-2
branch
from
30b7445 to
ddf5413
Compare
ArthurSens
added
ready to merge
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
ArthurSens
added
the
ready to merge
Dylan-M
pushed a commit
to Dylan-M/opentelemetry-collector-contrib
that referenced
this pull request
Aug 5, 2025
#### Description Bumps the Prometheus library to the commit(prometheus/prometheus@0502f2d) that updates otlptranslator to the same version we need in open-telemetry#39827 Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Dylan-M
pushed a commit
to Dylan-M/opentelemetry-collector-contrib
that referenced
this pull request
Aug 5, 2025
…ry#39827) #### Description This is an alternative for open-telemetry#39753, where we don't migrate the constants to the new library, but keep them closer to where they are used. I apologize for pinging so many people in this PR, but I tried to migrate all components at once just to see how big/challenging it is to do the whole migration. Pinging @aknuds1 and @ywwg as my fellow maintainer of prometheus/otlptranslators :) I see that we're still lacking a few things, like BuildCompliantUnit with and without UTF-8 allowed. We also need to think about what to do with the Prometheus receiver (pinging fellow code owner @krajorama), which uses the old library to do the reverse conversion. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Related to open-telemetry#39822 --------- Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is an alternative for #39753, where we don't migrate the constants to the new library, but keep them closer to where they are used.
I apologize for pinging so many people in this PR, but I tried to migrate all components at once just to see how big/challenging it is to do the whole migration.
Pinging @aknuds1 and @ywwg as my fellow maintainer of prometheus/otlptranslators :)
I see that we're still lacking a few things, like BuildCompliantUnit with and without UTF-8 allowed. We also need to think about what to do with the Prometheus receiver (pinging fellow code owner @krajorama), which uses the old library to do the reverse conversion.
Link to tracking issue
Related to #39822