diff --git a/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/client/TraceWebClientAutoConfiguration.java b/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/client/TraceWebClientAutoConfiguration.java index 46bc2f589..3e7d85c46 100644 --- a/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/client/TraceWebClientAutoConfiguration.java +++ b/spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/client/TraceWebClientAutoConfiguration.java @@ -26,6 +26,7 @@ import org.springframework.boot.autoconfigure.AutoConfigureBefore; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.autoconfigure.security.oauth2.resource.UserInfoRestTemplateCustomizer; import org.springframework.boot.web.client.RestTemplateCustomizer; @@ -104,6 +105,7 @@ class TraceWebClientAutoConfiguration { @Configuration(proxyBeanMethods = false) @ConditionalOnClass(HttpHeadersFilter.class) + @ConditionalOnMissingClass("reactor.netty.http.client.HttpClient") static class HttpHeadersFilterConfig { @Bean diff --git a/spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/client/GatewayAutoConfigurationTests.java b/spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/client/GatewayAutoConfigurationTests.java new file mode 100644 index 000000000..94711c59f --- /dev/null +++ b/spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/client/GatewayAutoConfigurationTests.java @@ -0,0 +1,50 @@ +/* + * Copyright 2013-2021 the original author or authors. + * + * Licensed 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 + * + * https://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.springframework.cloud.sleuth.autoconfig.instrument.web.client; + +import org.junit.jupiter.api.Test; +import reactor.netty.http.client.HttpClient; + +import org.springframework.boot.autoconfigure.AutoConfigurations; +import org.springframework.boot.test.context.FilteredClassLoader; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.cloud.gateway.filter.headers.HttpHeadersFilter; +import org.springframework.cloud.sleuth.autoconfig.TraceNoOpAutoConfiguration; +import org.springframework.cloud.sleuth.instrument.web.client.TraceRequestHttpHeadersFilter; +import org.springframework.cloud.sleuth.instrument.web.client.TraceResponseHttpHeadersFilter; + +import static org.assertj.core.api.Assertions.assertThat; + +class GatewayAutoConfigurationTests { + + private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() + .withPropertyValues("spring.sleuth.noop.enabled=true").withConfiguration( + AutoConfigurations.of(TraceNoOpAutoConfiguration.class, TraceWebClientAutoConfiguration.class)); + + @Test + void should_not_create_gateway_trace_filters_when_reactor_netty_client_on_classpath() { + this.contextRunner.run(context -> assertThat(context).doesNotHaveBean(HttpHeadersFilter.class)); + } + + @Test + void should_create_gateway_trace_filters_when_reactor_netty_client_not_on_classpath() { + this.contextRunner.withClassLoader(new FilteredClassLoader(HttpClient.class)) + .run(context -> assertThat(context).hasSingleBean(TraceResponseHttpHeadersFilter.class) + .hasSingleBean(TraceRequestHttpHeadersFilter.class)); + } + +} diff --git a/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java b/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java index 118d081bc..0e3de4206 100644 --- a/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java +++ b/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java @@ -25,6 +25,8 @@ import java.util.function.BiConsumer; import java.util.function.Function; import java.util.function.Supplier; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import reactor.core.publisher.Mono; import reactor.netty.Connection; import reactor.netty.http.client.HttpClient; @@ -86,6 +88,8 @@ public class HttpClientBeanPostProcessor implements BeanPostProcessor { static class TracingMapConnect implements Function, Mono> { + private static final Log log = LogFactory.getLog(TracingMapConnect.class); + static final Exception CANCELLED_ERROR = new CancellationException("CANCELLED") { @Override public Throwable fillInStackTrace() { @@ -116,6 +120,9 @@ public class HttpClientBeanPostProcessor implements BeanPostProcessor { // like onComplete() completed the span (clearing the reference). Span span = pendingSpan.getAndSet(null); if (span != null) { + if (log.isDebugEnabled()) { + log.debug("Marking span [" + span + "] with cancelled error"); + } span.error(CANCELLED_ERROR); span.end(); } @@ -126,6 +133,8 @@ public class HttpClientBeanPostProcessor implements BeanPostProcessor { private static class TracingDoOnRequest implements BiConsumer { + private static final Log log = LogFactory.getLog(TracingDoOnRequest.class); + final ConfigurableApplicationContext context; HttpClientHandler handler; @@ -153,15 +162,16 @@ public class HttpClientBeanPostProcessor implements BeanPostProcessor { // update this code! Span span = pendingSpan.getAndSet(null); if (span != null) { - assert false : "span exists when it shouldn't!"; span.abandon(); // abandon instead of break } // Start a new client span with the appropriate parent TraceContext parent = req.currentContextView().getOrDefault(TraceContext.class, null); HttpClientRequestWrapper request = new HttpClientRequestWrapper(req, connection); - span = handler().handleSend(request, parent); + if (log.isDebugEnabled()) { + log.debug("Handled send of the netty client span [" + span + "] with parent [" + parent + "]"); + } pendingSpan.set(span); } @@ -211,6 +221,8 @@ public class HttpClientBeanPostProcessor implements BeanPostProcessor { private static abstract class AbstractTracingDoOnHandler { + private static final Log log = LogFactory.getLog(AbstractTracingDoOnHandler.class); + final ConfigurableApplicationContext context; HttpClientHandler handler; @@ -236,6 +248,9 @@ public class HttpClientBeanPostProcessor implements BeanPostProcessor { if (span == null) { return; // Unexpected. In the handle method, without a span to finish! } + if (log.isDebugEnabled()) { + log.debug("Handle receive of the netty client span [" + span + "]"); + } HttpClientResponseWrapper response = new HttpClientResponseWrapper(resp, error); handler().handleReceive(response, span); } diff --git a/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java b/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java index 4ba470676..5be41ac2c 100644 --- a/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java +++ b/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java @@ -44,6 +44,8 @@ public class TraceRequestHttpHeadersFilter extends AbstractHttpHeadersFilter { static final String TRACE_REQUEST_ATTR = TraceContext.class.getName(); + static final String TRACE_REQUEST_ATTR_FROM_TRACE_WEB_FILTER = Span.class.getName(); + public TraceRequestHttpHeadersFilter(Tracer tracer, HttpClientHandler handler, Propagator propagator) { super(tracer, handler, propagator); } @@ -84,12 +86,20 @@ public class TraceRequestHttpHeadersFilter extends AbstractHttpHeadersFilter { private Span currentSpan(ServerWebExchange exchange) { Object attribute = exchange.getAttribute(TRACE_REQUEST_ATTR); + Object span = exchange.getAttribute(TRACE_REQUEST_ATTR_FROM_TRACE_WEB_FILTER); if (attribute instanceof Span) { if (log.isDebugEnabled()) { log.debug("Found trace request attribute in the server web exchange [" + attribute + "]"); } return (Span) attribute; } + else if (span instanceof Span) { + if (log.isDebugEnabled()) { + log.debug("Found trace request attribute in the server web exchange set by TraceWebFilter [" + span + + "]"); + } + return (Span) span; + } return this.tracer.currentSpan(); } @@ -97,10 +107,7 @@ public class TraceRequestHttpHeadersFilter extends AbstractHttpHeadersFilter { if (currentSpan == null) { return this.handler.handleSend(request); } - try (Tracer.SpanInScope ws = this.tracer.withSpan(currentSpan)) { - Span clientSpan = this.tracer.nextSpan(); - return this.handler.handleSend(request, clientSpan.context()); - } + return this.handler.handleSend(request, currentSpan.context()); } private void addHeadersWithInput(HttpHeaders filteredHeaders, HttpHeaders headersWithInput) { diff --git a/tests/common/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java b/tests/common/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java index 5cda42385..69777c4e7 100644 --- a/tests/common/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java +++ b/tests/common/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java @@ -24,6 +24,7 @@ import org.assertj.core.api.BDDAssertions; import org.junit.jupiter.api.Test; import org.springframework.cloud.gateway.filter.headers.HttpHeadersFilter; +import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.test.TestTracingAwareSupplier; import org.springframework.http.HttpHeaders; import org.springframework.mock.http.server.reactive.MockServerHttpRequest; @@ -44,7 +45,11 @@ public abstract class TraceRequestHttpHeadersFilterTests implements TestTracingA MockServerWebExchange exchange = MockServerWebExchange.builder(request).build(); HttpHeaders filteredHeaders = filter.filter(requestHeaders(httpHeaders), exchange); + thenTraceContinuedWithNewSpan(httpHeaders, filteredHeaders); + BDDAssertions.then((Object) exchange.getAttribute(TraceRequestHttpHeadersFilter.SPAN_ATTRIBUTE)).isNotNull(); + } + private void thenTraceContinuedWithNewSpan(HttpHeaders httpHeaders, HttpHeaders filteredHeaders) { // we want to continue the trace BDDAssertions.then(high(filteredHeaders.get("X-B3-TraceId"))).isEqualTo(high(httpHeaders.get("X-B3-TraceId"))); // but we want to have a new span id @@ -53,6 +58,25 @@ public abstract class TraceRequestHttpHeadersFilterTests implements TestTracingA BDDAssertions.then(filteredHeaders.get("X-Hello-Request")) .isEqualTo(Collections.singletonList("Request World")); BDDAssertions.then(filteredHeaders.get("X-Auth-User")).hasSize(1); + } + + @Test + public void should_continue_span_tracing_when_span_already_in_exchange_attributes() { + HttpHeadersFilter filter = new TraceRequestHttpHeadersFilter(tracerTest().tracing().tracer(), + tracerTest().tracing().httpClientHandler(), tracerTest().tracing().propagator()); + HttpHeaders httpHeaders = new HttpHeaders(); + Span span = tracerTest().tracing().tracer().nextSpan(); + httpHeaders.set("X-Hello", "World"); + httpHeaders.set("X-B3-TraceId", span.context().traceId()); + httpHeaders.set("X-B3-SpanId", span.context().spanId()); + MockServerHttpRequest request = MockServerHttpRequest.post("foo/bar").headers(httpHeaders).build(); + MockServerWebExchange exchange = MockServerWebExchange.builder(request).build(); + exchange.getAttributes().put(TraceRequestHttpHeadersFilter.TRACE_REQUEST_ATTR_FROM_TRACE_WEB_FILTER, span); + + HttpHeaders filteredHeaders = filter.filter(requestHeaders(httpHeaders), exchange); + + // we want to continue the trace + thenTraceContinuedWithNewSpan(httpHeaders, filteredHeaders); BDDAssertions.then((Object) exchange.getAttribute(TraceRequestHttpHeadersFilter.SPAN_ATTRIBUTE)).isNotNull(); }