add support 4 Apache Dubbo #2108

tydhot · 2021-01-25T09:57:45Z

support apache dubbo version: 2.7.0+
feature #1888

…try-java-instrumentation into issue#1888

tydhot · 2021-01-25T17:14:31Z

@anuraaga Is this what you said about start with library instrumentation?:)

anuraaga

Thanks a lot for the instrumentation!

anuraaga · 2021-01-26T05:36:28Z

...c/main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/client/DubboClientTracer.java

+
+  public Span startSpan(String name) {
+    SpanBuilder spanBuilder = tracer.spanBuilder(name).setSpanKind(CLIENT);
+    spanBuilder.setAttribute(SemanticAttributes.RPC_SYSTEM, "dubbo");


Separate from this PR it will be nice to have abstract String getRpcSystem() on RpcClientTracer to abstract this away.

Maybe it would be more appropriate to do it in another pr.

anuraaga · 2021-01-26T05:37:12Z

.../main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/client/DubboInjectAdapter.java

+import io.opentelemetry.context.propagation.TextMapPropagator;
+import org.apache.dubbo.rpc.RpcInvocation;
+
+public class DubboInjectAdapter implements TextMapPropagator.Setter<RpcInvocation> {


Suggested change

public class DubboInjectAdapter implements TextMapPropagator.Setter<RpcInvocation> {

class DubboInjectAdapter implements TextMapPropagator.Setter<RpcInvocation> {

...main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/client/TracingClientFilter.java

anuraaga · 2021-01-26T05:38:01Z

...main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/client/TracingClientFilter.java

+    this.tracer = new DubboClientTracer();
+  }
+
+  public static TracingClientFilter newFilter() {


How about a factory that accepts DubboClientTracer as a parameter too

anuraaga · 2021-01-26T05:38:31Z

...main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/client/TracingClientFilter.java

+
+  @Override
+  public Result invoke(Invoker<?> invoker, Invocation invocation) throws RpcException {
+    if (invocation instanceof RpcInvocation) {


Let's reduce indent

if (!(invocation instanceoc RpcInvocation)) { return invoker.invoke(invocation); }

anuraaga · 2021-01-26T05:42:34Z

...main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/client/TracingClientFilter.java

+  public Result invoke(Invoker<?> invoker, Invocation invocation) throws RpcException {
+    if (invocation instanceof RpcInvocation) {
+      String methodName = invocation.getMethodName();
+      Span span = tracer.startSpan(methodName);


Span name must be the full name

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/rpc.md#span-name

So I guess it is invoker.getInterface().getName() + '/' + invocation.getMethodName()

I guess we can add a utility in the helper for it for use in server side too

anuraaga · 2021-01-26T05:44:00Z

...ary/src/main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/common/DubboHelper.java

+
+  public static void prepareSpan(Span span, String methodName, Invoker<?> invoker) {
+    span.setAttribute(
+        SemanticAttributes.RPC_SERVICE, invoker.getInterface().getSimpleName() + ":" + methodName);


RPC_SERVICE is only the class name, so I guess invoker.getInterface().getName()

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/rpc.md#attributes

anuraaga · 2021-01-26T05:44:17Z

...ary/src/main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/common/DubboHelper.java

+  public static void prepareSpan(Span span, String methodName, Invoker<?> invoker) {
+    span.setAttribute(
+        SemanticAttributes.RPC_SERVICE, invoker.getInterface().getSimpleName() + ":" + methodName);
+    if (methodName != null) {


Can methodname be null?

anuraaga · 2021-01-26T05:45:01Z

...main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/server/TracingServerFilter.java

+import org.apache.dubbo.rpc.RpcException;
+import org.apache.dubbo.rpc.RpcInvocation;
+
+public class TracingServerFilter implements Filter {


Similar comments as client/side

anuraaga · 2021-01-26T05:46:36Z

...io/opentelemetry/javaagent/instrumentation/dubbo/apache/v2_7/DubboFilterInstrumentation.java

+            .and(takesArgument(1, String.class))
+            .and(takesArgument(2, String.class))
+            .and(isPublic()),
+        DubboFilterInstrumentation.class.getName() + "$AddFilterAdvice");


Does it work to use resource injection instead?

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/master/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java#L33

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/master/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInitializationInstrumentation.java#L23

In dubbo, I found that importing resources directly does not work.It should be declared or configured in advance.Maybe code here it's easier to understand now。

Can you describe more what the issue is? This injection happens on classload, so it's as early as it needs to be I think. For the type matcher in DubboInitializationInstrumentation, I think you can just use ExtensionLoader.

Looking at this version of the instrumentation, I see we instrument one of these similarly named methods

https://github.com/apache/dubbo/blob/master/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java#L246

It's unclear whether this is complete, or if it will need maintenance in the future. We can be more confident if we use the official mechanism though, which I guess is the resource file?

…y-java-instrumentation into issue#1888

anuraaga · 2021-01-27T03:07:21Z

...io/opentelemetry/javaagent/instrumentation/dubbo/apache/v2_7/DubboFilterInstrumentation.java

+            .and(takesArgument(1, String.class))
+            .and(takesArgument(2, String.class))
+            .and(isPublic()),
+        DubboFilterInstrumentation.class.getName() + "$AddFilterAdvice");


Can you describe more what the issue is? This injection happens on classload, so it's as early as it needs to be I think. For the type matcher in DubboInitializationInstrumentation, I think you can just use ExtensionLoader.

Looking at this version of the instrumentation, I see we instrument one of these similarly named methods

https://github.com/apache/dubbo/blob/master/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java#L246

It's unclear whether this is complete, or if it will need maintenance in the future. We can be more confident if we use the official mechanism though, which I guess is the resource file?

anuraaga · 2021-01-27T03:08:25Z

...c/main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/client/DubboClientTracer.java

+    return span;
+  }
+
+  public Context withClient(Span span) {


I don't think we need this trivial helper, and the name is confusing I guess

anuraaga · 2021-01-27T03:09:47Z

...ary/src/main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/common/DubboHelper.java

+
+  private DubboHelper() {}
+
+  public static void prepareSpan(Span span, String methodName, String interfaceName) {


Order it interfaceName, then methodName which is more common for class+method

anuraaga · 2021-01-27T03:11:20Z

...avaagent/src/test/groovy/io/opentelemetry/instrumentation/dubbo/apache/v2_7/DubboTest.groovy

+
+class DubboTest extends AbstractDubboTest implements AgentTestTrait {
+  @Override
+  ServiceConfig configureServer() {


We generally have the tests differ only in the way they are instrumenting. Is it possible to have the registry / service definition happen in the base class, only difference is non-agent version calls addFilter if it's required?

yes,I will fix it.

I've made the unit test code cleaner

anuraaga · 2021-01-27T03:12:11Z

.../library/src/test/groovy/io/opentelemetry/instrumentation/dubbo/apache/v2_7/DubboTest.groovy

+    service.setInterface(HelloService)
+    service.setRef(new HelloServiceImpl())
+    service.setRegistry(registerConfig)
+    service.setFilter("OtelServerFilter")


Is setFilter required even though we have the resource files?

I found that if I add the Activate annotation at the beginning of a class it will be loaded automatically, and it is worked well in library.But maybe Dubbo's spi is more complicated than I thought, if I inject the resource file directly, it will be something wrong in javaagent's unit test.It will be the exception like failOnContextLeak I've spent a lot of time checking, but I don't have a clue.It seems that the process of Dubbo loading the server filter is different from that of the client filter.I have changed the code in javaagent in order to make it work by Activate annotation, and it can avoid the exception failOnContextLeak now in javaagent's unit test.

anuraaga · 2021-01-27T03:14:13Z

.../src/main/groovy/io/opentelemetry/instrumentation/dubbo/apache/v2_7/AbstractDubboTest.groovy

+      trace(0, 3) {
+        basicSpan(it, 0, "parent")
+        span(1) {
+          name "org.apache.dubbo.rpc.service.GenericService/\$invoke"


I would generally expect the client service / method to match the server one. In dubbo, is it not possible for the client to have the name HelloService/hello instead of this generic name? I see setInterface and the method name hello in the invocation, so it seems like the client has all the information.

Dubbo will use javaassist to set up a proxy for the interface to call directly instead of generic invoke, but I find that the proxy cannot work in groovy's unit test.So I use generic invoke here instead.

I see - we'll need to add a smoke-test to verify the behavior in a future PR then. Can you add a comment here?

I'd love to do it

Ah sorry I meant a comment in the code :P // Dubbo's codegen does not work properly from Groovy tests, so we need to verify in a smoke test instead.

Can you go ahead and add this code comment suggestion?

…y-java-instrumentation into issue#1888

tydhot · 2021-01-27T11:38:35Z

Hi @anuraaga !The library module I had refer to the implementation of Zipkin and rewrite it.Now the code is cleaner and additional support for asynchronous future is added.In library, the filter will be Autoload.
And I tried to inject the resource file in javaagent,the test case will be an exception like context leak.So I decided to remove the filter's autoload and add it when method back instead in javaagent module.It seemed work and the code is cleaner than before. Plz review it:)!

tydhot · 2021-01-28T03:21:43Z

@anuraaga I have changed the way and now the javaagent module is liked aws's javaagent module.I think It is more confident to do like it so.

…y-java-instrumentation into issue#1888

anuraaga · 2021-02-03T09:13:37Z

@tydhot Sorry for the delay in checking this out - I found a problem with our tooling with a fix in #2179.

Now there is a different issue where the agent doesn't inject the references from the dubbo file since it's not a standard formatted SPI file. I think we can tweak our mechanism for resolving from SPI so instrumentation can customize the parsing technique. We basically need a way for

https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/2108/files#diff-92953523ffff3580ebb3ff4d0a1b1276d3aacfa375416f2a36e070117022bacfR1 to be parsed by

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollector.java#L60

@mateuszrzeszutek what do you think of this? Another simpler option, that loses the complete automation, is to have MuzzleCodeGenerator also pass additionalHelperClasses to the reference collector. Manually defining one helper class would be no problem here.

…y-java-instrumentation into issue#1888

…nstrumentation into issue#1888

…trumentation into issue#1888

anuraaga · 2021-02-04T08:22:41Z

instrumentation/dubbo/dubbo-apache-2.7/javaagent/dubbo-apache-2.7-javaagent.gradle

+dependencies {
+  implementation project(':instrumentation:dubbo:dubbo-apache-2.7:library')
+
+  library group: 'org.apache.dubbo', name: 'dubbo', version: '2.7.0'


We target tests to oldest version supported

Before Dubbo 2.7.0, it is called alibaba Dubbo.And it is called Apache Dubbo since 2.7.0.

anuraaga · 2021-02-04T08:22:59Z

...io/opentelemetry/javaagent/instrumentation/dubbo/apache/v2_7/DubboInstrumentationModule.java

+  @Override
+  public String[] helperResourceNames() {
+    return new String[] {
+      "META-INF/services/org.apache.dubbo.rpc.Filter",


I found /services is handled by dubbo too, so this works with our muzzle

anuraaga · 2021-02-04T08:23:21Z

.../dubbo-apache-2.7/javaagent/src/main/resources/META-INF/services/org.apache.dubbo.rpc.Filter

@@ -0,0 +1 @@
+io.opentelemetry.javaagent.shaded.instrumentation.dubbo.apache.v2_7.OpenTelemetryFilter


I found it automatically transfers the classname to a name if not provided, which works well for us and ensures working with Muzzle

https://github.com/apache/dubbo/blob/master/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java#L1011

anuraaga · 2021-02-04T08:23:45Z

...er/src/main/java/io/opentelemetry/javaagent/testing/exporter/OtlpInMemoryMetricExporter.java

@@ -69,9 +69,6 @@ void reset() {

  @Override
  public CompletableResultCode export(Collection<MetricData> metrics) {
-    for (MetricData metric : metrics) {


Really spammy - let's restore in some form if metrics gets more stable

anuraaga · 2021-02-04T08:24:09Z

Did some debugging and got something that seems to work, almost there

tydhot · 2021-02-04T08:27:47Z

@anuraaga Yes, I founded that the classname in resource's name had to be renamed by shaded just before. It took me a lot of time to debug it...

anuraaga

Few hopefully easy comments left, thanks for the patience

anuraaga · 2021-02-04T08:24:57Z

instrumentation/dubbo/dubbo-apache-2.7/javaagent/dubbo-apache-2.7-javaagent.gradle

@@ -0,0 +1,17 @@
+apply from: "$rootDir/gradle/instrumentation.gradle"


Can we name the folders either apache-dubbo/apache-dubbo-2.7 for consistency with other apache projects, or just dubbo/dubbo-2.7? I guess dubbo is a unique enough name that apache may not be needed but whatever seems more familiar to users I guess

In China, Alibaba's Dubbo still has a certain number of users

Ah got it - then yeah let's use apache-dubbo/apache-dubbo-2.7 (currently it is dubbo-apache-2.7)

Maybe it would be more cautious to separate Alibaba Dubbo and Apache Dubbo

Ok I got it

anuraaga · 2021-02-04T08:25:28Z

....7/library/src/main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/DubboHelper.java

+
+  private DubboHelper() {}
+
+  public static void prepareSpan(Span span, String interfaceName, String methodName) {


Methods can be package private

anuraaga · 2021-02-04T08:26:32Z

....7/library/src/main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/DubboTracer.java

+            .spanBuilder(DubboHelper.getSpanName(interfaceName, methodName))
+            .setSpanKind(SERVER)
+            .setParent(
+                GlobalOpenTelemetry.getPropagators()


How about trying this again, it should work now. The issue previously was we applied the filter twice, once found in library instrumentation (hence needing #2179) and once in agent instrumentation.

anuraaga · 2021-02-04T08:28:24Z

...ry/src/main/java/io/opentelemetry/instrumentation/dubbo/apache/v2_7/OpenTelemetryFilter.java

+      context = tracer.startClientSpan(interfaceName, methodName);
+      GlobalOpenTelemetry.getPropagators()
+          .getTextMapPropagator()
+          .inject(context, (RpcInvocation) invocation, SETTER);


Not for this PR, nor necessarily you, but inject needs to be moved from HttpClientTracer to BaseTracer

anuraaga · 2021-02-04T08:28:56Z

.../src/main/groovy/io/opentelemetry/instrumentation/dubbo/apache/v2_7/AbstractDubboTest.groovy

+      trace(0, 3) {
+        basicSpan(it, 0, "parent")
+        span(1) {
+          name "org.apache.dubbo.rpc.service.GenericService/\$invoke"


Can you go ahead and add this code comment suggestion?

anuraaga

Thanks!

tydhot · 2021-02-04T09:50:43Z

@anuraaga hah, it was a challenging and interesting development experience.

anuraaga · 2021-02-04T10:25:22Z

@tydhot So sorry my comment said "Thanks?" that was a typo for "Thanks!". On phone's Japanese keyboard, ? is too close to ! :(

tydhot · 2021-02-04T10:34:56Z

@anuraaga ah, I got it immediately. BTW, I have translated doc in this project 70 percent , it help me a lot :)! Espaclly the doc about class loader . open-telemetry/docs-cn#49

trask

great work @tydhot!

...io/opentelemetry/javaagent/instrumentation/apache/dubbo/v2_7/DubboInstrumentationModule.java

.../apache-dubbo-2.7/javaagent/src/main/resources/META-INF/services/org.apache.dubbo.rpc.Filter

...ry/src/main/java/io/opentelemetry/instrumentation/apache/dubbo/v2_7/DubboExtractAdapter.java

....7/library/src/main/java/io/opentelemetry/instrumentation/apache/dubbo/v2_7/DubboTracer.java

…y-java-instrumentation into issue#1888

trask · 2021-02-07T05:56:32Z

thanks again @tydhot!

tydhot · 2021-02-07T09:04:47Z

@trask Well, I hope this is a good start for the Chinese community :)

tydhot added 3 commits January 25, 2021 01:31

add apache dubbo library

c7c784b

add apache dubbo javaagent

9a90fbf

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

3d738db

…try-java-instrumentation into issue#1888

tydhot requested review from anuraaga, iNikem, jkwatson, mateuszrzeszutek, pavolloffay, trask and tylerbenson as code owners January 25, 2021 09:57

tydhot added 5 commits January 25, 2021 18:28

polish the test case

5a25a46

polish groovy

8258f5f

polish groovy

80bc67c

codenarcTest

fc01763

codenarcMain

115ad75

anuraaga reviewed Jan 26, 2021

View reviewed changes

Base automatically changed from master to main January 26, 2021 05:50

tydhot added 2 commits January 26, 2021 14:23

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

42f60b8

…y-java-instrumentation into issue#1888

code polish

f385a89

tydhot requested a review from anuraaga January 27, 2021 02:47

anuraaga reviewed Jan 27, 2021

View reviewed changes

tydhot added 4 commits January 27, 2021 14:50

code polish

34af183

code polish

b34dc5f

rewrite dubbo

78c92bf

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

54c4ebe

…y-java-instrumentation into issue#1888

tydhot requested a review from anuraaga January 27, 2021 11:39

rewrite dubbo

9b0d197

tydhot added 3 commits February 2, 2021 11:26

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

7814cec

…y-java-instrumentation into issue#1888

change test

6c69ea0

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

49eef86

…y-java-instrumentation into issue#1888

tydhot and others added 6 commits February 4, 2021 10:41

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

12137e5

…y-java-instrumentation into issue#1888

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

80ea7dd

…y-java-instrumentation into issue#1888

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

6cae27c

…nstrumentation into issue#1888

Finish

1f8af9a

Older dubbo

d31190e

Merge branch 'issue#1888' of github.com:tydhot/opentelemetry-java-ins…

70788d2

…trumentation into issue#1888

anuraaga reviewed Feb 4, 2021

View reviewed changes

Merge remote-tracking branch 'origin/issue#1888' into issue#1888

2c958e1

anuraaga reviewed Feb 4, 2021

View reviewed changes

dubbo module name change

b71b4de

anuraaga approved these changes Feb 4, 2021

View reviewed changes

trask approved these changes Feb 6, 2021

View reviewed changes

tydhot added 5 commits February 6, 2021 23:10

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

1f43388

…y-java-instrumentation into issue#1888

change package name 'apache.dubbo' to 'apachedubbo'

296ead6

remove the resource in agent

ef67173

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

07ebc88

…y-java-instrumentation into issue#1888

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

a53afc7

…y-java-instrumentation into issue#1888

trask merged commit a6527c2 into open-telemetry:main Feb 7, 2021

mateuszrzeszutek mentioned this pull request Mar 19, 2021

Add support for apache dubbo. #1888

Closed

	public class DubboInjectAdapter implements TextMapPropagator.Setter<RpcInvocation> {
	class DubboInjectAdapter implements TextMapPropagator.Setter<RpcInvocation> {


		private DubboHelper() {}

		public static void prepareSpan(Span span, String methodName, String interfaceName) {

		@@ -0,0 +1 @@
		io.opentelemetry.javaagent.shaded.instrumentation.dubbo.apache.v2_7.OpenTelemetryFilter

		@@ -0,0 +1,17 @@
		apply from: "$rootDir/gradle/instrumentation.gradle"

add support 4 Apache Dubbo #2108

add support 4 Apache Dubbo #2108

Conversation

tydhot commented Jan 25, 2021 • edited

tydhot commented Jan 25, 2021

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

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

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

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

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

tydhot commented Jan 27, 2021

tydhot commented Jan 28, 2021

anuraaga commented Feb 3, 2021 • edited

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

anuraaga commented Feb 4, 2021

tydhot commented Feb 4, 2021

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

anuraaga Feb 4, 2021 • edited

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

Choose a reason for hiding this comment

anuraaga left a comment • edited

Choose a reason for hiding this comment

tydhot commented Feb 4, 2021

anuraaga commented Feb 4, 2021

tydhot commented Feb 4, 2021

trask left a comment

Choose a reason for hiding this comment

trask commented Feb 7, 2021

tydhot commented Feb 7, 2021

tydhot commented Jan 25, 2021 •

edited

anuraaga commented Feb 3, 2021 •

edited

anuraaga Feb 4, 2021 •

edited

anuraaga left a comment •

edited