Split out gRPC library instrumentation. #1329
Conversation
anuraaga
requested review from
iNikem,
jkwatson,
mateuszrzeszutek,
trask and
tylerbenson
as code owners
| @@ -77,8 +85,6 @@ class GrpcTest extends AgentTestRunner { | |||
| "${SemanticAttributes.RPC_SYSTEM.key()}" "grpc" | |||
| "${SemanticAttributes.RPC_SERVICE.key()}" "example.Greeter" | |||
| "${SemanticAttributes.RPC_METHOD.key()}" "SayHello" | |||
| "${SemanticAttributes.NET_PEER_NAME.key()}" "localhost" | |||
There was a problem hiding this comment.
This is a regression, but I want to fix forward. Previously auto instrumentation got the address from the channel builder, but it seems weird to force that into library instrumentation too. gRPC does have a remote transport address attribute with some behavior difference between versions. Want to dig into that more as a followup instead of having to add address as a parameter to the interceptor's factories. How does that sound?
There was a problem hiding this comment.
No objection from me, let's create a tracking issue for this if @iNikem does not object.
|
@anuraaga The brave-grpc instrumentation crapped out for my simple use case. You might want to check if your impl works when the |
…-instr-java into separate-grpc-instrumentation
|
@asarkar Thanks for the note, we'll definitely make sure that use case works well. I'm currently refactoring OTel's context propagation from the ground up and gRPC context interop is a main goal of the changes so hopefully it helps you too. |
| implementation group: 'javax.annotation', name: 'javax.annotation-api', version: '1.3.2' | ||
|
|
||
| implementation deps.guava | ||
|
|
There was a problem hiding this comment.
are these dependencies needed?
| implementation group: 'javax.annotation', name: 'javax.annotation-api', version: '1.3.2' | |
| implementation deps.guava |
| } | ||
|
|
||
| dependencies { | ||
| compileOnly(project(':instrumentation:grpc-1.5:library')) |
There was a problem hiding this comment.
it's amazing to me that this does not create a cyclic dependency
the test that uses GrpcHelper does look a bit suspicious (using the same method to verify the status as was used to map the status), maybe better to add an extra spock data column to those test for otelStatus?
There was a problem hiding this comment.
Previously I was avoiding having to duplicate the statuscode mapping in the logic, its unit test, and here too. Now it's always one status though so no reason to use the helper :)
| @@ -77,8 +85,6 @@ class GrpcTest extends AgentTestRunner { | |||
| "${SemanticAttributes.RPC_SYSTEM.key()}" "grpc" | |||
| "${SemanticAttributes.RPC_SERVICE.key()}" "example.Greeter" | |||
| "${SemanticAttributes.RPC_METHOD.key()}" "SayHello" | |||
| "${SemanticAttributes.NET_PEER_NAME.key()}" "localhost" | |||
There was a problem hiding this comment.
No objection from me, let's create a tracking issue for this if @iNikem does not object.
…-instr-java into separate-grpc-instrumentation
For #1313