[configgrpc, confighttp] Support lists of name/value pairs for headers #13996
+427
−89
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13996 +/- ##
==========================================
+ Coverage 91.61% 91.65% +0.03%
==========================================
Files 655 657 +2
Lines 42793 42864 +71
==========================================
+ Hits 39205 39287 +82
+ Misses 2765 2758 -7
+ Partials 823 819 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
codeboten
reviewed
Oct 15, 2025
songy23
pushed a commit
to open-telemetry/opentelemetry-collector-contrib
that referenced
this pull request
Oct 15, 2025
#### Description #43509 changed the `make for-all` Makefile target to access the `ALL_MODS` variable through environment variables instead of expanding it into the shell command, in order to bypass Windows' 8192 byte command line length limit. However, the environment variable was not defined, which led to `make for-all` becoming a no-op. This can be confirmed by running `make for-all CMD='echo test'`. (I discovered this issue because [this PR](open-telemetry/opentelemetry-collector#13996) in core repository, which should be failing `contrib-tests`, suddenly started passing them. This was because the job inserts `replace` statements in a copy of contrib using `for-all`; failing to do that caused the contrib tests to run against whatever version of core is imported here rather than against the PR's code.) This PR fixes that oversight, by adding the `export` keyword to the `ALL_MODS` variable.
|
Next steps:
|
ChrsMark
pushed a commit
to ChrsMark/opentelemetry-collector-contrib
that referenced
this pull request
Oct 20, 2025
#### Description open-telemetry#43509 changed the `make for-all` Makefile target to access the `ALL_MODS` variable through environment variables instead of expanding it into the shell command, in order to bypass Windows' 8192 byte command line length limit. However, the environment variable was not defined, which led to `make for-all` becoming a no-op. This can be confirmed by running `make for-all CMD='echo test'`. (I discovered this issue because [this PR](open-telemetry/opentelemetry-collector#13996) in core repository, which should be failing `contrib-tests`, suddenly started passing them. This was because the job inserts `replace` statements in a copy of contrib using `for-all`; failing to do that caused the contrib tests to run against whatever version of core is imported here rather than against the PR's code.) This PR fixes that oversight, by adding the `export` keyword to the `ALL_MODS` variable.
|
I pushed the corresponding changes in contrib on my fork here; all in all, about 150 small changes, mostly in tests. Along the way, I ended up modifying this PR's core API to make it match the native
I will mark this PR as ready for review, but as a reminder, we can still change course and go for the "backwards-compatible" approach if the benefits of this PR are not deemed to be worth the additional complexity. |
jade-guiton-dd
requested review from
a team,
dmitryax,
evan-bradley and
mx-psi
as code owners
mx-psi
reviewed
Oct 21, 2025
config/configgrpc/configgrpc.go
Outdated
|
|
||
| // The headers associated with gRPC requests. | ||
| Headers map[string]configopaque.String `mapstructure:"headers,omitempty"` | ||
| Headers *configopaque.MapList `mapstructure:"headers,omitempty"` |
There was a problem hiding this comment.
If we can do without the * that would be better
config/confighttp/client.go
Outdated
| // Existing header values are overwritten if collision happens. | ||
| // Header values are opaque since they may be sensitive. | ||
| Headers map[string]configopaque.String `mapstructure:"headers,omitempty"` | ||
| Headers *configopaque.MapList `mapstructure:"headers,omitempty"` |
|
I am going to mark this as draft until it is ready for another review, feel free to mark as ready whenever that happens! |
|
I think I've addressed all the previous comments, and I've filed a corresponding draft PR in contrib (open-telemetry/opentelemetry-collector-contrib#43701) which is almost passing tests, so I'll mark this as ready for another round of review. |
mx-psi
reviewed
Oct 23, 2025
mx-psi
approved these changes
Oct 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces:
MapList[T]type, which is equivalent to[]{ Name string, Value T }, but can be unmarshalled from amap[string]Tas well. This type is defined in a newinternal/maplistpackage.configopaque.MapListconfiggrpcandconfighttpare changed to use this type instead ofmap[string]configopaque.Stringfor storing header configuration, so as to be consistent with the SDK declarative config (see tracking issue).Note that while this is relatively elegant, it causes a number of breaking changes. An alternative would be to keep the current
map[string]configopaque.Stringtypes, and add an Unmarshal method on the surrounding struct: see this PoC.Link to tracking issue
Fixes #13930
Testing
I added a basic unit test in
internal/maplist.