[config] Deprecate config.UnmarshalConfig #9750

atoulme · 2024-03-13T01:30:14Z

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

codecov · 2024-03-13T01:39:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.02%. Comparing base (d1e631b) to head (dfa57f1).

❗ Current head dfa57f1 differs from pull request most recent head 05a0d08. Consider uploading reports for the commit 05a0d08 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9750      +/-   ##
==========================================
- Coverage   91.67%   91.02%   -0.65%     
==========================================
  Files         360      353       -7     
  Lines       16642    18633    +1991     
==========================================
+ Hits        15256    16961    +1705     
- Misses       1052     1347     +295     
+ Partials      334      325       -9

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

atoulme · 2024-03-13T01:59:17Z

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

cmd/mdatagen/loader.go

cmd/mdatagen/metricdata.go

component/config_test.go

confmap/confmap.go

mx-psi · 2024-03-14T14:56:57Z

confmap/confmap.go

@@ -37,7 +40,8 @@ func NewFromStringMap(data map[string]any) *Conf {
 // Conf represents the raw configuration map for the OpenTelemetry Collector.
 // The confmap.Conf can be unmarshalled into the Collector's config using the "service" package.
 type Conf struct {
-	k *koanf.Koanf
+	k    *koanf.Koanf
+	self any


Before I think we could call the same Conf to unmarshal it on two different structs, but now it can only be used once, right? Is there any way we can restore that property? If not, is there a way where we can error out if we have already unmarshaled?

This doesn't break your ability to use the same Conf to unmarshal on two different structs. It stops recursive calls as part of unmarshaling from calling Unmarshal on the same object twice.

confmap/confmap.go

cmd/mdatagen/templates/component_test.go.tmpl

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.

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.

Reverts #9765 We need to revert those changes as contrib has issues with them in isolation from #9750.

mx-psi · 2024-03-18T09:45:16Z

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

dmitryax · 2024-03-26T22:44:58Z

confmap/confmap.go

+			for i := 0; i < to.Type().NumField(); i++ {
+				// check embedded structs:
+				f := to.Type().Field(i)
+				if f.IsExported() && f.Anonymous && slices.Contains(strings.Split(f.Tag.Get("mapstructure"), ","), "squash") {


Why do we need to check for the mapstructure tag?

This is explained in the comment above: if the embedded struct is using the ,squash value for the mapstructure key, then this struct Unmarshal method should not be used as the main struct unmarshaling.

**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.

…squashing (#9861) This is taking a small slice of #9750 to document the behavior of confmap and make sure we can unmarshal embedded structs.

github-actions · 2024-04-10T03:15:56Z

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

atoulme requested a review from a team as a code owner March 13, 2024 01:30

atoulme requested a review from jpkrohling March 13, 2024 01:30

atoulme force-pushed the embedded_struct_unmarshaler branch 2 times, most recently from 0c0a748 to 311078f Compare March 13, 2024 01:51

atoulme mentioned this pull request Mar 13, 2024

[chore] reuse struct when parsing to avoid recursive parsing open-telemetry/opentelemetry-collector-contrib#31727

Merged

atoulme force-pushed the embedded_struct_unmarshaler branch from 311078f to 07fa33d Compare March 13, 2024 06:47

mx-psi requested a review from bogdandrutu March 13, 2024 14:00

mx-psi reviewed Mar 14, 2024

View reviewed changes

atoulme force-pushed the embedded_struct_unmarshaler branch 2 times, most recently from ccbf4cc to f8f4fe6 Compare March 14, 2024 19:57

atoulme mentioned this pull request Mar 14, 2024

[chore] Remove the top level error if it indicates an empty name #9763

Merged

atoulme commented Mar 14, 2024

View reviewed changes

confmap/confmap.go Outdated Show resolved Hide resolved

atoulme mentioned this pull request Mar 14, 2024

[chore] change the way we unmarshal the config in tests #9765

Merged

atoulme commented Mar 14, 2024

View reviewed changes

cmd/mdatagen/templates/component_test.go.tmpl Outdated Show resolved Hide resolved

atoulme force-pushed the embedded_struct_unmarshaler branch from 51e5f78 to dfa57f1 Compare March 14, 2024 21:35

atoulme mentioned this pull request Mar 15, 2024

Revert "[chore] change the way we unmarshal the config in tests" #9771

Merged

dmitryax pushed a commit that referenced this pull request Mar 15, 2024

Revert "[chore] change the way we unmarshal the config in tests" (#9771)

06f177a

Reverts #9765 We need to revert those changes as contrib has issues with them in isolation from #9750.

atoulme mentioned this pull request Mar 18, 2024

[chore] reuse the timer object when parsing open-telemetry/opentelemetry-collector-contrib#31802

Closed

mx-psi mentioned this pull request Mar 18, 2024

Investigate alternative for Unmarshaler and Marshaler #7101

Open

dmitryax reviewed Mar 26, 2024

View reviewed changes

This was referenced Mar 27, 2024

[confmap] support unmarshaling for embedded structs with and without squashing #9861

Merged

[confmap] Remove top level lock when considering struct as Unmarshalers. #9862

Open

github-actions bot added Stale and removed Stale labels Apr 10, 2024

atoulme added 2 commits April 20, 2024 17:24

[config] Deprecate config.UnmarshalConfig

3756383

add version to deprecation message

76b9c1f

atoulme force-pushed the embedded_struct_unmarshaler branch from 429f386 to 4445cc4 Compare April 21, 2024 00:25

bring back test in config as well

05a0d08

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[config] Deprecate config.UnmarshalConfig #9750

[config] Deprecate config.UnmarshalConfig #9750

atoulme commented Mar 13, 2024 •

edited

codecov bot commented Mar 13, 2024 •

edited

atoulme commented Mar 13, 2024

mx-psi Mar 14, 2024

atoulme Mar 14, 2024

mx-psi commented Mar 18, 2024

dmitryax Mar 26, 2024

atoulme Mar 26, 2024

github-actions bot commented Apr 10, 2024

[config] Deprecate config.UnmarshalConfig #9750

Are you sure you want to change the base?

[config] Deprecate config.UnmarshalConfig #9750

Conversation

atoulme commented Mar 13, 2024 • edited

codecov bot commented Mar 13, 2024 • edited

Codecov Report

atoulme commented Mar 13, 2024

mx-psi Mar 14, 2024

Choose a reason for hiding this comment

atoulme Mar 14, 2024

Choose a reason for hiding this comment

mx-psi commented Mar 18, 2024

dmitryax Mar 26, 2024

Choose a reason for hiding this comment

atoulme Mar 26, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 10, 2024

atoulme commented Mar 13, 2024 •

edited

codecov bot commented Mar 13, 2024 •

edited