Include request headers in client.Info

[disq]

Description: Exposes HTTP headers from the originating request in the client.Info struct, to be passed on to processors.

This was suggested in the contrib issue open-telemetry/opentelemetry-collector-contrib#6700
I tried to keep it in-line with the Header field in http.Request (and grpc metadata.MD) as possible (so I just used map[string][]string in an immutable struct with a getter)

Closes #2401

[jpkrohling]

This looks like a good start, but we are missing:

• A similar change to the configgrpc, adding the metadata to the Header. This might be helpful: https://pkg.go.dev/google.golang.org/grpc/metadata#MD
• Tests

[jpkrohling]

LGTM

[codecov]

Codecov Report

Merging #4547 (8fb3a30) into main (2317957) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4547      +/-   ##
==========================================
+ Coverage   90.52%   90.54%   +0.02%     
==========================================
  Files         179      179              
  Lines       10638    10663      +25     
==========================================
+ Hits         9630     9655      +25     
  Misses        785      785              
  Partials      223      223              

Impacted Files	Coverage Δ
client/client.go	100.00% <100.00%> (ø)
config/configgrpc/configgrpc.go	92.57% <100.00%> (+0.30%)	⬆️
config/confighttp/clientinfohandler.go	100.00% <100.00%> (ø)
config/confighttp/confighttp.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2317957...8fb3a30. Read the comment docs.

[bogdandrutu]

@disq @jpkrohling can we document better the motivation of this?

[disq]

@bogdandrutu Since I'm running the collector behind a load balancer, my motivation is actually same one as having the net.Addr in the client.Info: having access to the "real" client IP in the processor.

There are other motivations such as running a multi-tenant infra to distinguish between tenants without having to inject something in the actual OTLP data, which is documented in #2401

[jpkrohling]

@disq @jpkrohling can we document better the motivation of this?

I linked #2401 to this, which should provide enough background on the request.

[jpkrohling]

Apart from a typo and improved godoc for the new property, this looks good to me.

[jpkrohling]

There's a test failure here:

=== RUN   TestContextWithClient/existing_client_with_metadata,_no_metadata_processing
    configgrpc_test.go:660: 
        	Error Trace:	configgrpc_test.go:660
        	Error:      	Not equal: 
        	            	expected: client.Info{Addr:net.Addr(nil), Auth:client.AuthData(nil), Metadata:map[string][]string(nil)}
        	            	actual  : client.Info{Addr:net.Addr(nil), Auth:client.AuthData(nil), Metadata:map[string][]string{"test-metadata-key":[]string{"test-value"}}}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -3,3 +3,7 @@
        	            	  Auth: (client.AuthData) <nil>,
        	            	- Metadata: (map[string][]string) <nil>
        	            	+ Metadata: (map[string][]string) (len=1) {
        	            	+  (string) (len=17) "test-metadata-key": ([]string) (len=1) {
        	            	+   (string) (len=10) "test-value"
        	            	+  }
        	            	+ }
        	            	 }
        	Test:       	TestContextWithClient/existing_client_with_metadata,_no_metadata_processing

[jpkrohling]

@disq, you have enough approvals :-) Could you fix the changelog conflict? Once this is done, this PR will be merged.

[disq]

@disq, you have enough approvals :-) Could you fix the changelog conflict? Once this is done, this PR will be merged.

Another conflict? Fixed

[jpkrohling]

Another conflict? Fixed

Sorry about that, we do need to find a better solution for the frequent conflicts involving the changelog.

[bogdandrutu]

Sorry I have to blocked this again, but I missed it. We cannot make Info (a.k.a an object in the context) mutable unless is thread-safe because the context is shared between multiple gorutines potentially.

[bogdandrutu]

Do we want users to be able to select which "metadata" to copy from the request? Maybe not now, but we can be safe and mark this experimental for the moment.

[disq]

Certainly doable, blindly copying all of it could be bad for memory if the request is decorated with large metadata? But that would also mean we'd have to roll our own md.Copy() or header.Clone() routines, as copying-first-then-filtering would be pointless in terms of memory.

[bogdandrutu]

That's why I recommend just adding a comment that this is experimental, and we should look a bit into how/if we want this.

[disq]

@bogdandrutu added