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

Remove component.UnmarshalConfig #7102

Closed
bogdandrutu opened this issue Feb 3, 2023 · 4 comments · Fixed by #9750 or #10340
Closed

Remove component.UnmarshalConfig #7102

bogdandrutu opened this issue Feb 3, 2023 · 4 comments · Fixed by #9750 or #10340
Assignees
Labels
release:required-for-ga Must be resolved before GA release

Comments

@bogdandrutu
Copy link
Member

There are mostly historical reasons why this exists, but we should clean this. Here are few options:

@bogdandrutu
Copy link
Member Author

@gbbr when I say "remove" it means doing the correct "deprecation" process that we have in place for this repo.

@gbbr
Copy link
Member

gbbr commented Feb 8, 2023

I have checked all (component.Config).Unmarshal implementations in this repository and the contrib distro and they all call (confmap.Conf).Unmarshal without exception. This is an obvious necessity, so for that reason I'm proposing to move the (confmap.Conf).Unmarshal call out of there, so that the collector does this automatically. There is no reason to expect all users to do this. This will not only solve the cyclic issue but also makes more sense as de-duplication.

Lastly, why is (component.Config) even expected to implement Unmarshaler? It is not really unmarshalling anything (within the new context). It is simply validating config. In this scenario it should either be called PostUnmarshal or somehow merged with Validate.

What do you think about going down this path?

@atoulme atoulme added the release:required-for-ga Must be resolved before GA release label Dec 19, 2023
bogdandrutu pushed a commit that referenced this issue Dec 22, 2023
…g configuration (#9154)

**Description:**
Make the option `WithErrorUnused` enabled by default when unmarshaling
configuration
The option `WithErrorUnused` is now enabled by default, and a new option
`WithIgnoreUnused` is introduced to ignore
errors about unused fields.

**Link to tracking Issue:**
This relates to #7102 to some extent.

**Testing:**
N/A

**Documentation:**
N/A
@rnishtala-sumo
Copy link
Contributor

rnishtala-sumo commented Jan 12, 2024

@atoulme this change in v0.92.0 seems to have broken one of our tests and the UnMarshalOption WithIgnoreUnused doesn't seem to ignore the extra keys in the source config.

https://github.com/SumoLogic/sumologic-otel-collector/blob/ot-upgrade/pkg/receiver/jobreceiver/config_test.go#L94

Get the following error

Received unexpected error:
1 error(s) decoding:
            	            	
* error decoding 'output': 1 error(s) decoding:
            	            	
* '' has invalid keys: log_entries

Is there any other change needed to not error on extra keys? This used to work for v0.91.0

@atoulme
Copy link
Contributor

atoulme commented Jan 12, 2024

You use a nested struct which needs to change the way it unmarshals to set WithIgnoreUnused.
https://github.com/SumoLogic/sumologic-otel-collector/blob/ot-upgrade/pkg/receiver/jobreceiver/output/config.go#L55C44-L55C44

You should not change your test for this.

@atoulme atoulme self-assigned this Mar 17, 2024
mx-psi pushed a commit that referenced this issue May 29, 2024
**Description:**
This PR removes the top level if/else in `component.UnmarshalConfig`,
handling recursive state in the confmap.Conf object instead.
This PR deprecates `component.UnmarshalConfig` in favor of calling
directly `Unmarshal` on the confmap.Conf object.

**Link to tracking Issue:**
Fixes #7102
Fixes #7101

---------

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
codeboten added a commit that referenced this issue Jun 6, 2024
#### Description
Remove deprecated `component.UnmarshalConfig`

#### Link to tracking issue
Fixes #7102

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
steves-canva pushed a commit to Canva/opentelemetry-collector that referenced this issue Jun 14, 2024
**Description:**
This PR removes the top level if/else in `component.UnmarshalConfig`,
handling recursive state in the confmap.Conf object instead.
This PR deprecates `component.UnmarshalConfig` in favor of calling
directly `Unmarshal` on the confmap.Conf object.

**Link to tracking Issue:**
Fixes open-telemetry#7102
Fixes open-telemetry#7101

---------

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment