-
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
Adds parsers and handlers for Rpc abstraction and ports Dubbo and gRPC #999
Conversation
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.
LGTM so far
#1000 pulls the most important parts out into a separate change |
7dde7df
to
554e454
Compare
instrumentation/rpc/README.md
Outdated
|
||
## Span data policy | ||
By default, the following are added to both rpc client and server spans: | ||
* Span.name is the rpc method in lowercase: ex "zipkin.proto3.spanservice/report" |
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.
NOTE: ignore this it is copy/paste and needs to be re-written
maybe |
so we need a message (status) in case of success or fail. probably we need
examples... the main issue is we wont know which message is an error.
otoh maybe you are right we can just have two strings one for status and
one for error (which might be status also or empty string if we know
nothing except it is error)
…On Wed, Oct 2, 2019, 7:48 PM Jorge Quilcate Otoya ***@***.***> wrote:
maybe @nullable String errorMessage() or similar would be better, as just
message could be confusing
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#999?email_source=notifications&email_token=AAAPVV3K5LOUNFXDPVVOMOTQMSC7VA5CNFSM4I32D5B2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAEO2AY#issuecomment-537455875>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAPVVZTFE35XSEJEGDGXXTQMSC7VANCNFSM4I32D5BQ>
.
|
554e454
to
9aca0eb
Compare
ok I added error parsing. next steps on this are to integrate gRPC and dubbo. After this is merged, I'd recommend pulling out the brave.Response type and backporting http to implement it. After that, try again on the messaging abstraction! |
instrumentation/rpc/src/test/java/brave/rpc/ITRpcTracingClassLoader.java
Show resolved
Hide resolved
PS I plan to stop on this PR last friday :) I will hard stop on it tomorrow morning, so hopefully we can have a volunteer to carry forward after that. |
ok formally setting this down for someone else to pick up or delete. NEXT STEPS:
|
a165bb8
to
1dcb21a
Compare
looks like this stopped, which is also blocking #914, which is probably blocking some other things.. |
so this is from otel (possibly just lifting grpc stuff) https://github.com/open-telemetry/opentelemetry-java/blob/c24e4a963669cfd044d7478a801e8f170faba4fe/api/src/main/java/io/opentelemetry/trace/Status.java#L38 I think we do need a message, regardless of whether it is success or failure. I'm less sure about "code" as not sure that's entirely portable. @minwoox do you know between the various frameworks armeria supports (grpc, thrift etc) which properties are actually portable? |
sorry, I just fixed a small concurrency problem there |
thanks for the reply @trytocatch @beiwei30 do you have any commentary on RPC status/message portability from Dubbo? |
Similar to #999, HTTP should also be able to derive the error directly from `HttpResponse` vs a second argument. This also allows exceptions that nest status codes to become parsable into span tags.
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
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
) 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
This fixes some glitches in preparation of #999. These changes were already reviewed, just being raised separately to reduce the diff. * Restores old dubbo.error_code numeric value * 999 will use this unless initialized with RpcTracing * Sets callback context of alibaba dubbo client to invocation context. * Makes integration tests consistently use setTracing unless sampling * This helps classify legacy behavior vs new * General cleanups on alibaba dubbo Future code * Addition of types used for 999
…1177) This fixes some glitches in preparation of #999. These changes were already reviewed, just being raised separately to reduce the diff. * Restores old dubbo.error_code numeric value * 999 will use this unless initialized with RpcTracing * Sets callback context of alibaba dubbo client to invocation context. * Makes integration tests consistently use setTracing unless sampling * This helps classify legacy behavior vs new * General cleanups on alibaba dubbo Future code * Addition of types used for 999
c5308b2
to
163d622
Compare
ok this is back in business. I'm happy with the code I think and hope you are. @anuraaga has done a lot of work moving my hands. The commit log will betray this, so please thank him. tomorrow, I will polish up the readmes for shipit! |
@@ -25,3 +25,14 @@ record the application exception. | |||
In short, we choose to not mask the gRPC status with a potential application | |||
exception on close. In worst case, if there is no instrumentation for the layer | |||
that throws, its trace and span ID could be known via log correlation. | |||
|
|||
## Why don't we use RpcClientHandler or RpcServerHandler for unexpected errors? |
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.
@anuraaga since we now have a more compelling story about parsing, I agree that we should keep it nicer on the happy path vs the edge cases. This follows up your points on the other pull request I hope. Otherwise, let me know how to change it.
@@ -509,6 +489,59 @@ void initMessageTaggingClient() { | |||
assertThat(reporter.takeRemoteSpan(Span.Kind.CLIENT).parentId()).isNull(); | |||
} | |||
|
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.
NOTE: all ITs still pass, across dubbo and grpc. This means the data policy is exactly the same unless, someone initializes with RpcTracing, which will set the new policy from the rpc abstraction (ex "rpc.error_code")
2014238
to
9a47aca
Compare
rebased over a jax-rs test flake fix. proceeding to add README content to finish this |
9a47aca
to
edef7f0
Compare
OK this is done now, imho |
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.
Small nits but looks good, thanks!
instrumentation/dubbo/src/main/java/brave/dubbo/TracingFilter.java
Outdated
Show resolved
Hide resolved
* | ||
* @since 5.12 | ||
*/ | ||
@Override public Metadata headers() { |
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.
Thanks!
*/ | ||
// NOTE: gRPC is Java 1.7+, so we cannot add methods to this later | ||
public interface GrpcRequest { | ||
MethodDescriptor<?, ?> methodDescriptor(); |
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.
Suggest just method()
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.
agree. that's a lot better and now's the only time to change it!
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.
ergh. now I remember. we define method() in RpcRequest already. I'll make a comment
instrumentation/grpc/src/main/java/brave/grpc/TracingClientInterceptor.java
Outdated
Show resolved
Hide resolved
Scope scope = currentTraceContext.newScope(span.context()); | ||
Result result = null; | ||
Throwable error = null; | ||
try { | ||
result = invoker.invoke(invocation); | ||
error = result.getException(); | ||
Future<Object> future = rpcContext.getFuture(); // the case on async client invocation | ||
if (future instanceof FutureAdapter) { |
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.
before, this instanceof was also checking null. In practice, this is always a (dubbo) FutureAdapter
@anuraaga you're the hero of this change! thanks for the weeks of review on this (and all the ancillary changes around it) |
This adds basic infrastructure for uniform parsing of RPC libraries
By default, the following are added to both RPC client and server spans:
As we know propagation fields are not always headers, "getters and setter" methods used for propagation are protected, not public methods, and not named headers.