Skip to content
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

add LocalAddr to peer.Peer client side #6577

Closed
rittneje opened this issue Aug 24, 2023 · 7 comments · Fixed by #6779
Closed

add LocalAddr to peer.Peer client side #6577

rittneje opened this issue Aug 24, 2023 · 7 comments · Fixed by #6779
Assignees
Labels
Hacktoberfest P2 Status: Help Wanted Type: Feature New features or improvements in behavior

Comments

@rittneje
Copy link

Use case(s) - what problem will this feature solve?

We want to be able to log the local address in addition to the remote address from our server-side interceptors to aid in debugging.

Proposed Solution

Add LocalAddr net.Addr to peer.Peer.

Alternatives Considered

This information could also be made available via some other context attribute. It doesn't really matter as long as there is a way to get it.

Additional Context

This was already added to the Java implementation several years ago. grpc/grpc-java#4906

@rittneje rittneje added the Type: Feature New features or improvements in behavior label Aug 24, 2023
@mingdaoy
Copy link

mingdaoy commented Oct 1, 2023

hi @ginayeh, I'd like to take a stab at this. Could you assign it to me?

prestonvasquez added a commit to prestonvasquez/grpc-go that referenced this issue Oct 8, 2023
prestonvasquez added a commit to prestonvasquez/grpc-go that referenced this issue Oct 8, 2023
prestonvasquez added a commit to prestonvasquez/grpc-go that referenced this issue Oct 8, 2023
@arvindbr8
Copy link
Member

arvindbr8 commented Oct 16, 2023

@mingdaoy -- sorry about that. seems like we already have had @prestonvasquez with a fix in flight for it #6702.

Please use this filter to look for issues that are marked "Help wanted" and currently are unassigned.

@CarlosRDomin
Copy link
Contributor

I've been playing with master now that #6716 has been merged and it's great to have access to the local address from server code, kudos @zasweq! However, there's also useful applications of retrieving the local port a client is using (e.g. to correlate it with prometheus metrics that we expose using the client address as a label). Not sure if this was intentional, but I noticed that #6702 exposed the LocalAddr in http2Client.getPeer (see here), while #6716 only updates http2_server.go.

Would it be possible to cherry-pick that one line from #6702? I modified the code locally and can confirm that's all that's needed for client code to retrieve local port through peer.FromContext(stream.Context()). I'd even be happy to contribute with a tiny MR if needed.

Thanks!

@zasweq
Copy link
Contributor

zasweq commented Nov 3, 2023

Oh sure, we cna keep this open and try to add that. I will update the title accordingly.

@zasweq zasweq changed the title add LocalAddr to peer.Peer add LocalAddr to peer.Peer client side Nov 3, 2023
@CarlosRDomin
Copy link
Contributor

Awesome thanks @zasweq! Will you take care of the changes or want me to help in any way?

@dfawley
Copy link
Member

dfawley commented Nov 7, 2023

@CarlosRDomin - if you don't mind sending a PR, that would be great, thanks!

@CarlosRDomin
Copy link
Contributor

Apologies for the delay, here it is @dfawley @zasweq #6779
It's my first PR to this repo so please let me know if I need to follow any special instructions, naming conventions, etc

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Hacktoberfest P2 Status: Help Wanted Type: Feature New features or improvements in behavior
Projects
None yet
7 participants