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 GRPC client tracing interceptor for grpcs when cert is given #293

Merged
merged 6 commits into from
Nov 28, 2023

Conversation

Breina
Copy link
Contributor

@Breina Breina commented Nov 21, 2023

This case seems to have been mistakenly omitted? Why not trace this code path as well?

@Breina Breina requested a review from a team as a code owner November 21, 2023 13:25
@Breina
Copy link
Contributor Author

Breina commented Nov 24, 2023

Do you need me to create an Issue for this first, or how do we go about this?

Copy link
Member

@bestbeforetoday bestbeforetoday left a 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:

  1. 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.
  2. This change would make all the non-error code paths for the outer try/catch block call both .intercept(clientInterceptor) and addNettyBuilderProps(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.

@Breina
Copy link
Contributor Author

Breina commented Nov 24, 2023

Sure, I'm down to refactor it.

@Breina
Copy link
Contributor Author

Breina commented Nov 25, 2023

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]>
Copy link
Member

@bestbeforetoday bestbeforetoday left a 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

src/main/java/org/hyperledger/fabric/sdk/Endpoint.java Outdated Show resolved Hide resolved
src/main/java/org/hyperledger/fabric/sdk/Endpoint.java Outdated Show resolved Hide resolved
src/main/java/org/hyperledger/fabric/sdk/Endpoint.java Outdated Show resolved Hide resolved
src/main/java/org/hyperledger/fabric/sdk/Endpoint.java Outdated Show resolved Hide resolved
src/main/java/org/hyperledger/fabric/sdk/Endpoint.java Outdated Show resolved Hide resolved
src/main/java/org/hyperledger/fabric/sdk/Endpoint.java Outdated Show resolved Hide resolved
src/main/java/org/hyperledger/fabric/sdk/Endpoint.java Outdated Show resolved Hide resolved
src/main/java/org/hyperledger/fabric/sdk/Endpoint.java Outdated Show resolved Hide resolved
@Breina
Copy link
Contributor Author

Breina commented Nov 28, 2023

Thanks for the review! :)

Implemented all feedback.

Copy link
Member

@bestbeforetoday bestbeforetoday left a 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!

@bestbeforetoday bestbeforetoday merged commit b184586 into hyperledger:main Nov 28, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants