-
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
alts: Record network latency and pass it to the handshaker service. #6851
alts: Record network latency and pass it to the handshaker service. #6851
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6851 +/- ##
==========================================
+ Coverage 83.64% 83.76% +0.12%
==========================================
Files 286 286
Lines 30786 30792 +6
==========================================
+ Hits 25750 25793 +43
+ Misses 3977 3944 -33
+ Partials 1059 1055 -4
|
@matthewstevenson88: Any reason this is in draft? Is this good to be looked at? |
Sorry for the delayed reply, busy week. :) I just wanted a chance to fix up the vet-proto issues caused by me using an old version of the @easwars Would you be able to review? |
I can do a pass for Go readability related stuff. But I don't know enough to review for correctness etc. |
Thanks @easwars. Added @cesarghali to take a first look. |
@easwars Would you mind taking a quick pass for readability now? |
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 minor nits.
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.
Is this change covered under #6857?
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.
Good call, sync-ed this branch.
Thanks for the review @easwars! |
The corresponding field was added to the proto in grpc/grpc-proto#138.
@cesarghali Could you give me a quick gut check on whether this looks reasonable to you?
RELEASE NOTES: none