From 93b59a6d75f34a06f47e1bbc6f1045eb84487a79 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Tue, 16 Mar 2021 14:42:44 +0100 Subject: [PATCH] Don't allow double Gateway instrumentation (#1882) * Don't allow double Gateway instrumentation with this change we're doing both HeaderFilter based Gateway instrumentation and the Netty Client one. with this change we're conditionally enabling the HeaderFilter instrumentation only when there is no Netty Client one present on the classpath. fixes gh-1840 --- .../TraceWebClientAutoConfiguration.java | 2 + .../client/GatewayAutoConfigurationTests.java | 50 +++++++++++++++++++ .../client/HttpClientBeanPostProcessor.java | 19 ++++++- .../client/TraceRequestHttpHeadersFilter.java | 15 ++++-- .../TraceRequestHttpHeadersFilterTests.java | 24 +++++++++ 5 files changed, 104 insertions(+), 6 deletions(-) create mode 100644 spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/client/GatewayAutoConfigurationTests.java 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(); }