SpringCloud/spring-cloud-sleuth
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Don't allow double Gateway instrumentation (#1882)

Browse Source 
* 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
This commit is contained in:
Marcin Grzejszczak
2021-03-16 14:42:44 +01:00
committed by GitHub
parent 2348cf17cc
commit 93b59a6d75
5 changed files with 104 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
spring-cloud-sleuth-autoconfigure/src/main/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/client/TraceWebClientAutoConfiguration.java
View File

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

50
spring-cloud-sleuth-autoconfigure/src/test/java/org/springframework/cloud/sleuth/autoconfig/instrument/web/client/GatewayAutoConfigurationTests.java Normal file
View File

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

19
spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java
View File

@@ -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<? extends Connection>, Mono<? extends Connection>> {
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<HttpClientRequest, Connection> {
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);
}

15
spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java
View File

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

24
tests/common/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java
View File

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