Don't call Marshal in unmarshalerEmbeddedStructsHookFunc

[jade-guiton-dd]

Context

confmap uses a mapstructure hook to call the Unmarshal method on types which define it as part of the unmarshaling process, but mapstructure hooks are not called for squashed fields, so we have a separate hook which manually inspects all structs for squashed fields that implement Unmarshaler.

This second hook currently returns a modified version of the raw YAML config, to be used by mapstructure to finish unmarshaling the full struct. This is a problem, because an Unmarshal method can set the state of the struct in arbitrary ways, which we want to preserve without mapstructure overwriting fields. (See TestEmbeddedUnmarshaler)

The way this is currently worked around is by marshaling the embedded struct into a map, and then copying that map back into the raw config, in the hope that nothing will change when the raw config is mapped back into the struct.

However, this assumption breaks when dealing with a configopaque.String inside an embedded Unmarshaler. That type has the very deliberate behavior of returning [REDACTED] when marshaled instead of its true contents, meaning marshaling then unmarshaling does not round trip.

Description

This PR attempts to fix this issue and make embedded Unmarshalers more reliable by modifying the hook so that it returns a fully unmarshaled struct, leading mapstructure to do nothing. (This is what the basic Unmarshaler hook does to avoid "interference".)

This means the hook needs to somehow unmarshal the non-Unmarshaler subfields manually, without affecting the input or output of Unmarshal. I did this by constructing a custom "partial" struct using reflect.StructOf which only contains the non-Unmarshaler fields, copying the initial values into it, calling Decode on it, then copying the output values back into the original struct.

In a previous version of this PR, I kept most of the hook intact, but changed the logic so that we clear fields from the raw config when they might conflict with the embedded struct (instead of setting those fields to the embedded struct's current value). However, this has the side effect of breaking unmarshaling of structs with multiple identically-named fields, of which there is at least one in contrib (both docker.Config and scraperhelper.ControllerConfig have a Timeout field, and one of them implements Unmarshaler).

[codspeed-hq]

CodSpeed Performance Report

Merging #14213 will degrade performances by 31.6%

_{Comparing jade-guiton-dd:no-marshal-in-unmarshal-hook (51180e9) with main (43396d4)}

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

Summary

⚡ 4 improvements
❌ 3 regressions
✅ 66 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
⚡	zstdNoConcurrency	65.1 µs	36 µs	+81.02%
⚡	BenchmarkSplittingBasedOnItemCountHugeLogs	44 ms	34.8 ms	+26.27%
⚡	BenchmarkSplittingBasedOnItemCountHugeMetrics	119.2 ms	93.6 ms	+27.42%
⚡	BenchmarkSplittingBasedOnItemCountManyMetricsSlightlyAboveLimit	112.8 ms	85.1 ms	+32.47%
❌	BenchmarkLogsToProto	3.1 µs	4.3 µs	-28.68%
❌	BenchmarkTracesToProto	4.7 µs	6.3 µs	-25.73%
❌	BenchmarkTraceSizeBytes	298.8 µs	436.9 µs	-31.6%

[codecov]

Codecov Report

❌ Patch coverage is 89.47368% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.17%. Comparing base (43396d4) to head (51180e9).

Files with missing lines	Patch %	Lines
confmap/internal/decoder.go	89.47%	2 Missing and 2 partials ⚠️

❌ Your patch check has failed because the patch coverage (89.47%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14213      +/-   ##
==========================================
- Coverage   92.17%   92.17%   -0.01%     
==========================================
  Files         668      668              
  Lines       41463    41485      +22     
==========================================
+ Hits        38219    38237      +18     
- Misses       2212     2214       +2     
- Partials     1032     1034       +2     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mx-psi]

This seems to fix the issue that motivated this over at #14203

I would be okay with

breaking unmarshaling of structs with multiple identically-named fields

I am a bit worried about how to test this. Are there any tests we could add here for this?

[jade-guiton-dd]

There are already tests for the current embedded unmarshaler behavior, but I think it would be worth adding one using configopaque.String, to show that it now works.

[jade-guiton-dd]

I added a test that uses a type similar to configopaque.String, which failed before this PR and succeeds now.

[jade-guiton-dd]

This is pretty sensitive stuff, so I think more reviews would be good to ensure it won't break anything that's not currently tested for.

[jmacd]

I couldn't parse this comment.

The PR description was very helpful. I suggest maybe adding a comment to give future readers an overview of what's happening on lines 200-265 or so.

[jade-guiton-dd]

I reworded the comment and moved that code to the place it's needed (just before the recursive call to Decode). Tell me if that makes things clearer.

[mx-psi]

I'll merge this on Tuesday if there have been no comments on this or earlier if @evan-bradley approves before then