From c5a23c55254b77bbd389ef6cd4fe8d6f3bf5f0c4 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 9 Jan 2019 11:34:45 +0100 Subject: [PATCH] Fixed lack of propagating the proxy's client span; fixes gh-1141 --- .../client/TraceRequestHttpHeadersFilter.java | 56 ++++++++-------- .../TraceRequestHttpHeadersFilterTests.java | 49 ++++++++++++++ .../TraceResponseHttpHeadersFilterTests.java | 65 +++++++++++++++++++ 3 files changed, 140 insertions(+), 30 deletions(-) create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceResponseHttpHeadersFilterTests.java diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java index 41604465c..079fd6428 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java @@ -15,6 +15,8 @@ */ package org.springframework.cloud.sleuth.instrument.web.client; +import java.util.Collections; + import brave.Span; import brave.Tracer; import brave.http.HttpClientHandler; @@ -44,24 +46,18 @@ class TraceRequestHttpHeadersFilter extends AbstractHttpHeadersFilter { @Override public HttpHeaders filter(HttpHeaders input, ServerWebExchange exchange) { - Object storedSpan = exchange.getAttribute(SPAN_ATTRIBUTE); if (log.isDebugEnabled()) { log.debug("Will instrument the HTTP request headers"); } - Span span = clientSent(exchange, storedSpan); + ServerHttpRequest.Builder builder = exchange.getRequest().mutate(); + Span span = this.handler.handleSend(this.injector, builder); if (log.isDebugEnabled()) { - log.debug("Client span created for the request " + span); + log.debug( + "Client span " + span + " created for the request. New headers are " + + builder.build().getHeaders().toSingleValueMap()); } exchange.getAttributes().put(SPAN_ATTRIBUTE, span); - return new HttpHeaders(exchange.getRequest().getHeaders()); - } - - private Span clientSent(ServerWebExchange exchange, Object storedSpan) { - if (storedSpan != null) { - return this.handler.handleSend(this.injector, exchange.getRequest(), - (Span) storedSpan); - } - return this.handler.handleSend(this.injector, exchange.getRequest()); + return new HttpHeaders(builder.build().getHeaders()); } @Override @@ -73,7 +69,8 @@ class TraceRequestHttpHeadersFilter extends AbstractHttpHeadersFilter { class TraceResponseHttpHeadersFilter extends AbstractHttpHeadersFilter { - private static final Log log = LogFactory.getLog(TraceResponseHttpHeadersFilter.class); + private static final Log log = LogFactory + .getLog(TraceResponseHttpHeadersFilter.class); static HttpHeadersFilter create(HttpTracing httpTracing) { return new TraceResponseHttpHeadersFilter(httpTracing); @@ -94,7 +91,7 @@ class TraceResponseHttpHeadersFilter extends AbstractHttpHeadersFilter { } this.handler.handleReceive(exchange.getResponse(), null, (Span) storedSpan); if (log.isDebugEnabled()) { - log.debug("The response was handled"); + log.debug("The response was handled for span " + storedSpan); } return new HttpHeaders(input); } @@ -110,25 +107,24 @@ abstract class AbstractHttpHeadersFilter implements HttpHeadersFilter { static final String SPAN_ATTRIBUTE = Span.class.getName(); - private static final Propagation.Setter SETTER = new Propagation.Setter() { + private static final Propagation.Setter SETTER = new Propagation.Setter() { @Override - public void put(ServerHttpRequest carrier, String key, String value) { - if (!carrier.getHeaders().containsKey(key)) { - carrier.getHeaders().add(key, value); - } + public void put(ServerHttpRequest.Builder carrier, String key, String value) { + carrier.headers(httpHeaders -> httpHeaders.replace(key, + Collections.singletonList(value))); } @Override public String toString() { - return "ServerHttpRequest::HttpHeaders::add"; + return "ServerHttpRequest.Builder::header"; } }; final Tracer tracer; - final HttpClientHandler handler; + final HttpClientHandler handler; - final TraceContext.Injector injector; + final TraceContext.Injector injector; final HttpTracing httpTracing; @@ -139,22 +135,22 @@ abstract class AbstractHttpHeadersFilter implements HttpHeadersFilter { this.httpTracing = httpTracing; } - private static class ServerHttpAdapter - extends brave.http.HttpClientAdapter { + private static class ServerHttpAdapter extends + brave.http.HttpClientAdapter { @Override - public String method(ServerHttpRequest request) { - return request.getMethodValue(); + public String method(ServerHttpRequest.Builder request) { + return request.build().getMethodValue(); } @Override - public String url(ServerHttpRequest request) { - return request.getURI().toString(); + public String url(ServerHttpRequest.Builder request) { + return request.build().getURI().toString(); } @Override - public String requestHeader(ServerHttpRequest request, String name) { - Object result = request.getHeaders().get(name); + public String requestHeader(ServerHttpRequest.Builder request, String name) { + Object result = request.build().getHeaders().get(name); return result != null ? result.toString() : ""; } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java new file mode 100644 index 000000000..deb1b74de --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java @@ -0,0 +1,49 @@ +package org.springframework.cloud.sleuth.instrument.web.client; + +import brave.Tracing; +import brave.http.HttpTracing; +import brave.propagation.StrictScopeDecorator; +import brave.propagation.ThreadLocalCurrentTraceContext; +import org.assertj.core.api.BDDAssertions; +import org.junit.Test; + +import org.springframework.cloud.gateway.filter.headers.HttpHeadersFilter; +import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; +import org.springframework.http.HttpHeaders; +import org.springframework.mock.http.server.reactive.MockServerHttpRequest; +import org.springframework.mock.web.server.MockServerWebExchange; + +public class TraceRequestHttpHeadersFilterTests { + + ArrayListSpanReporter reporter = new ArrayListSpanReporter(); + Tracing tracing = Tracing.newBuilder() + .currentTraceContext(ThreadLocalCurrentTraceContext.newBuilder() + .addScopeDecorator(StrictScopeDecorator.create()).build()) + .spanReporter(this.reporter).build(); + HttpTracing httpTracing = HttpTracing + .newBuilder(this.tracing).build(); + + @Test + public void should_override_any_tracing_headers() { + HttpHeadersFilter filter = TraceRequestHttpHeadersFilter.create(this.httpTracing); + HttpHeaders httpHeaders = new HttpHeaders(); + httpHeaders.set("X-B3-TraceId", "52f112af7472aff0"); + httpHeaders.set("X-B3-SpanId", "53e6ab6fc5dfee58"); + MockServerHttpRequest request = MockServerHttpRequest + .post("foo/bar") + .headers(httpHeaders) + .build(); + MockServerWebExchange exchange = MockServerWebExchange + .builder(request) + .build(); + + HttpHeaders filteredHeaders = filter.filter(httpHeaders, exchange); + + BDDAssertions.then(filteredHeaders.get("X-B3-TraceId")) + .isNotEqualTo(httpHeaders.get("X-B3-TraceId")); + BDDAssertions.then(filteredHeaders.get("X-B3-SpanId")) + .isNotEqualTo(httpHeaders.get("X-B3-SpanId")); + BDDAssertions.then((Object) exchange.getAttribute(TraceRequestHttpHeadersFilter.SPAN_ATTRIBUTE)).isNotNull(); + } + +} \ No newline at end of file diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceResponseHttpHeadersFilterTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceResponseHttpHeadersFilterTests.java new file mode 100644 index 000000000..0f25d9efd --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceResponseHttpHeadersFilterTests.java @@ -0,0 +1,65 @@ +package org.springframework.cloud.sleuth.instrument.web.client; + +import brave.Tracing; +import brave.http.HttpTracing; +import brave.propagation.StrictScopeDecorator; +import brave.propagation.ThreadLocalCurrentTraceContext; +import org.assertj.core.api.BDDAssertions; +import org.junit.Test; + +import org.springframework.cloud.gateway.filter.headers.HttpHeadersFilter; +import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; +import org.springframework.http.HttpHeaders; +import org.springframework.mock.http.server.reactive.MockServerHttpRequest; +import org.springframework.mock.web.server.MockServerWebExchange; + +public class TraceResponseHttpHeadersFilterTests { + + ArrayListSpanReporter reporter = new ArrayListSpanReporter(); + Tracing tracing = Tracing.newBuilder() + .currentTraceContext(ThreadLocalCurrentTraceContext.newBuilder() + .addScopeDecorator(StrictScopeDecorator.create()).build()) + .spanReporter(this.reporter).build(); + HttpTracing httpTracing = HttpTracing + .newBuilder(this.tracing).build(); + + @Test + public void should_not_report_span_when_no_span_was_present_in_attribute() { + HttpHeadersFilter filter = TraceResponseHttpHeadersFilter.create(this.httpTracing); + HttpHeaders httpHeaders = new HttpHeaders(); + httpHeaders.set("X-B3-TraceId", "52f112af7472aff0"); + httpHeaders.set("X-B3-SpanId", "53e6ab6fc5dfee58"); + MockServerHttpRequest request = MockServerHttpRequest + .post("foo/bar") + .headers(httpHeaders) + .build(); + MockServerWebExchange exchange = MockServerWebExchange + .builder(request) + .build(); + + filter.filter(httpHeaders, exchange); + + BDDAssertions.then(this.reporter.getSpans()).isEmpty(); + } + + @Test + public void should_report_span_when_span_was_present_in_attribute() { + HttpHeadersFilter filter = TraceResponseHttpHeadersFilter.create(this.httpTracing); + HttpHeaders httpHeaders = new HttpHeaders(); + httpHeaders.set("X-B3-TraceId", "52f112af7472aff0"); + httpHeaders.set("X-B3-SpanId", "53e6ab6fc5dfee58"); + MockServerHttpRequest request = MockServerHttpRequest + .post("foo/bar") + .headers(httpHeaders) + .build(); + MockServerWebExchange exchange = MockServerWebExchange + .builder(request) + .build(); + exchange.getAttributes().put(TraceResponseHttpHeadersFilter.SPAN_ATTRIBUTE, this.tracing.tracer().nextSpan()); + + filter.filter(httpHeaders, exchange); + + BDDAssertions.then(this.reporter.getSpans()).isNotEmpty(); + } + +} \ No newline at end of file