Refactor prometheusreceiver config to avoid some custom unmarshal work #29901

bogdandrutu · 2023-12-15T00:41:40Z

This is NOT a user config breaking change.

This PR improves the usability of our public APIs removing unnecessary placeholders public members. For API developers they only need to interact with the new wrapper types and don't worry about placeholders.

Also this PR simplifies the unmarshalling logic allowing to add more fields more confidently.

Aneurysm9

If this is a breaking change it needs significantly more description of its purpose and effect. What are we trying to do here? Why is it necessary and advisable? What impact does it have on users (both end users and developers of components that interact with this component)? There's no description on this PR nor is there an issue detailing what needs to change and why.

dashpole · 2023-12-15T17:07:43Z

It is an API breaking change because it changes a public type. It isn't a breaking change for users.

bogdandrutu · 2023-12-15T17:37:59Z

@Aneurysm9 tried to clarify what happens. It is a clear improvement in my opinion since we remove the hacky placeholders.

Aneurysm9

Still reviewing this but I'm on-call today and Monday and unlikely to be able to finish a review before Tuesday.

receiver/prometheusreceiver/config.go

Aneurysm9 · 2023-12-15T21:53:58Z

It is an API breaking change because it changes a public type. It isn't a breaking change for users.

API users are still users. Has this been communicated to @open-telemetry/operator-ta-maintainers? I know they're at least one group that is going to have an interest in this.

bogdandrutu · 2023-12-15T22:51:54Z

API users are still users.

As per my understanding in the changelog we use api and user to differentiate between API users and end-user configuration/behavior breaking changes. If you disagree with these usages of the 2 words we can change that.

Has this been communicated to @open-telemetry/operator-ta-maintainers? I know they're at least one group that is going to have an interest in this.

I don't see a need to do that, since https://github.com/search?q=repo%3Aopen-telemetry%2Fopentelemetry-operator%20prometheusreceiver&type=code doesn't show any usage of the public API.

bogdandrutu · 2023-12-29T23:49:18Z

@Aneurysm9 friendly ping

receiver/prometheusreceiver/config.go

bogdandrutu · 2024-01-09T20:20:19Z

@Aneurysm9 friendly ping

bogdandrutu · 2024-01-10T17:11:49Z

@Aneurysm9 friendly ping

jpkrohling

Apart from the failing unit tests due to the change in logic in one validation, this looks good.

Out of curiosity: are you doing this change so that you can build a more specialized Prometheus receiver? If so, this might benefit the Pure Storage receivers as well (cc @chrroberts-pure , @dgoscn)

receiver/prometheusreceiver/config.go

receiver/prometheusreceiver/factory.go

bogdandrutu · 2024-01-12T00:06:25Z

Out of curiosity: are you doing this change so that you can build a more specialized Prometheus receiver? If so, this might benefit the Pure Storage receivers as well (cc @chrroberts-pure , @dgoscn)

For the moment focus on cleaning how we do unmarshal/validation and understand patterns.

aknuds1

LGTM - left some nits.

receiver/prometheusreceiver/config.go

aknuds1 · 2024-01-12T11:36:25Z

receiver/prometheusreceiver/config.go

 		return nil
 	}
-	// We need custom unmarshaling because prometheus "config" subkey defines its own
-	// YAML unmarshaling routines so we need to do it explicitly.
+	cfgMap["url"] = "http://placeholder" // we have to set it as else the marshal will fail


[Nit]

Suggested change

cfgMap["url"] = "http://placeholder" // we have to set it as else the marshal will fail

cfgMap["url"] = "http://placeholder" // we have to set it as else marshalling will fail

I am not a native english speaker, so I will not fight with the linter:

Error: prometheusreceiver/config.go:148:68: `marshalling` is a misspelling of `marshaling` (misspell) cfgMap["url"] = "http://placeholder" // we have to set it as else marshalling will fail

receiver/prometheusreceiver/config.go

jpkrohling

LGTM

I still do not believe that there has been adequate explanation of the purpose or value of these changes or whether they are self-contained or the beginning of a stepwise refactoring to an unstated desired end state. That said, it is not my desire to be an impediment if the maintainers believe that this change must be made.

alolita · 2024-01-13T02:02:43Z

Points noted @Aneurysm9 Thanks!

Signed-off-by: Bogdan Drutu <[email protected]>

jpkrohling · 2024-01-17T09:25:53Z

After careful consideration, I am proceeding to merge this PR in my capacity as a @open-telemetry/collector-contrib-maintainer. I would like to outline the reasons for this decision:

The modifications presented in this PR, particularly in relation to the config placeholder, represent an improvement to our code base.
The approach taken in this refactoring adheres to best practices, notably in maintaining the integrity of existing tests. Adjustments have been made solely for the incorporation of new types. This gives me a high level of confidence that the changes are unlikely to introduce new bugs, and may help in identifying any previously untested scenarios.
Although the motivations and subsequent steps for this change were not entirely clear, the nature of the change is such that I would have endorsed it if it were proposed for a component under my code ownership. It is important to maintain consistent standards across our contributors, particularly when they are not strangers to the project/SIG. In this case, the author has met the same level of quality and commitment we expect from all contributors.

open-telemetry#29901) This is **NOT** a user config breaking change. This PR improves the usability of our public APIs removing unnecessary placeholders public members. For API developers they only need to interact with the new wrapper types and don't worry about placeholders. Also this PR simplifies the unmarshalling logic allowing to add more fields more confidently. Signed-off-by: Bogdan Drutu <[email protected]>

bogdandrutu requested review from Aneurysm9 and dashpole as code owners December 15, 2023 00:41

bogdandrutu requested a review from a team December 15, 2023 00:41

github-actions bot assigned djaglowski Dec 15, 2023

github-actions bot added the receiver/prometheus Prometheus receiver label Dec 15, 2023

bogdandrutu force-pushed the refactorprom branch 3 times, most recently from d55ec29 to b5d929d Compare December 15, 2023 05:08

bogdandrutu requested review from fatsheep9146 and jpkrohling as code owners December 15, 2023 05:08

github-actions bot added receiver/purefa receiver/purefb receiver/simpleprometheus labels Dec 15, 2023

bogdandrutu force-pushed the refactorprom branch 3 times, most recently from 77918ad to 3558d52 Compare December 15, 2023 05:30

bogdandrutu requested a review from a team December 15, 2023 05:30

github-actions bot added cmd/otelcontribcol otelcontribcol command exporter/prometheus testbed labels Dec 15, 2023

dashpole approved these changes Dec 15, 2023

View reviewed changes

Aneurysm9 suggested changes Dec 15, 2023

View reviewed changes

bogdandrutu requested a review from Aneurysm9 December 15, 2023 17:39

Aneurysm9 previously requested changes Dec 15, 2023

View reviewed changes

receiver/prometheusreceiver/config.go Outdated Show resolved Hide resolved

receiver/prometheusreceiver/config.go Outdated Show resolved Hide resolved

bogdandrutu force-pushed the refactorprom branch from 3558d52 to 06c5e01 Compare December 17, 2023 16:59

bogdandrutu requested a review from Aneurysm9 December 18, 2023 17:21

bogdandrutu requested a review from Aneurysm9 December 20, 2023 18:43

bogdandrutu force-pushed the refactorprom branch from 6a2ada0 to 8ddccca Compare December 29, 2023 23:54

Aneurysm9 reviewed Jan 8, 2024

View reviewed changes

receiver/prometheusreceiver/config.go Outdated Show resolved Hide resolved

Aneurysm9 mentioned this pull request Jan 8, 2024

Fix prometheusreceiver TA/scrape_config validation logic #30135

Closed

bogdandrutu requested a review from Aneurysm9 January 8, 2024 21:12

bogdandrutu force-pushed the refactorprom branch 2 times, most recently from 5e618ff to 8d77098 Compare January 10, 2024 17:32

jpkrohling reviewed Jan 11, 2024

View reviewed changes

receiver/prometheusreceiver/config.go Outdated Show resolved Hide resolved

receiver/prometheusreceiver/factory.go Show resolved Hide resolved

bogdandrutu force-pushed the refactorprom branch 3 times, most recently from 4aaf902 to 0e0b4b2 Compare January 11, 2024 18:11

aknuds1 approved these changes Jan 12, 2024

View reviewed changes

bogdandrutu force-pushed the refactorprom branch from 0e0b4b2 to d26bfed Compare January 12, 2024 17:22

jpkrohling approved these changes Jan 12, 2024

View reviewed changes

bogdandrutu force-pushed the refactorprom branch 2 times, most recently from 0e458de to ac09ec1 Compare January 13, 2024 01:08

Refactor prometheusreceiver config to avoid some custom unmarshal work

3d56cf0

Signed-off-by: Bogdan Drutu <[email protected]>

bogdandrutu force-pushed the refactorprom branch from ac09ec1 to 3d56cf0 Compare January 16, 2024 18:44

jpkrohling merged commit c10a264 into open-telemetry:main Jan 17, 2024
87 checks passed

github-actions bot added this to the next release milestone Jan 17, 2024

bogdandrutu deleted the refactorprom branch January 17, 2024 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor prometheusreceiver config to avoid some custom unmarshal work #29901

Refactor prometheusreceiver config to avoid some custom unmarshal work #29901

bogdandrutu commented Dec 15, 2023 •

edited

Loading

Aneurysm9 left a comment

dashpole commented Dec 15, 2023

bogdandrutu commented Dec 15, 2023

Aneurysm9 left a comment

Aneurysm9 commented Dec 15, 2023

bogdandrutu commented Dec 15, 2023

bogdandrutu commented Dec 29, 2023

bogdandrutu commented Jan 9, 2024

bogdandrutu commented Jan 10, 2024

jpkrohling left a comment

bogdandrutu commented Jan 12, 2024

aknuds1 left a comment

aknuds1 Jan 12, 2024

bogdandrutu Jan 12, 2024

bogdandrutu Jan 16, 2024

jpkrohling left a comment

alolita commented Jan 13, 2024

jpkrohling commented Jan 17, 2024

	cfgMap["url"] = "http://placeholder" // we have to set it as else the marshal will fail
	cfgMap["url"] = "http://placeholder" // we have to set it as else marshalling will fail

Refactor prometheusreceiver config to avoid some custom unmarshal work #29901

Refactor prometheusreceiver config to avoid some custom unmarshal work #29901

Conversation

bogdandrutu commented Dec 15, 2023 • edited Loading

Aneurysm9 left a comment

Choose a reason for hiding this comment

dashpole commented Dec 15, 2023

bogdandrutu commented Dec 15, 2023

Aneurysm9 left a comment

Choose a reason for hiding this comment

Aneurysm9 commented Dec 15, 2023

bogdandrutu commented Dec 15, 2023

bogdandrutu commented Dec 29, 2023

bogdandrutu commented Jan 9, 2024

bogdandrutu commented Jan 10, 2024

jpkrohling left a comment

Choose a reason for hiding this comment

bogdandrutu commented Jan 12, 2024

aknuds1 left a comment

Choose a reason for hiding this comment

aknuds1 Jan 12, 2024

Choose a reason for hiding this comment

bogdandrutu Jan 12, 2024

Choose a reason for hiding this comment

bogdandrutu Jan 16, 2024

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

alolita commented Jan 13, 2024

jpkrohling commented Jan 17, 2024

bogdandrutu commented Dec 15, 2023 •

edited

Loading