-
Notifications
You must be signed in to change notification settings - Fork 708
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 GRPC client tracing interceptor for grpcs when cert is given #293
Conversation
Signed-off-by: Breina <[email protected]>
Do you need me to create an Issue for this first, or how do we go about 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.
Although it's generally nice to have an associated issue, this looks like a small enough fix/change that I can live without one.
@atoulme Do you know any reason why the GrpcTelemetry client interceptor was not (or should not) be included in this code path?
Unless there is some reason not to add the interceptor for this code path, the change looks reasonable to me. Just two things that would be good to consider:
- Is it possible to unit test the Endpoint creation to check that an interceptor is correctly added. This would avoid anyone breaking it in the future.
- This change would make all the non-error code paths for the outer try/catch block call both
.intercept(clientInterceptor)
andaddNettyBuilderProps(channelBuilder, properties)
. Could the code be refactored so those calls happen at one common point rather than being duplicated in three code paths, which makes it easy for one to be missed.
Sure, I'm down to refactor it. |
Alright I refactored it. I looked into unit testing the interceptor, but it's inside netty's builder and makes it complex to get it out. I've tested locally instead. |
- Split into digestible chunks - Reduce cyclometer complexity - Improve code reuse Signed-off-by: Breina <[email protected]>
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.
Thank you for the effort you have put into this refactor. A few small suggestions inline, but generally nice. I like breaking sections of logic into more focused methods, and extracting constants and regex patterns into constants.
The key missing piece from this change is that there are still three points in the code where .intercept(clientInterceptor)
and addNettyBuilderProps(channelBuilder, properties)
are called. They should be consolidated to a single call of each, probably in the Endpoint constructor after calling the appropriate function to create the channelBuilder
Co-authored-by: Mark S. Lewis <[email protected]> Signed-off-by: Breina <[email protected]>
Co-authored-by: Mark S. Lewis <[email protected]> Signed-off-by: Breina <[email protected]>
Co-authored-by: Mark S. Lewis <[email protected]> Signed-off-by: Breina <[email protected]>
Thanks for the review! :) Implemented all feedback. |
Signed-off-by: Breina <[email protected]>
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.
Thank you for the contribution!
This case seems to have been mistakenly omitted? Why not trace this code path as well?