-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Changes from 1 commit
46af5f9
1fd9e20
8a70332
27c10aa
11a5313
1b2cb8c
18dab87
037ad1e
4d2490f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -167,6 +167,14 @@ | |
|
||
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). | ||
|
||
func (ht *serverHandlerTransport) Peer() *peer.Peer { | ||
return &peer.Peer{ | ||
Addr: ht.RemoteAddr(), | ||
} | ||
} | ||
|
||
// strAddr is a net.Addr backed by either a TCP "ip:port" string, or | ||
// the empty string if unknown. | ||
type strAddr string | ||
|
@@ -347,7 +355,7 @@ | |
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
// With this transport type there will be exactly 1 stream: this HTTP request. | ||
|
||
ctx := ht.req.Context() | ||
|
@@ -371,16 +379,16 @@ | |
}() | ||
|
||
req := ht.req | ||
|
||
s := &Stream{ | ||
id: 0, // irrelevant | ||
requestRead: func(int) {}, | ||
cancel: cancel, | ||
buf: newRecvBuffer(), | ||
st: ht, | ||
method: req.URL.Path, | ||
recvCompress: req.Header.Get("grpc-encoding"), | ||
contentSubtype: ht.contentSubtype, | ||
id: 0, // irrelevant | ||
requestRead: func(int) {}, | ||
cancel: cancel, | ||
buf: newRecvBuffer(), | ||
st: ht, | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Done. Thanks. |
||
} | ||
pr := &peer.Peer{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reuse There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, good point. Done. |
||
Addr: ht.RemoteAddr(), | ||
|
@@ -390,15 +398,6 @@ | |
} | ||
ctx = metadata.NewIncomingContext(ctx, ht.headerMD) | ||
s.ctx = peer.NewContext(ctx, pr) | ||
for _, sh := range ht.stats { | ||
s.ctx = sh.TagRPC(s.ctx, &stats.RPCTagInfo{FullMethodName: s.method}) | ||
inHeader := &stats.InHeader{ | ||
FullMethod: s.method, | ||
RemoteAddr: ht.RemoteAddr(), | ||
Compression: s.recvCompress, | ||
} | ||
sh.HandleRPC(s.ctx, inHeader) | ||
} | ||
s.trReader = &transportReader{ | ||
reader: &recvBufferReader{ctx: s.ctx, ctxDone: s.ctx.Done(), recv: s.buf, freeBuffer: func(*bytes.Buffer) {}}, | ||
windowHandler: func(int) {}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ import ( | |
"google.golang.org/grpc/internal/channelz" | ||
"google.golang.org/grpc/keepalive" | ||
"google.golang.org/grpc/metadata" | ||
"google.golang.org/grpc/peer" | ||
"google.golang.org/grpc/resolver" | ||
"google.golang.org/grpc/stats" | ||
"google.golang.org/grpc/status" | ||
|
@@ -265,7 +266,8 @@ type Stream struct { | |
// headerValid indicates whether a valid header was received. Only | ||
// meaningful after headerChan is closed (always call waitOnHeader() before | ||
// reading its value). Not valid on server side. | ||
headerValid bool | ||
headerValid bool | ||
headerWireLength int // Only set on server side. | ||
|
||
// hdrMu protects header and trailer metadata on the server-side. | ||
hdrMu sync.Mutex | ||
|
@@ -425,6 +427,12 @@ func (s *Stream) Context() context.Context { | |
return s.ctx | ||
} | ||
|
||
// SetContext sets the context of the stream. This will be deleted once the | ||
// stats handler callouts all move to gRPC layer. | ||
func (s *Stream) SetContext(ctx context.Context) { | ||
s.ctx = ctx | ||
} | ||
|
||
// Method returns the method for the stream. | ||
func (s *Stream) Method() string { | ||
return s.method | ||
|
@@ -437,6 +445,12 @@ func (s *Stream) Status() *status.Status { | |
return s.status | ||
} | ||
|
||
// HeaderWireLength returns the size of the headers of the stream as received | ||
// from the wire. Valid only on the server. | ||
func (s *Stream) HeaderWireLength() int { | ||
return s.headerWireLength | ||
} | ||
|
||
// SetHeader sets the header metadata. This can be called multiple times. | ||
// Server side only. | ||
// This should not be called in parallel to other data writes. | ||
|
@@ -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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Decided to do this, added this comment. |
||
HandleStreams(func(*Stream)) | ||
HandleStreams(context.Context, func(*Stream)) | ||
|
||
// WriteHeader sends the header metadata for the given stream. | ||
// WriteHeader may not be called on all streams. | ||
|
@@ -717,9 +731,15 @@ type ServerTransport interface { | |
// handlers will be terminated asynchronously. | ||
Close(err error) | ||
|
||
// LocalAddr returns the local network address. | ||
LocalAddr() net.Addr | ||
|
||
// RemoteAddr returns the remote network address. | ||
RemoteAddr() net.Addr | ||
|
||
// Peer returns the peer of the server transport. | ||
Peer() *peer.Peer | ||
Comment on lines
+734
to
+735
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
|
||
// Drain notifies the client this ServerTransport stops accepting new RPCs. | ||
Drain(debugData string) | ||
|
||
|
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.