-
Notifications
You must be signed in to change notification settings - Fork 112
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
Expose peer information to handlers and clients #364
Conversation
Where the generated code replaces Request with a stream, expose the Spec.
On requests and streams, expose the peer's address. For clients, the address is the host or host:port, parsed from the URL. For servers, the address is an IP:port pair, provided by `net/http` as `Request.RemoteAddr`. In #357, a handful of users asked for this information for logging, per-IP rate limiting, and a variety of other server-side concerns. It's also necessary for OpenTelemetry interceptors (#344). Fixes #357.
@elliotmjackson and @joshcarp, added you as reviewers in case you're interested or want to use this as a chance to start digging into |
@@ -217,6 +220,10 @@ type connectClient struct { | |||
protocolClientParams | |||
} | |||
|
|||
func (c *connectClient) Peer() Peer { | |||
return newPeerFromURL(c.URL) |
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 appears to be the URL passed to the client constructor. Do you think people would want to have this be set to the URL which was ultimately accessed (in case of redirects)?
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.
Possibly? I'm not sure how to accomplish that, though, since we need this information before we've attempted to make a request.
// contains the host or host:port from the server's URL. When accessed | ||
// server-side, Addr contains the client's address in IP:port format. | ||
type Peer struct { | ||
Addr 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.
gRPC exposes this as net.Addr
(which includes a Network string: https://pkg.go.dev/google.golang.org/grpc/peer#Peer). Do you think we won't need to provide that additional data?
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.
Possibly? We can always expose a Network
attribute down the road too. I'm not sure how to reliably detect non-TCP transports through the net/http abstractions - for example, unix domain sockets are sort of the wild west, and I have no idea what HTTP/3 will end up looking like.
* Add Spec to some user-facing streams Where the generated code replaces Request with a stream, expose the Spec. * Expose peer information to servers and clients On requests and streams, expose the peer's address. For clients, the address is the host or host:port, parsed from the URL. For servers, the address is an IP:port pair, provided by `net/http` as `Request.RemoteAddr`. In #357, a handful of users asked for this information for logging, per-IP rate limiting, and a variety of other server-side concerns. It's also necessary for OpenTelemetry interceptors (#344). Fixes #357.
On requests and streams, expose the peer's address. For clients, the address is the host or host:port, parsed from the URL. For servers, the address is an IP:port pair, provided by
net/http
asRequest.RemoteAddr
.In https://github.com/bufbuild/connect-go/issues/357, a handful of users asked for this information for logging, per-IP rate limiting, and a variety of other server-side concerns. It's also necessary for OpenTelemetry interceptors (https://github.com/bufbuild/connect-go/issues/344).
Fixes https://github.com/bufbuild/connect-go/issues/357.