[chore] prometheusreceiver: Refactor and remove dead code#44742
Merged
songy23 merged 4 commits intoDec 10, 2025
Conversation
aknuds1
changed the title
aknuds1
force-pushed
the
arve/remove-dead-unmarshaler
branch
from
b719e23 to
c4063df
Compare
aknuds1
force-pushed
the
arve/remove-dead-unmarshaler
branch
from
c4063df to
3e5d523
Compare
aknuds1
changed the title
Refactor PromHTTPSDConfig.Unmarshal and PromHTTPClientConfig.Unmarshal so they call a more specific function unmarshalConf, that knows it won't have to YAML marshal commonconfig.Secret instances. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
aknuds1
force-pushed
the
arve/remove-dead-unmarshaler
branch
from
3e5d523 to
87cf432
Compare
ArthurSens
approved these changes
Dec 9, 2025
ArthurSens
left a comment
ArthurSens
left a comment
Member
There was a problem hiding this comment.
I couldn't understand what part of the code you're removing was dead 🤔, but nonetheless LGTM because code looks simpler. Could you rebase your PR to solve conflicts?
…ead-unmarshaler Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
ArthurSens
approved these changes
Dec 10, 2025
ArthurSens
left a comment
ArthurSens
left a comment
Member
There was a problem hiding this comment.
I'll wait until Friday to add the Ready to Merge label, to give @dashpole and @krajorama the opportunity to review this as well.
dashpole
approved these changes
Dec 10, 2025
ArthurSens
added
the
ready to merge
Contributor
|
Thank you for your contribution @aknuds1! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help. |
6 tasks
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
4 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
Refactor
PromHTTPSDConfig.UnmarshalandPromHTTPClientConfig.Unmarshalso they call a more specific functionunmarshalConf, that knows it won't have to YAML marshalcommonconfig.Secretinstances. This allows me to remove the custom YAML marshalling ofcommonconfig.Secret, which is actually buggy as evidenced by #44602 fixing the same logic in another location. Good thing it isn't needed :)Link to tracking issue
Fixes
Testing
I'm adding
TestUnmarshalConf, with a set of sub-tests for relevant cases. Thetargetallocatorcoverage improves from 77.7% to 83.6% versus main.Additionally, I'm adding
TestLoadTargetAllocatorConfig/special_characters_in_password, which fully verifies that loading aconfmap.Conffrom a YAML file with special password characters and then unmarshalling intotargetallocator.Configworks. The old, and non-functioning, code was supposed to ensure the same.Documentation