-
Notifications
You must be signed in to change notification settings - Fork 714
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
Renovates gRPC instrumentation in preparation of standard parsing #1175
Conversation
5433f3b
to
66dd94e
Compare
FYI continuous-integration/travis-ci/pr rarely passes when you change things in the pom. ex it fails in invoker tests due to drift between master and whatever is in the PR. I'm not sure what this is about, but seems to explain why we get winky (one red one green) builds on changes sometimes. |
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.
Left some comments about how we handle error spans. Since grpc-java stubs implement the lifecycle, which includes propagating errors into close
, I believe we don't have to do as much work as we're doing.
...ation/grpc/src/it/grpc12/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
Outdated
Show resolved
Hide resolved
instrumentation/grpc/src/main/java/brave/grpc/TracingClientInterceptor.java
Outdated
Show resolved
Hide resolved
instrumentation/grpc/src/main/java/brave/grpc/TracingServerInterceptor.java
Outdated
Show resolved
Hide resolved
instrumentation/grpc/src/main/java/brave/grpc/TracingClientInterceptor.java
Show resolved
Hide resolved
Thanks for the thoughtful consideration @anuraaga. This is much better than before |
04df23e
to
f08c093
Compare
force pushed as I have no other good ideas on how to get the mockito change to pass in travis. |
I'm going to special case the mockito depedendency as I cannot get travis to pass in invoker mode in any way |
f08c093
to
8920d3c
Compare
This fixes some glitches in preparation of #999 * Bumps gRPC to 1.29 (there was no impact) * GrpcClientRequest and GrpcServerRequest unwrapped `this`, not the call. * Anonymous GrpcClientParser and GrpcServerParser used ErrorParser unnecessarily * This is implicitly done in the Zipkin span handler * Status.cause was ignored * This led to a less precise "error" tag * Client headers were unnecessarily null during sampling. This was caused by too eager allocation of a span. * ClientCall.Listener was wrapped twice * Most ClientCall hooks were not placed in scope * ServerCall's span was started too late
force pushed again, with specific includes on the grpc invoker tests. At least locally, this doesn't use mockito at all now. |
I am removing the mockito flag @anuraaga. It is unsupportable and there are so many other things worth time than flailing on this
|
fingers crossed. |
This fixes some glitches in preparation of #999
this
, not the call.too eager allocation of a span.