Fix not tracing in HttpClient v5 when HttpHost(arg[0]) is null but RoutingSupport#determineHost works. (#674)

Skywalking hc5 plugin worked the same as hc4 plugin: if the arg[0] is null, skip creating the exitSpan. this will cause a bug in hc5: when the HttpHost is null but InternalHttpClient determines the host from ClassicHttpRequest, InternalHttpClient will send the request but Skywalking will not record it.

Author: cylx3126

CHANGES.md

@@ -17,6 +17,7 @@ Release Notes.
 * Support for ActiveMQ-Artemis messaging tracing.
 * Archive the expired plugins `impala-jdbc-2.6.x-plugin`.
 * Fix a bug in Spring Cloud Gateway if HttpClientFinalizer#send does not invoke, the span created at NettyRoutingFilterInterceptor can not stop.
+* Fix not tracing in HttpClient v5 when HttpHost(arg[0]) is null but `RoutingSupport#determineHost` works.
 
 #### Documentation
 * Update docs to describe `expired-plugins`.

apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpClientDoExecuteInterceptor.java

@@ -38,19 +38,19 @@
 import java.net.MalformedURLException;
 import java.net.URL;
 
-public class HttpClientDoExecuteInterceptor implements InstanceMethodsAroundInterceptor {
+public abstract class HttpClientDoExecuteInterceptor implements InstanceMethodsAroundInterceptor {
     private static final String ERROR_URI = "/_blank";
 
     private static final ILog LOGGER = LogManager.getLogger(HttpClientDoExecuteInterceptor.class);
 
     @Override
     public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
             MethodInterceptResult result) throws Throwable {
-        if (allArguments[0] == null || allArguments[1] == null) {
+        if (skipIntercept(objInst, method, allArguments, argumentsTypes)) {
             // illegal args, can't trace. ignore.
             return;
         }
-        final HttpHost httpHost = (HttpHost) allArguments[0];
+        final HttpHost httpHost = getHttpHost(objInst, method, allArguments, argumentsTypes);
         ClassicHttpRequest httpRequest = (ClassicHttpRequest) allArguments[1];
         final ContextCarrier contextCarrier = new ContextCarrier();
 
@@ -75,10 +75,18 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
         }
     }
 
+    protected boolean skipIntercept(EnhancedInstance objInst, Method method, Object[] allArguments,
+                                    Class<?>[] argumentsTypes) {
+        return allArguments[1] == null || getHttpHost(objInst, method, allArguments, argumentsTypes) == null;
+    }
+
+    protected abstract HttpHost getHttpHost(EnhancedInstance objInst, Method method, Object[] allArguments,
+                                   Class<?>[] argumentsTypes) ;
+
     @Override
     public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
             Object ret) throws Throwable {
-        if (allArguments[0] == null || allArguments[1] == null) {
+        if (skipIntercept(objInst, method, allArguments, argumentsTypes)) {
             return ret;
         }
 
@@ -100,6 +108,9 @@ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allA
     @Override
     public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
             Class<?>[] argumentsTypes, Throwable t) {
+        if (skipIntercept(objInst, method, allArguments, argumentsTypes)) {
+            return;
+        }
         AbstractSpan activeSpan = ContextManager.activeSpan();
         activeSpan.log(t);
     }

apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/InternalClientDoExecuteInterceptor.java

@@ -0,0 +1,44 @@
+/*
+ * 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.httpclient.v5;
+
+import org.apache.hc.client5.http.routing.RoutingSupport;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+
+import java.lang.reflect.Method;
+
+public class InternalClientDoExecuteInterceptor extends HttpClientDoExecuteInterceptor {
+
+    @Override
+    protected HttpHost getHttpHost(EnhancedInstance objInst, Method method, Object[] allArguments,
+                                   Class<?>[] argumentsTypes) {
+        HttpHost httpHost = (HttpHost) allArguments[0];
+        if (httpHost != null) {
+            return httpHost;
+        }
+        try {
+            return RoutingSupport.determineHost((HttpRequest) allArguments[1]);
+        } catch (Exception ignore) {
+            // ignore
+            return null;
+        }
+    }
+}

apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/MinimalClientDoExecuteInterceptor.java

@@ -0,0 +1,33 @@
+/*
+ * 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.httpclient.v5;
+
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+
+import java.lang.reflect.Method;
+
+public class MinimalClientDoExecuteInterceptor extends HttpClientDoExecuteInterceptor {
+
+    @Override
+    protected HttpHost getHttpHost(EnhancedInstance objInst, Method method, Object[] allArguments,
+                                   Class<?>[] argumentsTypes) {
+        return (HttpHost) allArguments[0];
+    }
+}

apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/define/InternalHttpClientInstrumentation.java

@@ -0,0 +1,68 @@
+/*
+ * 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.httpclient.v5.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 org.apache.skywalking.apm.agent.core.plugin.match.MultiClassNameMatch;
+
+import static net.bytebuddy.matcher.ElementMatchers.named;
+
+public class InternalHttpClientInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
+
+    private static final String ENHANCE_CLASS_MINIMAL = "org.apache.hc.client5.http.impl.classic.InternalHttpClient";
+    private static final String METHOD_NAME = "doExecute";
+    private static final String INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.httpclient.v5.InternalClientDoExecuteInterceptor";
+
+    @Override
+    public ClassMatch enhanceClass() {
+        return MultiClassNameMatch.byMultiClassMatch(ENHANCE_CLASS_MINIMAL);
+    }
+
+    @Override
+    public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
+        return new ConstructorInterceptPoint[0];
+    }
+
+    @Override
+    public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
+        return new InstanceMethodsInterceptPoint[]{
+                new InstanceMethodsInterceptPoint() {
+                    @Override
+                    public ElementMatcher<MethodDescription> getMethodsMatcher() {
+                        return named(METHOD_NAME);
+                    }
+
+                    @Override
+                    public String getMethodsInterceptor() {
+                        return INTERCEPT_CLASS;
+                    }
+
+                    @Override
+                    public boolean isOverrideArgs() {
+                        return false;
+                    }
+                }
+        };
+    }
+}

…v5/define/HttpClientInstrumentation.java …ne/MinimalHttpClientInstrumentation.javaapm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/define/HttpClientInstrumentation.java renamed to apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/define/MinimalHttpClientInstrumentation.java apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/define/HttpClientInstrumentation.java renamed to apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/define/MinimalHttpClientInstrumentation.java

@@ -28,21 +28,20 @@
 
 import static net.bytebuddy.matcher.ElementMatchers.named;
 
-public class HttpClientInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
+public class MinimalHttpClientInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
 
     private static final String ENHANCE_CLASS_MINIMAL = "org.apache.hc.client5.http.impl.classic.MinimalHttpClient";
-    private static final String ENHANCE_CLASS_INTERNAL = "org.apache.hc.client5.http.impl.classic.InternalHttpClient";
     private static final String METHOD_NAME = "doExecute";
-    private static final String INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.httpclient.v5.HttpClientDoExecuteInterceptor";
+    private static final String INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.httpclient.v5.MinimalClientDoExecuteInterceptor";
 
     @Override
     public ClassMatch enhanceClass() {
-        return MultiClassNameMatch.byMultiClassMatch(ENHANCE_CLASS_MINIMAL, ENHANCE_CLASS_INTERNAL);
+        return MultiClassNameMatch.byMultiClassMatch(ENHANCE_CLASS_MINIMAL);
     }
 
     @Override
     public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
-        return null;
+        return new ConstructorInterceptPoint[0];
     }
 
     @Override

apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/resources/skywalking-plugin.def

@@ -14,6 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-httpclient-5.x=org.apache.skywalking.apm.plugin.httpclient.v5.define.HttpClientInstrumentation
+httpclient-5.x=org.apache.skywalking.apm.plugin.httpclient.v5.define.MinimalHttpClientInstrumentation
+httpclient-5.x=org.apache.skywalking.apm.plugin.httpclient.v5.define.InternalHttpClientInstrumentation
 httpclient-5.x=org.apache.skywalking.apm.plugin.httpclient.v5.define.HttpAsyncClientInstrumentation
 httpclient-5.x=org.apache.skywalking.apm.plugin.httpclient.v5.define.IOSessionImplInstrumentation