Add Host property for Metadata #4736
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## main #4736 +/- ##
=======================================
Coverage 90.72% 90.73%
=======================================
Files 179 179
Lines 10689 10696 +7
=======================================
+ Hits 9698 9705 +7
Misses 770 770
Partials 221 221
Continue to review full report at Codecov.
|
|
@jpkrohling i tried to add in metadata |
Co-authored-by: Juraci Paixão Kröhling <[email protected]>
config/configgrpc/configgrpc.go
Outdated
| if len(md["Host"]) == 0 && len(md[":authority"]) > 0 { | ||
| copiedMD["Host"] = md[":authority"] | ||
| } |
There was a problem hiding this comment.
should we remove the ":authority" in this case? (Not sure, but want to make sure somebody thinks about this).
There was a problem hiding this comment.
I think no, because it's real header
There was a problem hiding this comment.
Agree, at least as part of this PR here. We might want to start a discussion for something like "semantic conventions" for the headers.
There was a problem hiding this comment.
@jpkrohling Is this comment related to host const?
There was a problem hiding this comment.
I was agreeing in keeping :authority here, as it's the key used by gRPC, and would expect to see it there. The second part of my comment was about having a single value where downstream components would expect the "host" value to exist, but this is being addressed in this PR already. Sorry about the noise.
There was a problem hiding this comment.
"semantic conventions" for the headers.
This also may include us having the constant values for things like "Host", that's why I got confused. So you are ok with having that constant?
There was a problem hiding this comment.
Yes, at least for the moment. If we keep finding ourselves doing it often, we might want to split that out.
| @@ -400,7 +400,11 @@ func contextWithClient(ctx context.Context, includeMetadata bool) context.Contex | |||
| } | |||
| if includeMetadata { | |||
| if md, ok := metadata.FromIncomingContext(ctx); ok { | |||
| cl.Metadata = client.NewMetadata(md.Copy()) | |||
| copiedMD := md.Copy() | |||
| if len(md[client.MetadataHostName]) == 0 && len(md[":authority"]) > 0 { | |||
There was a problem hiding this comment.
I see now what @bogdandrutu meant earlier. I agree with the approach taken here, even though we end up with the host information duplicated in the client.Info. I don't think this is a problem.
|
ready to merge or this PR has some problems? |
|
@bogdandrutu would you please do the honours? |
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
This reverts commit da91c56.
Description: Add Host property for Metadata
Link to tracking Issue: #4722