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

[config] Deprecate component.UnmarshalConfig #9750

Merged
merged 6 commits into from
May 29, 2024

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Mar 13, 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

@atoulme atoulme requested review from a team and jpkrohling March 13, 2024 01:30
Copy link

codecov bot commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.48%. Comparing base (edae2c7) to head (0bbd39d).
Report is 17 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9750      +/-   ##
==========================================
+ Coverage   91.98%   92.48%   +0.50%     
==========================================
  Files         361      387      +26     
  Lines       16965    18244    +1279     
==========================================
+ Hits        15605    16873    +1268     
- Misses       1020     1025       +5     
- Partials      340      346       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@atoulme
Copy link
Contributor Author

atoulme commented Mar 13, 2024

Contrib tests fix: open-telemetry/opentelemetry-collector-contrib#31727

@atoulme atoulme force-pushed the embedded_struct_unmarshaler branch from 311078f to 07fa33d Compare March 13, 2024 06:47
@mx-psi mx-psi requested a review from bogdandrutu March 13, 2024 14:00
cmd/mdatagen/loader.go Outdated Show resolved Hide resolved
cmd/mdatagen/metricdata.go Outdated Show resolved Hide resolved
component/config_test.go Outdated Show resolved Hide resolved
confmap/confmap.go Outdated Show resolved Hide resolved
confmap/confmap.go Outdated Show resolved Hide resolved
@atoulme atoulme force-pushed the embedded_struct_unmarshaler branch 2 times, most recently from ccbf4cc to f8f4fe6 Compare March 14, 2024 19:57
confmap/confmap.go Outdated Show resolved Hide resolved
dmitryax pushed a commit that referenced this pull request Mar 14, 2024
This is a split of #9750 that tries to work around mapstructure, which
wraps an error around a decoding error.

In the case when an error is returned from a top level construct, we get
a not so helpful message that says:
```
error decoding '': error running encode hook: marshaling error
```

With this change, the error is unwrapped, giving the following string
representation:
```
error running encode hook: marshaling error
```

Because #9750 enforces going through mapstructure, it would change
errors returned with this not-so-helpful preamble. Adding this removes
the problem.
dmitryax pushed a commit that referenced this pull request Mar 14, 2024
This change is required in preparation of #9750 

This removes the call to `component.UnmarshalConfig` in preparation of
its deprecation, and instead has the `Conf` object unmarshal itself into
the `Config` struct.
@atoulme atoulme force-pushed the embedded_struct_unmarshaler branch from 51e5f78 to dfa57f1 Compare March 14, 2024 21:35
dmitryax pushed a commit that referenced this pull request Mar 15, 2024
Reverts #9765

We need to revert those changes as contrib has issues with them in
isolation from #9750.
@mx-psi
Copy link
Member

mx-psi commented Mar 18, 2024

Can you add 'Fixes #7101' to the PR description?

confmap/confmap.go Outdated Show resolved Hide resolved
mx-psi pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Mar 27, 2024
**Description:** 
This is a companion PR to handle recursive state of unmarshalers with
open-telemetry/opentelemetry-collector#9750.
Changing this behavior will allow the confmap.Conf object to recognize
that it has already run the `Unmarshal` method on the struct, and run
the mapstructure decoding of fields.
dmitryax pushed a commit that referenced this pull request Apr 3, 2024
…squashing (#9861)

This is taking a small slice of #9750 to document the behavior of
confmap and make sure we can unmarshal embedded structs.
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@atoulme atoulme force-pushed the embedded_struct_unmarshaler branch from 4445cc4 to 05a0d08 Compare April 21, 2024 00:28
Copy link
Contributor

github-actions bot commented May 8, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label May 8, 2024
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this May 22, 2024
mx-psi added a commit that referenced this pull request May 23, 2024
…shalers. (#9862)

This is a slice of #9750 focusing on removing the top level condition on
unmarshaling structs.

---------

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@atoulme atoulme reopened this May 24, 2024
@atoulme atoulme force-pushed the embedded_struct_unmarshaler branch from 05a0d08 to 2d0837a Compare May 24, 2024 02:05
@atoulme atoulme force-pushed the embedded_struct_unmarshaler branch from 2d0837a to 1757e25 Compare May 24, 2024 02:14
@atoulme atoulme changed the title [config] Deprecate config.UnmarshalConfig [config] Deprecate component.UnmarshalConfig May 24, 2024
@atoulme atoulme force-pushed the embedded_struct_unmarshaler branch 2 times, most recently from bd90692 to 8e1360c Compare May 24, 2024 03:03
@github-actions github-actions bot removed the Stale label May 24, 2024
@atoulme atoulme force-pushed the embedded_struct_unmarshaler branch from 8e1360c to 616b7ec Compare May 24, 2024 05:12
.chloggen/embedded_struct_unmarshaler.yaml Outdated Show resolved Hide resolved
andrzej-stencel pushed a commit to andrzej-stencel/opentelemetry-collector that referenced this pull request May 27, 2024
…shalers. (open-telemetry#9862)

This is a slice of open-telemetry#9750 focusing on removing the top level condition on
unmarshaling structs.

---------

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@mx-psi mx-psi merged commit 9a21643 into open-telemetry:main May 29, 2024
49 checks passed
@github-actions github-actions bot added this to the next release milestone May 29, 2024
steves-canva pushed a commit to Canva/opentelemetry-collector that referenced this pull request Jun 14, 2024
…shalers. (open-telemetry#9862)

This is a slice of open-telemetry#9750 focusing on removing the top level condition on
unmarshaling structs.

---------

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
steves-canva pushed a commit to Canva/opentelemetry-collector that referenced this pull request 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove component.UnmarshalConfig Investigate alternative for Unmarshaler and Marshaler
5 participants