From f586661b78d096c4652f93a12caa4103f831ace2 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Tue, 22 Oct 2019 12:41:53 +0200 Subject: [PATCH] Reusing input headers for Gateway instrumentation fixes gh-1469 --- .circleci/config.yml | 1 - .../client/TraceRequestHttpHeadersFilter.java | 58 +++++++++++-------- .../TraceRequestHttpHeadersFilterTests.java | 41 +++++++++++-- 3 files changed, 69 insertions(+), 31 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index bbc27ed71..a9d1dd966 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -10,7 +10,6 @@ jobs: branches: ignore: - gh-pages # list of branches to ignore - resource_class: large steps: - checkout - restore_cache: 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 4408c148c..5944aab85 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 @@ -29,6 +29,7 @@ import org.springframework.cloud.gateway.filter.headers.HttpHeadersFilter; import org.springframework.http.HttpHeaders; import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.http.server.reactive.ServerHttpResponse; +import org.springframework.lang.NonNull; import org.springframework.web.server.ServerWebExchange; final class TraceRequestHttpHeadersFilter extends AbstractHttpHeadersFilter { @@ -48,17 +49,16 @@ final class TraceRequestHttpHeadersFilter extends AbstractHttpHeadersFilter { if (log.isDebugEnabled()) { log.debug("Will instrument the HTTP request headers"); } - ServerHttpRequest.Builder builder = exchange.getRequest().mutate(); - Span span = this.handler.handleSend(this.injector, builder); + TraceCarrier carrier = new TraceCarrier(exchange.getRequest(), input); + Span span = this.handler.handleSend(this.injector, carrier); if (log.isDebugEnabled()) { - log.debug( - "Client span " + span + " created for the request. New headers are " - + builder.build().getHeaders().toSingleValueMap()); + log.debug("Client span " + span + " created for the request. New headers are " + + carrier.filteredHeaders.toSingleValueMap()); } exchange.getAttributes().put(SPAN_ATTRIBUTE, span); HttpHeaders headersWithInput = new HttpHeaders(); headersWithInput.addAll(input); - builder.build().getHeaders().forEach(headersWithInput::put); + carrier.filteredHeaders.forEach(headersWithInput::put); return headersWithInput; } @@ -69,10 +69,22 @@ final class TraceRequestHttpHeadersFilter extends AbstractHttpHeadersFilter { } +class TraceCarrier { + + final ServerHttpRequest originalRequest; + + final HttpHeaders filteredHeaders; + + TraceCarrier(@NonNull ServerHttpRequest originalRequest, @NonNull HttpHeaders filteredHeaders) { + this.originalRequest = originalRequest; + this.filteredHeaders = filteredHeaders; + } + +} + final class TraceResponseHttpHeadersFilter extends AbstractHttpHeadersFilter { - private static final Log log = LogFactory - .getLog(TraceResponseHttpHeadersFilter.class); + private static final Log log = LogFactory.getLog(TraceResponseHttpHeadersFilter.class); private TraceResponseHttpHeadersFilter(HttpTracing httpTracing) { super(httpTracing); @@ -109,23 +121,23 @@ 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.Builder carrier, String key, String value) { - carrier.headers(httpHeaders -> httpHeaders.set(key, value)); + public void put(TraceCarrier carrier, String key, String value) { + carrier.filteredHeaders.set(key, value); } @Override public String toString() { - return "ServerHttpRequest.Builder::header"; + return "TraceCarrier::httpHeaders::set"; } }; final Tracer tracer; - final HttpClientHandler handler; + final HttpClientHandler handler; - final TraceContext.Injector injector; + final TraceContext.Injector injector; final HttpTracing httpTracing; @@ -136,29 +148,27 @@ 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.Builder request) { - return request.build().getMethodValue(); + public String method(TraceCarrier request) { + return request.originalRequest.getMethodValue(); } @Override - public String url(ServerHttpRequest.Builder request) { - return request.build().getURI().toString(); + public String url(TraceCarrier request) { + return request.originalRequest.getURI().toString(); } @Override - public String requestHeader(ServerHttpRequest.Builder request, String name) { - Object result = request.build().getHeaders().get(name); + public String requestHeader(TraceCarrier request, String name) { + Object result = request.filteredHeaders.get(name); return result != null ? result.toString() : ""; } @Override public Integer statusCode(ServerHttpResponse response) { - return response.getStatusCode() != null ? response.getStatusCode().value() - : null; + return response.getStatusCode() != null ? response.getStatusCode().value() : null; } } 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 index 4917b4f4a..35ebce273 100644 --- 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 @@ -48,14 +48,13 @@ public class TraceRequestHttpHeadersFilterTests { HttpHeadersFilter filter = TraceRequestHttpHeadersFilter.create(this.httpTracing); HttpHeaders httpHeaders = new HttpHeaders(); httpHeaders.set("X-Hello", "World"); - httpHeaders.set("X-Auth-User", "aaaa"); 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(requestHeaders(), exchange); + HttpHeaders filteredHeaders = filter.filter(requestHeaders(httpHeaders), exchange); BDDAssertions.then(filteredHeaders.get("X-B3-TraceId")) .isNotEqualTo(httpHeaders.get("X-B3-TraceId")); @@ -81,7 +80,7 @@ public class TraceRequestHttpHeadersFilterTests { .headers(httpHeaders).build(); MockServerWebExchange exchange = MockServerWebExchange.builder(request).build(); - HttpHeaders filteredHeaders = filter.filter(requestHeaders(), exchange); + HttpHeaders filteredHeaders = filter.filter(requestHeaders(httpHeaders), exchange); BDDAssertions.then(filteredHeaders.get("X-B3-TraceId")).isNotEmpty(); BDDAssertions.then(filteredHeaders.get("X-B3-SpanId")).isNotEmpty(); @@ -95,18 +94,40 @@ public class TraceRequestHttpHeadersFilterTests { .isNotNull(); } - // #1352 + // #1469 @Test - public void should_set_tracing_headers_with_multiple_values() { + public void should_reuse_headers_only_from_input_since_exchange_may_contain_already_ignored_headers() { HttpHeadersFilter filter = TraceRequestHttpHeadersFilter.create(this.httpTracing); HttpHeaders httpHeaders = new HttpHeaders(); - httpHeaders.addAll("X-Hello", Arrays.asList("World1", "World2")); + httpHeaders.set("X-Hello", "World"); MockServerHttpRequest request = MockServerHttpRequest.post("foo/bar") .headers(httpHeaders).build(); MockServerWebExchange exchange = MockServerWebExchange.builder(request).build(); HttpHeaders filteredHeaders = filter.filter(requestHeaders(), exchange); + BDDAssertions.then(filteredHeaders.get("X-B3-TraceId")).isNotEmpty(); + BDDAssertions.then(filteredHeaders.get("X-B3-SpanId")).isNotEmpty(); + BDDAssertions.then(filteredHeaders.get("X-Hello")).isNullOrEmpty(); + BDDAssertions + .then((Object) exchange + .getAttribute(TraceRequestHttpHeadersFilter.SPAN_ATTRIBUTE)) + .isNotNull(); + } + + // #1352 + @Test + public void should_set_tracing_headers_with_multiple_values() { + HttpHeadersFilter filter = TraceRequestHttpHeadersFilter.create(this.httpTracing); + HttpHeaders httpHeaders = new HttpHeaders(); + httpHeaders.add("X-Hello-Request", "Request World"); + httpHeaders.addAll("X-Hello", Arrays.asList("World1", "World2")); + 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")).isNotEmpty(); BDDAssertions.then(filteredHeaders.get("X-B3-SpanId")).isNotEmpty(); BDDAssertions.then(filteredHeaders.get("X-Hello")) @@ -126,4 +147,12 @@ public class TraceRequestHttpHeadersFilterTests { return headers; } + private HttpHeaders requestHeaders(HttpHeaders originalHeaders) { + HttpHeaders headers = new HttpHeaders(); + headers.putAll(originalHeaders); + headers.add("X-Hello-Request", "Request World"); + headers.add("X-Auth-User", "aaaa"); + return headers; + } + }