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.
This commit is contained in:
cylx3126 2024-03-12 23:12:34 +08:00 committed by GitHub
parent f227543fc3
commit 466f173f98
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 431 additions and 12 deletions

View File

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

View File

@ -38,7 +38,7 @@ import java.lang.reflect.Method;
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);
@ -46,11 +46,11 @@ public class HttpClientDoExecuteInterceptor implements InstanceMethodsAroundInte
@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 class HttpClientDoExecuteInterceptor implements InstanceMethodsAroundInte
}
}
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 class HttpClientDoExecuteInterceptor implements InstanceMethodsAroundInte
@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);
}

View File

@ -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;
}
}
}

View File

@ -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];
}
}

View File

@ -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;
}
}
};
}
}

View File

@ -28,21 +28,20 @@ import org.apache.skywalking.apm.agent.core.plugin.match.MultiClassNameMatch;
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

View File

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

View File

@ -0,0 +1,262 @@
/*
* 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.ClassicHttpRequest;
import org.apache.hc.core5.http.ClassicHttpResponse;
import org.apache.hc.core5.http.HttpHost;
import org.apache.skywalking.apm.agent.core.boot.ServiceManager;
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.TraceSegment;
import org.apache.skywalking.apm.agent.core.context.util.TagValuePair;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
import org.apache.skywalking.apm.agent.test.helper.SegmentHelper;
import org.apache.skywalking.apm.agent.test.helper.SpanHelper;
import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule;
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.Assert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnit;
import org.mockito.junit.MockitoRule;
import java.net.URI;
import java.util.List;
import static junit.framework.TestCase.assertNotNull;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
@RunWith(TracingSegmentRunner.class)
public class InternalHttpClientExecuteInterceptorTest {
@SegmentStoragePoint
private SegmentStorage segmentStorage;
@Rule
public AgentServiceRule agentServiceRule = new AgentServiceRule();
@Rule
public MockitoRule rule = MockitoJUnit.rule();
private HttpClientDoExecuteInterceptor httpClientDoExecuteInterceptor;
@Mock
private HttpHost httpHost;
@Mock
private ClassicHttpRequest request;
@Mock
private ClassicHttpResponse httpResponse;
private Object[] allArguments;
private Object[] allArgumentsWithNullHttpHost;
private Class[] argumentsType;
@Mock
private EnhancedInstance enhancedInstance;
@Before
public void setUp() throws Exception {
ServiceManager.INSTANCE.boot();
httpClientDoExecuteInterceptor = new InternalClientDoExecuteInterceptor();
when(httpResponse.getCode()).thenReturn(200);
when(httpHost.getHostName()).thenReturn("127.0.0.1");
when(httpHost.getSchemeName()).thenReturn("http");
when(request.getUri()).thenReturn(new URI("http://127.0.0.1:8080/test-web/test"));
when(request.getMethod()).thenReturn("GET");
when(httpHost.getPort()).thenReturn(8080);
allArguments = new Object[]{
httpHost,
request
};
allArgumentsWithNullHttpHost = new Object[]{
null,
request
};
argumentsType = new Class[]{
httpHost.getClass(),
request.getClass()
};
}
@Test
public void testHttpClient() throws Throwable {
httpClientDoExecuteInterceptor.beforeMethod(enhancedInstance, null, allArguments, argumentsType, null);
httpClientDoExecuteInterceptor.afterMethod(enhancedInstance, null, allArguments, argumentsType, httpResponse);
Assert.assertThat(segmentStorage.getTraceSegments().size(), is(1));
TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0);
List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegment);
assertHttpSpan(spans.get(0));
verify(request, times(3)).setHeader(anyString(), anyString());
}
@Test
public void testNullHttpHostHttpClient() throws Throwable {
httpClientDoExecuteInterceptor.beforeMethod(enhancedInstance, null, allArgumentsWithNullHttpHost, argumentsType, null);
httpClientDoExecuteInterceptor.afterMethod(enhancedInstance, null, allArgumentsWithNullHttpHost, argumentsType, httpResponse);
Assert.assertThat(segmentStorage.getTraceSegments().size(), is(1));
TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0);
List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegment);
assertHttpSpan(spans.get(0));
verify(request, times(3)).setHeader(anyString(), anyString());
}
@Test
public void testStatusCodeNotEquals200() throws Throwable {
when(httpResponse.getCode()).thenReturn(500);
httpClientDoExecuteInterceptor.beforeMethod(enhancedInstance, null, allArguments, argumentsType, null);
httpClientDoExecuteInterceptor.afterMethod(enhancedInstance, null, allArguments, argumentsType, httpResponse);
Assert.assertThat(segmentStorage.getTraceSegments().size(), is(1));
TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0);
List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegment);
assertThat(spans.size(), is(1));
List<TagValuePair> tags = SpanHelper.getTags(spans.get(0));
assertThat(tags.size(), is(3));
assertThat(tags.get(2).getValue(), is("500"));
assertHttpSpan(spans.get(0));
assertThat(SpanHelper.getErrorOccurred(spans.get(0)), is(true));
verify(request, times(3)).setHeader(anyString(), anyString());
}
@Test
public void testNullHttpHostStatusCodeNotEquals200() throws Throwable {
when(httpResponse.getCode()).thenReturn(500);
httpClientDoExecuteInterceptor.beforeMethod(enhancedInstance, null, allArgumentsWithNullHttpHost, argumentsType, null);
httpClientDoExecuteInterceptor.afterMethod(enhancedInstance, null, allArgumentsWithNullHttpHost, argumentsType, httpResponse);
Assert.assertThat(segmentStorage.getTraceSegments().size(), is(1));
TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0);
List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegment);
assertThat(spans.size(), is(1));
List<TagValuePair> tags = SpanHelper.getTags(spans.get(0));
assertThat(tags.size(), is(3));
assertThat(tags.get(2).getValue(), is("500"));
assertHttpSpan(spans.get(0));
assertThat(SpanHelper.getErrorOccurred(spans.get(0)), is(true));
verify(request, times(3)).setHeader(anyString(), anyString());
}
@Test
public void testHttpClientWithException() throws Throwable {
httpClientDoExecuteInterceptor.beforeMethod(enhancedInstance, null, allArguments, argumentsType, null);
httpClientDoExecuteInterceptor.handleMethodException(enhancedInstance, null, allArguments, argumentsType,
new RuntimeException("testException"));
httpClientDoExecuteInterceptor.afterMethod(enhancedInstance, null, allArguments, argumentsType, httpResponse);
Assert.assertThat(segmentStorage.getTraceSegments().size(), is(1));
TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0);
List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegment);
assertThat(spans.size(), is(1));
AbstractTracingSpan span = spans.get(0);
assertHttpSpan(span);
assertThat(SpanHelper.getErrorOccurred(span), is(true));
assertHttpSpanErrorLog(SpanHelper.getLogs(span));
verify(request, times(3)).setHeader(anyString(), anyString());
}
@Test
public void testNullHttpHostHttpClientWithException() throws Throwable {
httpClientDoExecuteInterceptor.beforeMethod(enhancedInstance, null, allArgumentsWithNullHttpHost, argumentsType, null);
httpClientDoExecuteInterceptor.handleMethodException(enhancedInstance, null, allArgumentsWithNullHttpHost, argumentsType,
new RuntimeException("testException"));
httpClientDoExecuteInterceptor.afterMethod(enhancedInstance, null, allArgumentsWithNullHttpHost, argumentsType, httpResponse);
Assert.assertThat(segmentStorage.getTraceSegments().size(), is(1));
TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0);
List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegment);
assertThat(spans.size(), is(1));
AbstractTracingSpan span = spans.get(0);
assertHttpSpan(span);
assertThat(SpanHelper.getErrorOccurred(span), is(true));
assertHttpSpanErrorLog(SpanHelper.getLogs(span));
verify(request, times(3)).setHeader(anyString(), anyString());
}
@Test
public void testUriNotProtocol() throws Throwable {
when(request.getUri()).thenReturn(new URI("/test-web/test"));
httpClientDoExecuteInterceptor.beforeMethod(enhancedInstance, null, allArguments, argumentsType, null);
httpClientDoExecuteInterceptor.afterMethod(enhancedInstance, null, allArguments, argumentsType, httpResponse);
Assert.assertThat(segmentStorage.getTraceSegments().size(), is(1));
TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0);
List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegment);
assertHttpSpan(spans.get(0));
verify(request, times(3)).setHeader(anyString(), anyString());
}
@Test
public void testNullHttpHostUriNotProtocol() throws Throwable {
when(request.getUri()).thenReturn(new URI("/test-web/test"));
httpClientDoExecuteInterceptor.beforeMethod(enhancedInstance, null, allArgumentsWithNullHttpHost, argumentsType, null);
httpClientDoExecuteInterceptor.afterMethod(enhancedInstance, null, allArgumentsWithNullHttpHost, argumentsType, httpResponse);
Assert.assertThat(segmentStorage.getTraceSegments().size(), is(0));
}
private void assertHttpSpanErrorLog(List<LogDataEntity> logs) {
assertThat(logs.size(), is(1));
LogDataEntity logData = logs.get(0);
Assert.assertThat(logData.getLogs().size(), is(4));
Assert.assertThat(logData.getLogs().get(0).getValue(), CoreMatchers.<Object>is("error"));
Assert.assertThat(logData.getLogs()
.get(1)
.getValue(), CoreMatchers.<Object>is(RuntimeException.class.getName()));
Assert.assertThat(logData.getLogs().get(2).getValue(), is("testException"));
assertNotNull(logData.getLogs().get(3).getValue());
}
private void assertHttpSpan(AbstractTracingSpan span) {
assertThat(span.getOperationName(), is("/test-web/test"));
assertThat(SpanHelper.getComponentId(span), is(2));
List<TagValuePair> tags = SpanHelper.getTags(span);
assertThat(tags.get(0).getValue(), is("http://127.0.0.1:8080/test-web/test"));
assertThat(tags.get(1).getValue(), is("GET"));
assertThat(span.isExit(), is(true));
}
}

View File

@ -53,7 +53,7 @@ import org.mockito.junit.MockitoJUnit;
import org.mockito.junit.MockitoRule;
@RunWith(TracingSegmentRunner.class)
public class HttpClientExecuteInterceptorTest {
public class MinimalHttpClientExecuteInterceptorTest {
@SegmentStoragePoint
private SegmentStorage segmentStorage;
@ -82,7 +82,7 @@ public class HttpClientExecuteInterceptorTest {
public void setUp() throws Exception {
ServiceManager.INSTANCE.boot();
httpClientDoExecuteInterceptor = new HttpClientDoExecuteInterceptor();
httpClientDoExecuteInterceptor = new MinimalClientDoExecuteInterceptor();
when(httpResponse.getCode()).thenReturn(200);
when(httpHost.getHostName()).thenReturn("127.0.0.1");