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.

Sign up for GitHub

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

Jump to bottom

Investigate alternative for Unmarshaler and Marshaler #7101

Closed
bogdandrutu opened this issue Feb 3, 2023 · 1 comment · Fixed by #9750
Closed

Investigate alternative for Unmarshaler and Marshaler #7101

bogdandrutu opened this issue Feb 3, 2023 · 1 comment · Fixed by #9750
Labels
release:required-for-ga Must be resolved before GA release
Milestone
go.opentelemetry....

Comments

@bogdandrutu
Copy link
Member

bogdandrutu commented Feb 3, 2023 •
edited
Loading

The Unmarshaler because of the Unmarshal definition, has an issue when recursively unmarshal other structs that implement the Unmarshaler, see #6029 where the "given" destination to Conf.Unmarshal cannot be checked against the Unmarshaler implementation to avoid a deadlock. The Marshaler seem to not have (I did not carefully check) the same problem, but this needs to be confirmed.

Some possible solutions:

  • See yaml.v2 interfaces, they seem to work fine. We may want to switch to that.
  • Ignore this issue, document properly in the Conf.Unmarshal the caveat.
@bogdandrutu bogdandrutu mentioned this issue Feb 3, 2023
Closed
@atoulme atoulme added the release:required-for-ga Must be resolved before GA release label Dec 19, 2023
@mx-psi mx-psi mentioned this issue Mar 18, 2024
Merged
@mx-psi
Copy link
Member

mx-psi commented Mar 18, 2024

#9750 would remove this caveat from Conf.Unmarshal, solving this.

@mx-psi mx-psi moved this to In Progress in Collector: v1 Apr 18, 2024
@mx-psi mx-psi moved this from In Progress to Waiting for reviews in Collector: v1 Apr 18, 2024
@mx-psi mx-psi closed this as completed in 9a21643 May 29, 2024
@github-project-automation github-project-automation bot moved this from Waiting for reviews to Done in Collector: v1 May 29, 2024
steves-canva pushed a commit to Canva/opentelemetry-collector that referenced this issue Jun 14, 2024
@atoulme @evan-bradley @steves-canva
[config] Deprecate component.UnmarshalConfig (open-telemetry#9750)
779b625 
**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
Assignees
No one assigned
Labels
release:required-for-ga Must be resolved before GA release
Projects
Collector: v1
Archived in project
Milestone
go.opentelemetry.io/collector/confmap...
Development

Successfully merging a pull request may close this issue.

[config] Deprecate component.UnmarshalConfig atoulme/opentelemetry-collector
4 participants
@atoulme @bogdandrutu @mx-psi @gbbr