[configgrpc, confighttp] update headers to array of name/value pairs to match configuration schema

[codeboten]

Component(s)

No response

Describe the issue you're reporting

Starting with version 0.3.0 of the configuration schema, the headers field was changed to use a name/value pair (see open-telemetry/opentelemetry-configuration#115). Configuration looks like this:

Before v0.3.0

          headers:
            "key1": "value1"

v0.3.0 and newer

          headers:
          - name: "key1"
            value: "value1"

Supporting this natively in the collector means we now have multiple ways of configuring what feels like the same field (i know its configured for different components/areas but as a user, it's confusing).

I'm opening this issue to discuss migrating configgrpc's and confighttp's headers field to match the configuration schema. Doing this would require feature gates to support both formats for some version, and warning end users of the change.

An alternative to supporting only one could be to support both moving forward.

Tip

_{React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.}

[mx-psi]

I think it would be best to support both. I believe the only change needed before 1.x for this would be to change from map[string]configopaque.String to a new Headers type with a custom unmarshal function

[mx-psi]

#13931 shows that this is a feasible approach. I can work on a PoC for this.

[mx-psi]

Ah, I got that wrong, while the Unmarshal method works, mapstructure will fail before getting to the hook(thanks @jade-guiton-dd for pointing this out).

The way to support both then would be to add an Unmarshal method to ClientConfig. I think that we can have a PoC for this but don't necessarily need to merge it for marking confighttp/configgrpc as 1.x

[jade-guiton-dd]

I wrote two PoCs for supporting both maps and lists of name/value pairs in configgrpc.ClientConfig.Headers (and this could be generalized to confighttp.ClientConfig.Headers and confighttp.ServerConfig.ResponseHeaders).

The first one follows @mx-psi's above comment, ie. by defining a ClientConfig.Unmarshal method. It's not very elegant, and couldn't be reused as-is for ResponseHeaders if we want to keep the error messages sensible, but it doesn't require any API breaking changes.

The second one is based on the realization that, if Unmarshal isn't called when the input isn't a map[string]any, we can make the list representation the default, and use Unmarshal to handle the map case. In that way, we can concentrate the logic inside a new reusable Map type. This is more elegant, and would produce better error messages (no need to call conf.Unmarshal to parse the rest of ClientConfig), but it would be an API breaking change, meaning we would have to implement this before 1.0 if we want to go that route.

[jade-guiton-dd]

I filed #13996 as a more polished and complete version of my second PoC, which introduces a self-contained configopaque.MapList type as a replacement for map[string]opaque.String which also handles unmarshaling from a list. As previously noted, this is relatively elegant, but is a fairly big breaking change (hence all the failing contrib tests). Do you think it's worth it? If so, I'll mark it as ready for review and work on the corresponding contrib changes.

[jmacd]

I support the elegant/breaking variation.