Adds parsers and handlers for Rpc abstraction and ports Dubbo and gRPC #999

codefromthecrypt · 2019-09-30T12:20:19Z

This adds basic infrastructure for uniform parsing of RPC libraries

By default, the following are added to both RPC client and server spans:

Span.name is the RPC service/method. Ex. "zipkin.proto3.SpanService/Report"
- If the service is absent, the method is the name and visa versa.
Tags:
- "rpc.method", eg "Report"
- "rpc.service", eg "zipkin.proto3.SpanService"
- "rpc.error_code", eg "CANCELLED"
- "error" the RPC error code if there is no exception
Remote IP and port information

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.

jeqo

LGTM so far

instrumentation/rpc/README.md

codefromthecrypt · 2019-10-01T11:12:29Z

#1000 pulls the most important parts out into a separate change

codefromthecrypt · 2019-10-02T10:06:16Z

@jeqo @anuraaga do you have some modeling thoughts? should we just add the following to the response types?

@Nullable String message()
@Nullable Boolean isError()

codefromthecrypt · 2019-10-02T10:06:52Z

instrumentation/rpc/README.md

+
+## 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"


NOTE: ignore this it is copy/paste and needs to be re-written

jeqo · 2019-10-02T11:48:09Z

maybe @Nullable String errorMessage() or similar would be better, as just message could be confusing

codefromthecrypt · 2019-10-02T12:05:51Z

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> .

codefromthecrypt · 2019-11-02T01:55:44Z

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/main/java/brave/rpc/RpcHandler.java

instrumentation/rpc/src/test/java/brave/rpc/ITRpcTracingClassLoader.java

See #999 (comment)

codefromthecrypt · 2019-11-03T00:43:21Z

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.

See #999 (comment)

codefromthecrypt · 2019-11-03T01:33:14Z

ok formally setting this down for someone else to pick up or delete.

NEXT STEPS:

implement dubbo, dubbo-rpc, grpc

codefromthecrypt · 2020-01-12T06:56:46Z

looks like this stopped, which is also blocking #914, which is probably blocking some other things..

codefromthecrypt · 2020-04-15T00:03:29Z

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?
@trytocatch I looked at this for dubbo.. seems mostly to define stats, not generic properties like status message or code. do you have any comments about properties most required for stats/tracing? https://github.com/apache/dubbo/blob/master/dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/RpcStatus.java

trytocatch · 2020-04-15T05:47:44Z

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?
@trytocatch I looked at this for dubbo.. seems mostly to define stats, not generic properties like status message or code. do you have any comments about properties most required for stats/tracing? https://github.com/apache/dubbo/blob/master/dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/RpcStatus.java

sorry, I just fixed a small concurrency problem there

codefromthecrypt · 2020-04-15T05:54:57Z

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. 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

codefromthecrypt · 2020-04-27T15:35:19Z

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!

codefromthecrypt · 2020-04-27T15:37:17Z

instrumentation/grpc/RATIONALE.md

@@ -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?


@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.

instrumentation/grpc/src/main/java/brave/grpc/GrpcParser.java

codefromthecrypt · 2020-04-27T15:39:59Z

instrumentation/grpc/src/test/java/brave/grpc/BaseITTracingClientInterceptor.java

@@ -509,6 +489,59 @@ void initMessageTaggingClient() {
    assertThat(reporter.takeRemoteSpan(Span.Kind.CLIENT).parentId()).isNull();
  }



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")

codefromthecrypt · 2020-04-28T02:13:56Z

rebased over a jax-rs test flake fix.

proceeding to add README content to finish this

codefromthecrypt · 2020-04-28T04:10:33Z

OK this is done now, imho

anuraaga

Small nits but looks good, thanks!

instrumentation/dubbo/src/main/java/brave/dubbo/TracingFilter.java

anuraaga · 2020-04-28T04:42:02Z

instrumentation/grpc/src/main/java/brave/grpc/GrpcClientResponse.java

+   *
+   * @since 5.12
+   */
+  @Override public Metadata headers() {


anuraaga · 2020-04-28T04:42:27Z

instrumentation/grpc/src/main/java/brave/grpc/GrpcRequest.java

+ */
+// NOTE: gRPC is Java 1.7+, so we cannot add methods to this later
+public interface GrpcRequest {
+  MethodDescriptor<?, ?> methodDescriptor();


Suggest just method()

agree. that's a lot better and now's the only time to change it!

ergh. now I remember. we define method() in RpcRequest already. I'll make a comment

instrumentation/grpc/src/main/java/brave/grpc/TracingClientInterceptor.java

codefromthecrypt · 2020-04-28T05:50:31Z

instrumentation/dubbo-rpc/src/main/java/brave/dubbo/rpc/TracingFilter.java

    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) {


before, this instanceof was also checking null. In practice, this is always a (dubbo) FutureAdapter

codefromthecrypt · 2020-04-28T06:15:08Z

@anuraaga you're the hero of this change! thanks for the weeks of review on this (and all the ancillary changes around it)

See openzipkin/brave#999 (comment)

codefromthecrypt requested review from anuraaga and jeqo September 30, 2019 12:20

jeqo reviewed Sep 30, 2019

View reviewed changes

instrumentation/rpc/README.md Outdated Show resolved Hide resolved

codefromthecrypt mentioned this pull request Oct 2, 2019

Adds basic infrastructure for RPC request sampling #1000

Merged

codefromthecrypt force-pushed the rpc-instrumentation branch from 7dde7df to 554e454 Compare October 2, 2019 10:04

codefromthecrypt changed the title ~~Adds abstraction layer for RPC~~ Adds basic parsers and handlers for RPC instrumentation Oct 2, 2019

codefromthecrypt commented Oct 2, 2019

View reviewed changes

codefromthecrypt force-pushed the rpc-instrumentation branch from 554e454 to 9aca0eb Compare November 2, 2019 01:35

codefromthecrypt added the help-wanted label Nov 2, 2019

codefromthecrypt mentioned this pull request Nov 2, 2019

Messaging instrumentation #914

Closed

devinsba reviewed Nov 2, 2019

View reviewed changes

instrumentation/rpc/src/main/java/brave/rpc/RpcHandler.java Outdated Show resolved Hide resolved

devinsba reviewed Nov 2, 2019

View reviewed changes

instrumentation/rpc/src/test/java/brave/rpc/ITRpcTracingClassLoader.java Show resolved Hide resolved

codefromthecrypt pushed a commit that referenced this pull request Nov 3, 2019

Makes all instrumentation call Span.finish in scope

bcafc80

See #999 (comment)

codefromthecrypt mentioned this pull request Nov 3, 2019

Makes all instrumentation call Span.finish in scope #1026

Merged

codefromthecrypt pushed a commit that referenced this pull request Nov 3, 2019

Makes all instrumentation call Span.finish in scope (#1026)

c956c40

See #999 (comment)

codefromthecrypt force-pushed the rpc-instrumentation branch from a165bb8 to 1dcb21a Compare November 3, 2019 01:33

codefromthecrypt mentioned this pull request Jan 28, 2020

Brave/Sleuth example? ExpediaDotCom/haystack-blob-example#5

Open

anuraaga mentioned this pull request Feb 24, 2020

Java Invocation correlation: Span.invocation(...) #1093

Open

codefromthecrypt mentioned this pull request Apr 15, 2020

First cut at a Zipkin Span Exporter open-telemetry/opentelemetry-java#1106

Merged

codefromthecrypt mentioned this pull request Apr 27, 2020

Renovates Dubbo instrumentation in preparation of standard parsing #1177

Merged

codefromthecrypt force-pushed the rpc-instrumentation branch from c5308b2 to 163d622 Compare April 27, 2020 15:33

codefromthecrypt changed the title ~~Adds basic parsers and handlers for RPC instrumentation~~ Adds parsers and handlers for Rpc abstraction and ports Dubbo and gRPC Apr 27, 2020

codefromthecrypt commented Apr 27, 2020

View reviewed changes

instrumentation/grpc/src/main/java/brave/grpc/GrpcParser.java Outdated Show resolved Hide resolved

codefromthecrypt commented Apr 27, 2020

View reviewed changes

codefromthecrypt force-pushed the rpc-instrumentation branch from 2014238 to 9a47aca Compare April 28, 2020 02:13

codefromthecrypt force-pushed the rpc-instrumentation branch from 9a47aca to edef7f0 Compare April 28, 2020 04:09

Adrian Cole added 3 commits April 28, 2020 12:09

Adds parsers and handlers for Rpc abstraction and ports Dubbo and gRPC

f3a6d98

Makes it possible to read commonly defined gRPC data

1959291

Finishes README files

edef7f0

codefromthecrypt removed the help-wanted label Apr 28, 2020

anuraaga approved these changes Apr 28, 2020

View reviewed changes

Adrian Cole added 3 commits April 28, 2020 12:55

polish

82cacf8

bad license actually

396a4be

be more clear about dubbo's future

376f0d6

codefromthecrypt commented Apr 28, 2020

View reviewed changes

codefromthecrypt merged commit aea76a8 into master Apr 28, 2020

codefromthecrypt deleted the rpc-instrumentation branch April 28, 2020 06:14

thgoexpt added a commit to thgoexpt/Java-distributed-tracing-implementation that referenced this pull request Apr 7, 2022

Makes all instrumentation call Span.finish in scope (#1026)

4270370

See openzipkin/brave#999 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds parsers and handlers for Rpc abstraction and ports Dubbo and gRPC #999

Adds parsers and handlers for Rpc abstraction and ports Dubbo and gRPC #999

codefromthecrypt commented Sep 30, 2019 •

edited

Loading

jeqo left a comment

codefromthecrypt commented Oct 1, 2019

codefromthecrypt commented Oct 2, 2019

codefromthecrypt Oct 2, 2019

jeqo commented Oct 2, 2019

codefromthecrypt commented Oct 2, 2019 via email

codefromthecrypt commented Nov 2, 2019

codefromthecrypt commented Nov 3, 2019

codefromthecrypt commented Nov 3, 2019

codefromthecrypt commented Jan 12, 2020

codefromthecrypt commented Apr 15, 2020

trytocatch commented Apr 15, 2020 •

edited

Loading

codefromthecrypt commented Apr 15, 2020

codefromthecrypt commented Apr 27, 2020

codefromthecrypt Apr 27, 2020

codefromthecrypt Apr 27, 2020

codefromthecrypt commented Apr 28, 2020

codefromthecrypt commented Apr 28, 2020

anuraaga left a comment

anuraaga Apr 28, 2020

anuraaga Apr 28, 2020

codefromthecrypt Apr 28, 2020

codefromthecrypt Apr 28, 2020

codefromthecrypt Apr 28, 2020

codefromthecrypt commented Apr 28, 2020

		@@ -509,6 +489,59 @@ void initMessageTaggingClient() {
		assertThat(reporter.takeRemoteSpan(Span.Kind.CLIENT).parentId()).isNull();
		}

Adds parsers and handlers for Rpc abstraction and ports Dubbo and gRPC #999

Adds parsers and handlers for Rpc abstraction and ports Dubbo and gRPC #999

Conversation

codefromthecrypt commented Sep 30, 2019 • edited Loading

jeqo left a comment

Choose a reason for hiding this comment

codefromthecrypt commented Oct 1, 2019

codefromthecrypt commented Oct 2, 2019

Choose a reason for hiding this comment

jeqo commented Oct 2, 2019

codefromthecrypt commented Oct 2, 2019 via email

codefromthecrypt commented Nov 2, 2019

codefromthecrypt commented Nov 3, 2019

codefromthecrypt commented Nov 3, 2019

codefromthecrypt commented Jan 12, 2020

codefromthecrypt commented Apr 15, 2020

trytocatch commented Apr 15, 2020 • edited Loading

codefromthecrypt commented Apr 15, 2020

codefromthecrypt commented Apr 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Apr 28, 2020

codefromthecrypt commented Apr 28, 2020

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Apr 28, 2020

codefromthecrypt commented Sep 30, 2019 •

edited

Loading

trytocatch commented Apr 15, 2020 •

edited

Loading