[confmap] Enable DecodeNil option

[mx-psi]

Description

Enables DecoderConfig.DecodeNil

Link to tracking issue

Taken from #10260
Updates #12981

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.64%. Comparing base (5800834) to head (2057ce2).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12996      +/-   ##
==========================================
+ Coverage   91.62%   91.64%   +0.02%     
==========================================
  Files         504      504              
  Lines       27888    27886       -2     
==========================================
+ Hits        25553    25557       +4     
+ Misses       1844     1840       -4     
+ Partials      491      489       -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.

[yurishkuro]

The DecodeHookFuncType style hooks are fundamentally incompatible with DecodeNil because they are forcing a loss of information, see go-viper/mapstructure#37 (comment). I was even suggesting a new "ultimate" hook type but later realized that we can get away with DecodeHookFuncValue if we change our hook implementations to return reflect.Value (allowed by the signature) instead of the actual interface, and @mahadzaryab1 added a fix for that in go-viper/mapstructure#52

So TLDR - OTEL hooks need to be modified to match DecodeHookFuncValue signature AND to return the reflect.Value (especially when they are not making any changes) to preserve the type information.

[mx-psi]

@yurishkuro Okay, thanks. It still would make sense to me for mapstructure to give a more meaningful error rather than panic here, but I see what you mean.

Can you review #13001 to move existing hooks into reflect.Value?

[mx-psi]

@evan-bradley You can see on https://app.codecov.io/gh/open-telemetry/opentelemetry-collector/pull/12996/indirect-changes that this change leads to increased coverage on somes lines added over at #13001

Is there any other test you think we should add? I am honestly a bit scared every time I make changes on confmap, but in this case I am not even sure about what tests I would add here

[evan-bradley]

@evan-bradley You can see on app.codecov.io/gh/open-telemetry/opentelemetry-collector/pull/12996/indirect-changes that this change leads to increased coverage on somes lines added over at #13001

Great, thank you.

Is there any other test you think we should add?

I can look through it more, some initial ideas:

1. Are there any tests we should re-add from [confmap] Maintain nil slice values when marshaling and unmarshaling #11882?
2. Were any tests added that verify we won't run into the same issue that [confmap] Maintain nil slice values when marshaling and unmarshaling #11882 introduced?

I am honestly a bit scared every time I make changes on confmap, but in this case I am not even sure about what tests I would add here

Same here. I think if we merge this after today's release, that should hopefully give us two weeks to at least think it over or potentially catch any issues found by anyone using the latest from main.

[evan-bradley]

We discussed this offline and #12871 added tests that cover both of the situations I was asking about. At the moment I can't think of any other scenarios we need to test.