Intercept feign URL without parameters (#3854)

* Intercept feign RequestTemplate resolve args method ,get the url without parameters

* fix ci warnings

* optimize import

* fix: miss  path in feign target url like "http://localhost:8080/feign-scenario"
     fix feign-scenario test use pathVariable

* Add testcase to DefaultHttpClientInterceptorTest

* Clean ThreadLocal

* User one ThreadLocal replace two. User try/final  remove ThreadLocal .

* Code Optimize

apm-sniffer/apm-sdk-plugin/feign-default-http-9.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/feign/http/v9/DefaultHttpClientInterceptor.java

@@ -20,6 +20,17 @@ package org.apache.skywalking.apm.plugin.feign.http.v9;
 
 import feign.Request;
 import feign.Response;
+import org.apache.skywalking.apm.agent.core.context.CarrierItem;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.tag.Tags;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
@@ -30,16 +41,6 @@ import java.util.LinkedHashMap;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
-import org.apache.skywalking.apm.agent.core.context.CarrierItem;
-import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
-import org.apache.skywalking.apm.agent.core.context.ContextManager;
-import org.apache.skywalking.apm.agent.core.context.tag.Tags;
-import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
-import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
-import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
-import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
-import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
-import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
 
 /**
  * {@link DefaultHttpClientInterceptor} intercept the default implementation of http calls by the Feign.
@@ -48,27 +49,33 @@ import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInt
  */
 public class DefaultHttpClientInterceptor implements InstanceMethodsAroundInterceptor {
 
-    private static final String COMPONENT_NAME = "FeignDefaultHttp";
-
     /**
      * Get the {@link feign.Request} from {@link EnhancedInstance}, then create {@link AbstractSpan} and set host, port,
      * kind, component, url from {@link feign.Request}. Through the reflection of the way, set the http header of
      * context data into {@link feign.Request#headers}.
      *
-     * @param method
+     * @param method intercept method
      * @param result change this result, if you want to truncate the method.
-     * @throws Throwable
+     * @throws Throwable NoSuchFieldException or IllegalArgumentException
      */
-    @Override public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
-        Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
-        Request request = (Request)allArguments[0];
-
+    @Override
+    public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
+                             Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+        Request request = (Request) allArguments[0];
         URL url = new URL(request.url());
         ContextCarrier contextCarrier = new ContextCarrier();
         int port = url.getPort() == -1 ? 80 : url.getPort();
         String remotePeer = url.getHost() + ":" + port;
         String operationName = url.getPath();
-        if (operationName == null || operationName.length() == 0) {
+        FeignResolvedURL feignResolvedURL = PathVarInterceptor.URL_CONTEXT.get();
+        if (feignResolvedURL != null) {
+            try {
+                operationName = operationName.replace(feignResolvedURL.getUrl(), feignResolvedURL.getOriginUrl());
+            } finally {
+                PathVarInterceptor.URL_CONTEXT.remove();
+            }
+        }
+        if (operationName.length() == 0) {
             operationName = "/";
         }
         AbstractSpan span = ContextManager.createExitSpan(operationName, contextCarrier, remotePeer);
@@ -100,14 +107,14 @@ public class DefaultHttpClientInterceptor implements InstanceMethodsAroundInterc
      * Get the status code from {@link Response}, when status code greater than 400, it means there was some errors in
      * the server. Finish the {@link AbstractSpan}.
      *
-     * @param method
-     * @param ret the method's original return value.
-     * @return
-     * @throws Throwable
+     * @param method intercept method
+     * @param ret    the method's original return value.
+     * @return origin ret
      */
-    @Override public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
-        Class<?>[] argumentsTypes, Object ret) throws Throwable {
-        Response response = (Response)ret;
+    @Override
+    public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
+                              Class<?>[] argumentsTypes, Object ret) {
+        Response response = (Response) ret;
         if (response != null) {
             int statusCode = response.status();
 
@@ -123,8 +130,9 @@ public class DefaultHttpClientInterceptor implements InstanceMethodsAroundInterc
         return ret;
     }
 
-    @Override public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
-        Class<?>[] argumentsTypes, Throwable t) {
+    @Override
+    public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
+                                      Class<?>[] argumentsTypes, Throwable t) {
         AbstractSpan activeSpan = ContextManager.activeSpan();
         activeSpan.log(t);
         activeSpan.errorOccurred();

apm-sniffer/apm-sdk-plugin/feign-default-http-9.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/feign/http/v9/FeignResolvedURL.java

+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.skywalking.apm.plugin.feign.http.v9;
+
+/**
+ * class for {@link PathVarInterceptor} intercept feign url resolved params in url .
+ * @author qiyang
+ */
+public class FeignResolvedURL {
+    /**
+     * url before resolved
+     */
+    private String originUrl;
+    /**
+     * url after resolved
+     */
+    private String url;
+
+    public FeignResolvedURL(String originUrl) {
+        this.originUrl = originUrl;
+    }
+
+    public FeignResolvedURL(String originUrl, String url) {
+        this.originUrl = originUrl;
+        this.url = url;
+    }
+
+    public String getOriginUrl() {
+        return originUrl;
+    }
+
+    public void setOriginUrl(String originUrl) {
+        this.originUrl = originUrl;
+    }
+
+    public String getUrl() {
+        return url;
+    }
+
+    public void setUrl(String url) {
+        this.url = url;
+    }
+}

apm-sniffer/apm-sdk-plugin/feign-default-http-9.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/feign/http/v9/PathVarInterceptor.java

+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.feign.http.v9;
+
+import feign.RequestTemplate;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+/**
+ * {@link PathVarInterceptor} intercept the Feign RequestTemplate args resolve ;
+ *
+ * @author qiyang
+ */
+public class PathVarInterceptor implements InstanceMethodsAroundInterceptor {
+
+    static final ThreadLocal<FeignResolvedURL> URL_CONTEXT = new ThreadLocal<FeignResolvedURL>();
+
+    /**
+     * Get the {@link RequestTemplate#url()} before feign.ReflectiveFeign.BuildTemplateByResolvingArgs#resolve(Object[], RequestTemplate, Map)
+     *  put it into the {@link PathVarInterceptor#URL_CONTEXT}
+     *
+     * @param method intercept method
+     * @param result change this result, if you want to truncate the method.
+     */
+    @Override public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
+        Class<?>[] argumentsTypes, MethodInterceptResult result) {
+        RequestTemplate template = (RequestTemplate)allArguments[1];
+        URL_CONTEXT.set(new FeignResolvedURL(template.url()));
+    }
+
+    /**
+     * Get the resolved {@link RequestTemplate#url()} after feign.ReflectiveFeign.BuildTemplateByResolvingArgs#resolve(Object[], RequestTemplate, Map)
+     *  put it into the {@link PathVarInterceptor#URL_CONTEXT}
+     * @param method intercept method
+     * @param ret the method's original return value.
+     * @return result without change
+     */
+    @Override public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
+        Class<?>[] argumentsTypes, Object ret) {
+        RequestTemplate resolvedTemplate = (RequestTemplate) ret;
+        URL_CONTEXT.get().setUrl(resolvedTemplate.url());
+        return ret;
+    }
+
+    @Override public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
+        Class<?>[] argumentsTypes, Throwable t) {
+        if (URL_CONTEXT.get() != null) {
+            URL_CONTEXT.remove();
+        }
+    }
+}

apm-sniffer/apm-sdk-plugin/feign-default-http-9.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/feign/http/v9/define/PathVarInstrumentation.java

+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.feign.http.v9.define;
+
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine;
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static org.apache.skywalking.apm.agent.core.plugin.match.NameMatch.byName;
+
+public class PathVarInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
+
+    /**
+     * Enhance class.
+     */
+    private static final String ENHANCE_CLASS = "feign.ReflectiveFeign$BuildTemplateByResolvingArgs";
+
+    /**
+     * Intercept class.
+     */
+    private static final String INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.feign.http.v9.PathVarInterceptor";
+
+    @Override protected ClassMatch enhanceClass() {
+        return byName(ENHANCE_CLASS);
+    }
+
+    @Override public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
+        return new ConstructorInterceptPoint[0];
+    }
+
+    @Override public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
+        return new InstanceMethodsInterceptPoint[] {
+            new InstanceMethodsInterceptPoint() {
+                @Override public ElementMatcher<MethodDescription> getMethodsMatcher() {
+                    return named("resolve");
+                }
+
+                @Override public String getMethodsInterceptor() {
+                    return INTERCEPT_CLASS;
+                }
+
+                @Override public boolean isOverrideArgs() {
+                    return false;
+                }
+            }
+        };
+    }
+}

apm-sniffer/apm-sdk-plugin/feign-default-http-9.x-plugin/src/main/resources/skywalking-plugin.def

@@ -14,4 +14,5 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-feign-default-http-9.x=org.apache.skywalking.apm.plugin.feign.http.v9.define.DefaultHttpClientInstrumentation
\ No newline at end of file
+feign-default-http-9.x=org.apache.skywalking.apm.plugin.feign.http.v9.define.DefaultHttpClientInstrumentation
+feign-pathvar-9.x=org.apache.skywalking.apm.plugin.feign.http.v9.define.PathVarInstrumentation
\ No newline at end of file

apm-sniffer/apm-sdk-plugin/feign-default-http-9.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/feign/http/v9/DefaultHttpClientInterceptorTest.java

@@ -21,11 +21,6 @@ package org.apache.skywalking.apm.plugin.feign.http.v9;
 
 import feign.Request;
 import feign.Response;
-import java.nio.charset.Charset;
-import java.util.Collection;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
 import org.apache.skywalking.apm.agent.core.context.trace.AbstractTracingSpan;
 import org.apache.skywalking.apm.agent.core.context.trace.LogDataEntity;
 import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
@@ -40,6 +35,7 @@ import org.apache.skywalking.apm.agent.test.tools.SegmentStorage;
 import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint;
 import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner;
 import org.hamcrest.CoreMatchers;
+import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
@@ -50,6 +46,12 @@ import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 import org.powermock.modules.junit4.PowerMockRunnerDelegate;
 
+import java.nio.charset.Charset;
+import java.util.Collection;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
 import static junit.framework.TestCase.assertNotNull;
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertThat;
@@ -85,9 +87,9 @@ public class DefaultHttpClientInterceptorTest {
 
     @Before
     public void setUp() throws Exception {
-
+        PathVarInterceptor.URL_CONTEXT.set(new FeignResolvedURL("/test/{pathVar}","/test/var"));
         Map<String, Collection<String>> headers = new LinkedHashMap<String, Collection<String>>();
-        request = Request.create("GET", "http://skywalking.org/", headers, "Test".getBytes(), Charset.forName("UTF-8"));
+        request = Request.create("GET", "http://skywalking.org/test/var", headers, "Test".getBytes(), Charset.forName("UTF-8"));
         Request.Options options = new Request.Options();
         allArguments = new Object[] {request, options};
         argumentTypes = new Class[] {request.getClass(), options.getClass()};
@@ -112,7 +114,8 @@ public class DefaultHttpClientInterceptorTest {
         List<TagValuePair> tags = SpanHelper.getTags(finishedSpan);
         assertThat(tags.size(), is(2));
         assertThat(tags.get(0).getValue(), is("GET"));
-        assertThat(tags.get(1).getValue(), is("http://skywalking.org/"));
+        assertThat(tags.get(1).getValue(), is("http://skywalking.org/test/var"));
+        assertThat(finishedSpan.getOperationName(),is("/test/{pathVar}"));
 
         Assert.assertEquals(false, SpanHelper.getErrorOccurred(finishedSpan));
     }
@@ -135,8 +138,9 @@ public class DefaultHttpClientInterceptorTest {
         List<TagValuePair> tags = SpanHelper.getTags(finishedSpan);
         assertThat(tags.size(), is(3));
         assertThat(tags.get(0).getValue(), is("GET"));
-        assertThat(tags.get(1).getValue(), is("http://skywalking.org/"));
+        assertThat(tags.get(1).getValue(), is("http://skywalking.org/test/var"));
         assertThat(tags.get(2).getValue(), is("404"));
+        assertThat(finishedSpan.getOperationName(),is("/test/{pathVar}"));
 
         Assert.assertEquals(true, SpanHelper.getErrorOccurred(finishedSpan));
     }
@@ -166,7 +170,7 @@ public class DefaultHttpClientInterceptorTest {
         List<TagValuePair> tags = SpanHelper.getTags(finishedSpan);
         assertThat(tags.size(), is(2));
         assertThat(tags.get(0).getValue(), is("GET"));
-        assertThat(tags.get(1).getValue(), is("http://skywalking.org/"));
+        assertThat(tags.get(1).getValue(), is("http://skywalking.org/test/var"));
 
         Assert.assertEquals(true, SpanHelper.getErrorOccurred(finishedSpan));
 
@@ -179,4 +183,9 @@ public class DefaultHttpClientInterceptorTest {
         assertThat(logDataEntity.getLogs().get(2).getValue(), is("testException"));
         assertNotNull(logDataEntity.getLogs().get(3).getValue());
     }
+
+    @After
+    public void clean() {
+        PathVarInterceptor.URL_CONTEXT.remove();
+    }
 }

apm-sniffer/apm-sdk-plugin/feign-default-http-9.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/feign/http/v9/PathVarInterceptorTest.java

+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+
+package org.apache.skywalking.apm.plugin.feign.http.v9;
+
+import feign.RequestTemplate;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertThat;
+
+/**
+ * @author qiyang
+ */
+@RunWith(PowerMockRunner.class)
+public class PathVarInterceptorTest {
+
+    private PathVarInterceptor pathVarInterceptor;
+
+    @Mock
+    private EnhancedInstance enhancedInstance;
+
+    @Mock
+    private MethodInterceptResult result;
+
+    private Object[] allArguments;
+    private Class[] argumentTypes;
+
+    private RequestTemplate resolvedTemplate;
+
+    @Before
+    public void setUp() {
+
+        RequestTemplate template = new RequestTemplate();
+        template.append("http://skywalking.org/{pathVar}");
+
+        resolvedTemplate = new RequestTemplate();
+        resolvedTemplate.append("http://skywalking.org/value");
+
+        Map<String, Object> variables = new HashMap<String, Object>();
+        variables.put("pathVar","value");
+        allArguments = new Object[] {new Object[]{}, template,variables};
+        argumentTypes = new Class[] {Object[].class, RequestTemplate.class,Map.class};
+        pathVarInterceptor = new PathVarInterceptor();
+
+    }
+
+    @Test
+    public void testMethodsAround() throws Throwable {
+        pathVarInterceptor.beforeMethod(enhancedInstance,null,allArguments,argumentTypes,result);
+        pathVarInterceptor.afterMethod(enhancedInstance,null,allArguments,argumentTypes,resolvedTemplate);
+        assertThat(PathVarInterceptor.URL_CONTEXT.get().getOriginUrl(),is("http://skywalking.org/{pathVar}"));
+        assertThat(PathVarInterceptor.URL_CONTEXT.get().getUrl(),is("http://skywalking.org/value"));
+    }
+
+    @After
+    public void clean() {
+        PathVarInterceptor.URL_CONTEXT.remove();
+    }
+}

test/plugin/scenarios/feign-scenario/config/expectedData.yaml

@@ -136,7 +136,7 @@ segmentItems:
       tags:
       - {key: http.method, value: POST}
       - {key: url, value: 'http://localhost:8080/feign-scenario/create/'}
-    - operationName: /feign-scenario/get/1
+    - operationName: /feign-scenario/get/{id}
       operationId: 0
       parentSpanId: 0
       spanId: 2
@@ -152,7 +152,7 @@ segmentItems:
       tags:
       - {key: http.method, value: GET}
       - {key: url, value: 'http://localhost:8080/feign-scenario/get/1'}
-    - operationName: /feign-scenario/update/1
+    - operationName: /feign-scenario/update/{id}
       operationId: 0
       parentSpanId: 0
       spanId: 3
@@ -168,7 +168,7 @@ segmentItems:
       tags:
       - {key: http.method, value: PUT}
       - {key: url, value: 'http://localhost:8080/feign-scenario/update/1'}
-    - operationName: /feign-scenario/delete/1
+    - operationName: /feign-scenario/delete/{id}
       operationId: 0
       parentSpanId: 0
       spanId: 4