-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
grpc: Move some stats handler calls to gRPC layer, and add local address to peer.Peer #6716
grpc: Move some stats handler calls to gRPC layer, and add local address to peer.Peer #6716
Conversation
026fed9
to
46af5f9
Compare
internal/transport/handler_server.go
Outdated
@@ -167,6 +167,14 @@ func (ht *serverHandlerTransport) Close(err error) { | |||
|
|||
func (ht *serverHandlerTransport) RemoteAddr() net.Addr { return strAddr(ht.req.RemoteAddr) } | |||
|
|||
func (ht *serverHandlerTransport) LocalAddr() net.Addr { return nil } // Server Handler transport has no access to local addr (was simply not calling sh with local addr). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can be obtained with: ht.req.Context().Value(http.LocalAddrContextKey)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, done. Since you only want peer though will be added to the Peer() method and this will be deleted.
internal/transport/handler_server.go
Outdated
method: req.URL.Path, | ||
recvCompress: req.Header.Get("grpc-encoding"), | ||
contentSubtype: ht.contentSubtype, | ||
headerWireLength: 0, // doesn't know header wire length, will call into stats handler as 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one doesn't seem to be available. I think we'll need:
Please leave a comment in the code referencing the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks.
internal/transport/handler_server.go
Outdated
method: req.URL.Path, | ||
recvCompress: req.Header.Get("grpc-encoding"), | ||
contentSubtype: ht.contentSubtype, | ||
headerWireLength: 0, // doesn't know header wire length, will call into stats handler as 0. | ||
} | ||
pr := &peer.Peer{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reuse Peer()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. Done.
internal/transport/handler_server.go
Outdated
@@ -347,7 +355,7 @@ func (ht *serverHandlerTransport) WriteHeader(s *Stream, md metadata.MD) error { | |||
return err | |||
} | |||
|
|||
func (ht *serverHandlerTransport) HandleStreams(startStream func(*Stream)) { | |||
func (ht *serverHandlerTransport) HandleStreams(_ context.Context, startStream func(*Stream)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't ignoring the context mess everything up?
Can grpc not pass the http.Request.Context()
(with things like Peer added) so that it doesn't need to be ignored here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was being ignored in master already, and it uses the requests Context (ctx := ht.req.Context()). I think this is the same functionality, and I don't think I want to add something to gRPC layer using the http.Request since it's a server handler transport specific concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline; decided to move peer creation to construction time and read it in gRPC before calling this.
@@ -698,7 +712,7 @@ type ClientTransport interface { | |||
// Write methods for a given Stream will be called serially. | |||
type ServerTransport interface { | |||
// HandleStreams receives incoming streams using the given handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the Context given is used as the base context for all streams started on this transport."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per comment above, will wait until further discussion on the server handler transport to add this docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to do this, added this comment.
// Peer returns the peer of the server transport. | ||
Peer() *peer.Peer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we delete Local/RemoteAddr and only have this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done. There's an in flight PR for this but the contributor isn't getting back to my comments so I'll go ahead and just add it to this PR.
server.go
Outdated
// GetConnection gets the connection from the context. | ||
func GetConnection(ctx context.Context) net.Conn { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want this to be public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm ok I'll plumb it through /internal good point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pass. Got to all comments.
@zasweq -- seems like there is a conflict. Adding you to the assignees to take care of that. |
Ah thanks resolved |
internal/internal.go
Outdated
@@ -62,6 +62,8 @@ var ( | |||
// gRPC server. An xDS-enabled server needs to know what type of credentials | |||
// is configured on the underlying gRPC server. This is set by server.go. | |||
GetServerCredentials any // func (*grpc.Server) credentials.TransportCredentials | |||
// GetConnection gets the connection from the context. | |||
GetConnection any // func (context.Context) net.Conn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fine to give this a type to avoid the casts, since it's only things from the stdlib.
But, it should also be fine to implement it fully within the internal package. But then.. why not just leave it where it is and export the setter? Or find another place in internal for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about this, decided to move back to transport. I don't really think any of the current internal directories make semantical sense, and as per offline discussion I think there's complications about trying to keep file paths consistent.
a57125e
to
8a70332
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo the last comment.
internal/transport/handler_server.go
Outdated
@@ -75,11 +75,25 @@ func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request, stats []s | |||
return nil, errors.New(msg) | |||
} | |||
|
|||
var localAddr net.Addr | |||
if la := r.Context().Value(http.LocalAddrContextKey); la != nil { | |||
localAddr = la.(net.Addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will panic if the wrong type is here. localAddr, _ = ...
is safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…ess to peer.Peer (grpc#6716)
This PR moves ConnBegin, ConnEnd, and InHeader from the transport layer to the gRPC layer. The other two callouts still in the transport, OutHeader and OutTrailer will require a reworking of the ownership of streaming code and logic with respect to functionality in gRPC layer and transport layer. Continuation of #6624.
RELEASE NOTES: