fix two problems in dubbo plugin (#2149)

* ensure that dubbo transaction will only be created at the provider side

* fix the problem that the apm headers will be covered with unexpected value, which will be reproduced when the application invoke a dubbo api in a dubbo transaction.

* update CHANGELOG.asciidoc for dubbo plugin bugfix

* use random test ports

* prevent few random port failures

* refactor a bit dubbo tests

Co-authored-by: Sylvain Juge <sylvain.juge@elastic.co>

Author: CoderLan0668

CHANGELOG.asciidoc

@@ -37,6 +37,9 @@ endif::[]
 * Fix slf4j-related `LinkageError` - {pull}2390[#2390]
 * Fix possible deadlock occurring when Byte Buddy reads System properties by warming up bytecode instrumentation code
 paths. The BCI warmup is on by default and may be disabled through the internal `warmup_byte_buddy` config option - {pull}2368[#2368]
+* Fixed few dubbo plugin issues - {pull}2149[#2149]
+** Dubbo transaction will should be created at the provider side
+** APM headers conversion issue within dubbo transaction
 
 [[release-notes-1.x]]
 === Java Agent version 1.x

apm-agent-core/src/test/java/co/elastic/apm/agent/testutils/TestPort.java

@@ -33,7 +33,7 @@ public class TestPort {
     /**
      * @return random available TCP port
      */
-    public static int getAvailableRandomPort() {
+    public static synchronized int getAvailableRandomPort() {
         int port;
         do {
             port = MIN_PORT + rand.nextInt(MAX_PORT - MIN_PORT + 1);
@@ -46,7 +46,7 @@ private static boolean isAvailablePort(int port) {
             ServerSocket serverSocket = ServerSocketFactory.getDefault().createServerSocket(port, 1, InetAddress.getByName("localhost"));
             serverSocket.close();
             return true;
-        } catch (Exception var3) {
+        } catch (Exception ignored) {
             return false;
         }
     }

apm-agent-plugins/apm-dubbo-plugin/src/main/java/co/elastic/apm/agent/dubbo/advice/AlibabaMonitorFilterAdvice.java

@@ -48,12 +48,12 @@ public static Object onEnterFilterInvoke(@Advice.Argument(1) Invocation invocati
             Span span = DubboTraceHelper.createConsumerSpan(tracer, invocation.getInvoker().getInterface(),
                 invocation.getMethodName(), context.getRemoteAddress());
             if (span != null) {
-                span.propagateTraceContext(invocation, AlibabaDubboTextMapPropagator.INSTANCE);
+                span.propagateTraceContext(context, AlibabaDubboTextMapPropagator.INSTANCE);
                 return span;
             }
-        } else if (active == null) {
+        } else if (context.isProviderSide() && active == null) {
             // for provider side
-            Transaction transaction = tracer.startChildTransaction(invocation, AlibabaDubboTextMapPropagator.INSTANCE, Invocation.class.getClassLoader());
+            Transaction transaction = tracer.startChildTransaction(context, AlibabaDubboTextMapPropagator.INSTANCE, Invocation.class.getClassLoader());
             if (transaction != null) {
                 transaction.activate();
                 DubboTraceHelper.fillTransaction(transaction, invocation.getInvoker().getInterface(), invocation.getMethodName());

apm-agent-plugins/apm-dubbo-plugin/src/main/java/co/elastic/apm/agent/dubbo/advice/ApacheMonitorFilterAdvice.java

@@ -49,12 +49,12 @@ public static Object onEnterFilterInvoke(@Advice.Argument(1) Invocation invocati
             Span span = DubboTraceHelper.createConsumerSpan(tracer, invocation.getInvoker().getInterface(),
                 invocation.getMethodName(), context.getRemoteAddress());
             if (span != null) {
-                span.propagateTraceContext(invocation, ApacheDubboTextMapPropagator.INSTANCE);
+                span.propagateTraceContext(context, ApacheDubboTextMapPropagator.INSTANCE);
                 return span;
             }
-        } else if (active == null) {
+        } else if (context.isProviderSide() && active == null) {
             // for provider side
-            Transaction transaction = tracer.startChildTransaction(invocation, ApacheDubboTextMapPropagator.INSTANCE, Invocation.class.getClassLoader());
+            Transaction transaction = tracer.startChildTransaction(context, ApacheDubboTextMapPropagator.INSTANCE, Invocation.class.getClassLoader());
             if (transaction != null) {
                 transaction.activate();
                 DubboTraceHelper.fillTransaction(transaction, invocation.getInvoker().getInterface(), invocation.getMethodName());

apm-agent-plugins/apm-dubbo-plugin/src/main/java/co/elastic/apm/agent/dubbo/helper/AlibabaDubboTextMapPropagator.java

@@ -20,27 +20,27 @@
 
 import co.elastic.apm.agent.impl.transaction.TextHeaderGetter;
 import co.elastic.apm.agent.impl.transaction.TextHeaderSetter;
-import com.alibaba.dubbo.rpc.Invocation;
+import com.alibaba.dubbo.rpc.RpcContext;
 
 import javax.annotation.Nullable;
 
-public enum AlibabaDubboTextMapPropagator implements TextHeaderGetter<Invocation>, TextHeaderSetter<Invocation> {
+public enum AlibabaDubboTextMapPropagator implements TextHeaderGetter<RpcContext>, TextHeaderSetter<RpcContext> {
 
     INSTANCE;
 
     @Nullable
     @Override
-    public String getFirstHeader(String headerName, Invocation invocation) {
-        return invocation.getAttachment(headerName);
+    public String getFirstHeader(String headerName, RpcContext rpcContext) {
+        return rpcContext.getAttachment(headerName);
     }
 
     @Override
-    public <S> void forEach(String headerName, Invocation invocation, S state, HeaderConsumer<String, S> consumer) {
-        consumer.accept(invocation.getAttachment(headerName), state);
+    public <S> void forEach(String headerName, RpcContext rpcContext, S state, HeaderConsumer<String, S> consumer) {
+        consumer.accept(rpcContext.getAttachment(headerName), state);
     }
 
     @Override
-    public void setHeader(String headerName, String headerValue, Invocation invocation) {
-        invocation.getAttachments().put(headerName, headerValue);
+    public void setHeader(String headerName, String headerValue, RpcContext rpcContext) {
+        rpcContext.setAttachment(headerName, headerValue);
     }
 }

apm-agent-plugins/apm-dubbo-plugin/src/main/java/co/elastic/apm/agent/dubbo/helper/ApacheDubboTextMapPropagator.java

@@ -20,27 +20,28 @@
 
 import co.elastic.apm.agent.impl.transaction.TextHeaderGetter;
 import co.elastic.apm.agent.impl.transaction.TextHeaderSetter;
-import org.apache.dubbo.rpc.Invocation;
+import org.apache.dubbo.rpc.RpcContext;
 
 import javax.annotation.Nullable;
 
-public enum ApacheDubboTextMapPropagator implements TextHeaderGetter<Invocation>, TextHeaderSetter<Invocation> {
+public enum ApacheDubboTextMapPropagator implements TextHeaderGetter<RpcContext>, TextHeaderSetter<RpcContext> {
 
     INSTANCE;
 
     @Nullable
     @Override
-    public String getFirstHeader(String headerName, Invocation invocation) {
-        return invocation.getAttachment(headerName);
+    public String getFirstHeader(String headerName, RpcContext rpcContext) {
+        return rpcContext.getAttachment(headerName);
     }
 
     @Override
-    public <S> void forEach(String headerName, Invocation invocation, S state, HeaderConsumer<String, S> consumer) {
-        consumer.accept(invocation.getAttachment(headerName), state);
+    public <S> void forEach(String headerName, RpcContext rpcContext, S state, HeaderConsumer<String, S> consumer) {
+        //consumer.accept(invocation.getAttachment(headerName), state);
+        consumer.accept(rpcContext.getAttachment(headerName), state);
     }
 
     @Override
-    public void setHeader(String headerName, String headerValue, Invocation invocation) {
-        invocation.setAttachment(headerName, headerValue);
+    public void setHeader(String headerName, String headerValue, RpcContext rpcContext) {
+        rpcContext.setAttachment(headerName, headerValue);
     }
 }

apm-agent-plugins/apm-dubbo-plugin/src/test/java/co/elastic/apm/agent/dubbo/AbstractDubboInstrumentationTest.java

@@ -24,22 +24,29 @@
 import co.elastic.apm.agent.dubbo.api.exception.BizException;
 import co.elastic.apm.agent.impl.context.Destination;
 import co.elastic.apm.agent.impl.error.ErrorCapture;
+import co.elastic.apm.agent.impl.transaction.AbstractSpan;
 import co.elastic.apm.agent.impl.transaction.Outcome;
 import co.elastic.apm.agent.impl.transaction.Span;
 import co.elastic.apm.agent.impl.transaction.Transaction;
+import co.elastic.apm.agent.testutils.TestPort;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import javax.annotation.Nullable;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.when;
 
 public abstract class AbstractDubboInstrumentationTest extends AbstractInstrumentationTest {
 
+    private final int port = TestPort.getAvailableRandomPort();
+    private final int anotherPort = TestPort.getAvailableRandomPort();
+
     @Nullable
     private DubboTestApi dubboTestApi;
 
@@ -171,5 +178,46 @@ public void validateDubboSpan(Span span, Class<?> apiClass, String methodName) {
             .isNotEqualTo(Outcome.UNKNOWN);
     }
 
-    abstract int getPort();
+    protected int getPort() {
+        return port;
+    }
+
+    protected int getAnotherApiPort() {
+        return anotherPort;
+    }
+
+    @Test
+    public void testBothProviderAndConsumer() {
+        DubboTestApi dubboTestApi = getDubboTestApi();
+        String arg = "hello, testBothProviderAndConsumer";
+        String ret = dubboTestApi.willInvokeAnotherApi(arg);
+        assertThat(ret).isEqualTo(arg);
+
+        assertThat(reporter.getFirstTransaction(5000)).isNotNull();
+        assertThat(reporter.getTransactions()).hasSize(2);
+        assertThat(reporter.getSpans()).hasSize(2);
+
+        Map<String, Transaction> transactionMap = buildMap(reporter.getTransactions());
+        Map<String, Span> spanMap = buildMap(reporter.getSpans());
+
+        String testApiName = "DubboTestApi#willInvokeAnotherApi";
+        String anotherApiName = "AnotherApi#echo";
+
+        assertThat(spanMap.get(testApiName).getTraceContext().getId().toString())
+            .isEqualTo(transactionMap.get(testApiName).getTraceContext().getParentId().toString());
+
+        assertThat(transactionMap.get(testApiName).getTraceContext().getId().toString())
+            .isEqualTo(spanMap.get(anotherApiName).getTraceContext().getParentId().toString());
+
+        assertThat(spanMap.get(anotherApiName).getTraceContext().getId().toString())
+            .isEqualTo(transactionMap.get(anotherApiName).getTraceContext().getParentId().toString());
+    }
+
+    public <T extends AbstractSpan<?>> Map<String, T> buildMap(List<T> list) {
+        Map<String, T> map = new HashMap<>();
+        for (T t : list) {
+            map.put(t.getNameAsString(), t);
+        }
+        return map;
+    }
 }

apm-agent-plugins/apm-dubbo-plugin/src/test/java/co/elastic/apm/agent/dubbo/AlibabaDubboInstrumentationTest.java

@@ -18,8 +18,10 @@
  */
 package co.elastic.apm.agent.dubbo;
 
+import co.elastic.apm.agent.dubbo.api.AnotherApi;
 import co.elastic.apm.agent.dubbo.api.DubboTestApi;
 import co.elastic.apm.agent.dubbo.api.exception.BizException;
+import co.elastic.apm.agent.dubbo.api.impl.AnotherApiImpl;
 import co.elastic.apm.agent.dubbo.api.impl.DubboTestApiImpl;
 import co.elastic.apm.agent.impl.error.ErrorCapture;
 import co.elastic.apm.agent.impl.transaction.Span;
@@ -42,41 +44,27 @@
 
 public class AlibabaDubboInstrumentationTest extends AbstractDubboInstrumentationTest {
 
-    private static ReferenceConfig<DubboTestApi> referenceConfig;
-
-    private static ServiceConfig<DubboTestApi> serviceConfig;
-
     @Override
     protected DubboTestApi buildDubboTestApi() {
-        ApplicationConfig providerAppConfig = new ApplicationConfig();
-        providerAppConfig.setName("dubbo-provider");
+        RegistryConfig registryConfig = createRegistryConfig();
+        ApplicationConfig appConfig = createApplicationConfig();
 
-        ProtocolConfig protocolConfig = new ProtocolConfig();
-        protocolConfig.setName("dubbo");
-        protocolConfig.setPort(getPort());
-        protocolConfig.setThreads(10);
+        //build AnotherApi provider
+        ProtocolConfig anotherApiProtocolConfig = createProtocolConfig(getAnotherApiPort());
+        createAndExportServiceConfig(registryConfig, AnotherApi.class, new AnotherApiImpl(), appConfig, anotherApiProtocolConfig);
 
-        RegistryConfig registryConfig = new RegistryConfig();
-        registryConfig.setAddress("N/A");
+        //build AnotherApi consumer
+        ReferenceConfig<AnotherApi> anotherApiReferenceConfig = createReferenceConfig(AnotherApi.class, appConfig, anotherApiProtocolConfig.getPort());
 
-        serviceConfig = new ServiceConfig<>();
-        serviceConfig.setApplication(providerAppConfig);
-        serviceConfig.setProtocol(protocolConfig);
-        serviceConfig.setInterface(DubboTestApi.class);
-        serviceConfig.setRef(new DubboTestApiImpl());
-        serviceConfig.setRegistry(registryConfig);
-        serviceConfig.export();
+        // build DubboTestApi provider
+        ProtocolConfig protocolConfig = createProtocolConfig(getPort());
+        createAndExportServiceConfig(registryConfig, DubboTestApi.class, new DubboTestApiImpl(anotherApiReferenceConfig.get()), appConfig, protocolConfig);
 
-        ApplicationConfig consumerApp = new ApplicationConfig();
-        consumerApp.setName("dubbo-consumer");
-        referenceConfig = new ReferenceConfig<>();
-        referenceConfig.setApplication(consumerApp);
-        referenceConfig.setInterface(DubboTestApi.class);
-        referenceConfig.setUrl("dubbo://localhost:" + getPort());
-        referenceConfig.setTimeout(3000);
+        // build DubboTestApi consumer
+        ReferenceConfig<DubboTestApi> testApiReferenceConfig = createReferenceConfig(DubboTestApi.class, appConfig, protocolConfig.getPort());
 
         List<MethodConfig> methodConfigList = new LinkedList<>();
-        referenceConfig.setMethods(methodConfigList);
+        testApiReferenceConfig.setMethods(methodConfigList);
         MethodConfig asyncConfig = new MethodConfig();
         asyncConfig.setName("async");
         asyncConfig.setAsync(true);
@@ -88,12 +76,51 @@ protected DubboTestApi buildDubboTestApi() {
         asyncNoReturnConfig.setReturn(false);
         methodConfigList.add(asyncNoReturnConfig);
 
-        return referenceConfig.get();
+        return testApiReferenceConfig.get();
     }
 
-    @Override
-    int getPort() {
-        return 20880;
+    private static RegistryConfig createRegistryConfig() {
+        RegistryConfig registryConfig = new RegistryConfig();
+        registryConfig.setAddress("N/A");
+        return registryConfig;
+    }
+
+    private static ApplicationConfig createApplicationConfig() {
+        ApplicationConfig appConfig = new ApplicationConfig();
+        appConfig.setName("all-in-one-app");
+        return appConfig;
+    }
+
+    private static ProtocolConfig createProtocolConfig(int port){
+        ProtocolConfig protocolConfig = new ProtocolConfig();
+        protocolConfig.setName("dubbo");
+        protocolConfig.setPort(port);
+        protocolConfig.setThreads(10);
+        return protocolConfig;
+    }
+
+    private static <T> void createAndExportServiceConfig(RegistryConfig registryConfig,
+                                                         Class<T> interfaceClass,
+                                                         T interfaceImpl,
+                                                         ApplicationConfig applicationConfig,
+                                                         ProtocolConfig protocolConfig) {
+
+        ServiceConfig<T> serviceConfig = new ServiceConfig<T>();
+        serviceConfig.setApplication(applicationConfig);
+        serviceConfig.setProtocol(protocolConfig);
+        serviceConfig.setInterface(interfaceClass);
+        serviceConfig.setRef(interfaceImpl);
+        serviceConfig.setRegistry(registryConfig);
+        serviceConfig.export();
+    }
+
+    private static <T> ReferenceConfig<T> createReferenceConfig(Class<T> interfaceClass, ApplicationConfig applicationConfig, int port) {
+        ReferenceConfig<T> referenceConfig = new ReferenceConfig<>();
+        referenceConfig.setApplication(applicationConfig);
+        referenceConfig.setInterface(interfaceClass);
+        referenceConfig.setUrl(String.format("dubbo://localhost:%d", port));
+        referenceConfig.setTimeout(3000);
+        return referenceConfig;
     }
 
     @Test