search

eclipse-vertx / vertx-grpc

Development of the gRPC component for Eclipse Vert.x
Eclipse Public License 2.0
37 stars 23 forks source link

Provide common attributes in gRPC bridge ServerCallImpl #90

Closed kureuil closed 4 months ago

kureuil commented 4 months ago

Despite the ServerCall::getAttributes() API being marked "experimental", it seems to be the established way of fetching the network addresses of the current connection. It is used in various examples found online and also in the server interceptor included in OpenTelemetry.

I didn't put the test in the ServerTest base class because it only concerns the GrpcBridge mode.

Conformance:

You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

vietj commented 4 months ago

thanks @kureuil , does this applies to client calls as well ?

kureuil commented 4 months ago

It didn't even cross my mind 🙃 After checking grpc-java's sources it would seem that these attributes are also set when doing client calls, I'll update this PR.

cf. https://github.com/grpc/grpc-java/blob/ac62c8b055cc415f7d8b17edad6abcb3ade548cf/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java#L563 cf. https://github.com/grpc/grpc-java/blob/ac62c8b055cc415f7d8b17edad6abcb3ade548cf/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java#L1011
kureuil commented 4 months ago

I tried to implement the change for the client calls, but it is not possible yet : there is no GrpcClientBridge that would allow one to use grpc-java ClientInterceptor which is the API exposing ClientCall.

wdyt @vietj ?

vietj commented 4 months ago

I have no idea yet :-)

kureuil commented 4 months ago

Having thought about this, I don't think the implementation for client calls should be in this PR. A GrpcClientBridge implementation would deserve its own PR, and I'd prefer to keep the scope of this one as reasonable as possible.

vietj commented 4 months ago

can you make a backport PR to 4.x branch @kureuil ?