[confighttp] Move keepalive config into a dedicated optional section

[swiatekm]

Description

Move the keepalive-related options into a dedicated optional section.

We go from:

endpoint: "http://localhost:4318"
idle_conn_timeout: 90s
max_idle_conns: 100
max_idle_conns_per_host: 10

to:

endpoint: "http://localhost:4318"
keepalive:
  idle_conn_timeout: 90s
  max_idle_conns: 100
  max_idle_conns_per_host: 10

The old configuration still exists and works. We fail validation if both are set.

Link to tracking issue

Fixes #14020

Testing

Added unit tests for all the new functions and some more generic unmarshalling tests.

Documentation

Updated README and the config schema.

[codecov]

Codecov Report

❌ Patch coverage is 94.80519% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.03%. Comparing base (52e6bf4) to head (33215dc).

Files with missing lines	Patch %	Lines
config/confighttp/client.go	95.00%	1 Missing and 1 partial ⚠️
config/confighttp/server.go	93.93%	1 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (94.80%) 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   #15308   +/-   ##
=======================================
  Coverage   91.03%   91.03%           
=======================================
  Files         729      729           
  Lines       48491    48556   +65     
=======================================
+ Hits        44144    44205   +61     
- Misses       3020     3022    +2     
- Partials     1327     1329    +2     

☔ View full report in Codecov by Harness.
📢 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]

The approach looks good to me. I took a slightly different one when I did this on the Datadog exporter a few years ago, see open-telemetry/opentelemetry-collector-contrib/pull/8490, but I like this insofar as it removes the fields from the API now instead of later.

I left one comment about something I am not sure about.

[swiatekm]

Converting back to draft, as I noticed a problem while fixing the breakage in contrib. confmap doesn't gracefully handle the situation where an embedded struct with mapstructure: squash has an Unmarshal method, but the parent struct doesn't. This will require a fix in confmap that should go into a separate PR. I'm looking at how other decoding libraries handle it.

[swiatekm]

Updated to keeping the deprecated fields and checking that both aren't set during validation. This makes this change non-breaking for the api consumers, at least in theory.

In practice, it is a breaking change if the user instantiates the struct via its zero value rather than the New*Config() constructor. The zero value of disable_keep_alives is false, so "keepalives enabled". The zero value of a configoptional.Optional is None, which in this case means "keepalives disabled". In order to make this not a breaking change, we'd need to fix 30+ contrib components to use the constructor instead.

The contrib test failures are relatively benign and have more to do with tests assuming a marshal -> unmarshal roundtrip is idempotent, which is not true for configoptional.Default. They can be fixed easily.

[swiatekm]

I wanted to summarize the problem and the solutions I considered. If there's a silver bullet without downsides that I'm missing, I'd love to know what it is.

What do we want?

1. Deprecate the existing fields while still accepting them.
2. Add a new section which is recommended from now on, with default values.
3. Error if the user sets both.

What is the problem?

In order to achieve 3, we need to know the difference between a default and a user setting for the new fields.

What are the possible solutions?

Use configoptional.Default and extend it

configoptional.Default already distinguishes between an implicit default and an explicit value set by the user. The API is somewhat awkward for this use case but it does what we need.

Use a custom Unmarshaller

This works, but custom Unmarshallers have limitations. Notably, unexpected things happen when the struct is embedded in another struct. In effect, the parent struct needs to define their own Unmarshal that calls the child structs' Unmarshal methods.

Some components in contrib do this. This was the first approach of this PR. It's effectively a breaking change, albeit for a relatively small amount of downstream consumers.

Don't error on conflicts

We can decide to give the deprecated fields priority and simply ignore the new fields if the former are set. This is a less user-friendly option, but it's easy to implement.

???

If anyone has an alternative that I haven't considered, I'd love to hear it.

[evan-bradley]

Looking through contrib, this would be a massive change, but our guidelines state that embedded config structs should be avoided. If we made an API change to all contrib components to shift this for at least confighttp, the Unmarshaler implementation would work and we could use that to help with this transition.

[mx-psi]

I filed open-telemetry/opentelemetry-collector-contrib/pull/49789, open-telemetry/opentelemetry-collector-contrib/pull/49795, open-telemetry/opentelemetry-collector-contrib/pull/49796 and open-telemetry/opentelemetry-collector-contrib/pull/49797 to stop embedding confighttp and configgrpc fields.

If this seems like an acceptable solution I can mark them as ready for review or split them into smaller PRs.