diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceLoadBalancerFeignClient.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceLoadBalancerFeignClient.java index 9a459b7bc..d27656fb3 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceLoadBalancerFeignClient.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceLoadBalancerFeignClient.java @@ -17,7 +17,6 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; import java.io.IOException; -import java.util.HashMap; import brave.Span; import brave.Tracer; @@ -86,9 +85,8 @@ public class TraceLoadBalancerFeignClient extends LoadBalancerFeignClient { log.debug( "General exception was thrown, so most likely the traced client wasn't called. Falling back to a manual span"); } - fallbackSpan = tracingFeignClient().handleSend( - new HashMap<>(request.headers()), request, fallbackSpan); - tracingFeignClient().handleReceive(fallbackSpan, response, e); + tracingFeignClient().handleSendAndReceive(fallbackSpan, request, response, + e); } throw e; } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java index 1ca7adee4..49914142b 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java @@ -17,6 +17,7 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; import java.io.IOException; +import java.net.URI; import java.nio.charset.Charset; import java.util.Collection; import java.util.Collections; @@ -28,7 +29,6 @@ import brave.Tracer; import brave.http.HttpClientHandler; import brave.http.HttpTracing; import brave.propagation.Propagation; -import brave.propagation.TraceContext; import feign.Client; import feign.Request; import feign.Response; @@ -72,14 +72,11 @@ final class TracingFeignClient implements Client { final Client delegate; - final HttpClientHandler handler; - - final TraceContext.Injector>> injector; + final HttpClientHandler handler; TracingFeignClient(HttpTracing httpTracing, Client delegate) { this.tracer = httpTracing.tracing().tracer(); - this.handler = HttpClientHandler.create(httpTracing, new HttpAdapter()); - this.injector = httpTracing.tracing().propagation().injector(SETTER); + this.handler = HttpClientHandler.create(httpTracing); this.delegate = delegate; } @@ -88,74 +85,135 @@ final class TracingFeignClient implements Client { } @Override - public Response execute(Request request, Request.Options options) throws IOException { - Map> headers = new LinkedHashMap<>(request.headers()); - Span span = handleSend(headers, request, null); + public Response execute(Request req, Request.Options options) throws IOException { + HttpClientRequest request = new HttpClientRequest(req); + Span span = this.handler.handleSend(request); if (log.isDebugEnabled()) { log.debug("Handled send of " + span); } - Response response = null; + HttpClientResponse response = null; Throwable error = null; try (Tracer.SpanInScope ws = this.tracer.withSpanInScope(span)) { - response = this.delegate.execute(modifiedRequest(request, headers), options); - return response; + Response res = this.delegate.execute(request.build(), options); + if (res != null) { + response = new HttpClientResponse(res); + } + else { // possibly null on bad implementation or mocks + response = new HttpClientResponse( + Response.builder().request(req).build()); + } + return res; } catch (IOException | RuntimeException | Error e) { error = e; throw e; } finally { - handleReceive(span, response, error); + this.handler.handleReceive(response, error, span); + if (log.isDebugEnabled()) { log.debug("Handled receive of " + span); } } } - Span handleSend(Map> headers, Request request, - Span clientSpan) { - if (clientSpan != null) { - return this.handler.handleSend(this.injector, headers, request, clientSpan); - } - return this.handler.handleSend(this.injector, headers, request); + void handleSendAndReceive(Span span, Request request, Response response, + Throwable error) { + this.handler.handleSend(new HttpClientRequest(request), span); + this.handler.handleReceive( + response != null ? new HttpClientResponse(response) : null, error, span); } - void handleReceive(Span span, Response response, Throwable error) { - this.handler.handleReceive(response, error, span); - } + static final class HttpClientRequest extends brave.http.HttpClientRequest { - private Request modifiedRequest(Request request, - Map> headers) { - String method = request.method(); - String url = request.url(); - byte[] body = request.body(); - Charset charset = request.charset(); - return Request.create(method, url, headers, body, charset); - } + final Request delegate; - static final class HttpAdapter - extends brave.http.HttpClientAdapter { + Map> headers; - @Override - public String method(Request request) { - return request.method(); + HttpClientRequest(Request delegate) { + this.delegate = delegate; } @Override - public String url(Request request) { - return request.url(); + public Object unwrap() { + return delegate; } @Override - public String requestHeader(Request request, String name) { - Collection result = request.headers().get(name); + public String method() { + return delegate.method(); + } + + @Override + public String path() { + String url = url(); + if (url == null) { + return null; + } + return URI.create(url).getPath(); + } + + @Override + public String url() { + return delegate.url(); + } + + @Override + public String header(String name) { + Collection result = delegate.headers().get(name); return result != null && result.iterator().hasNext() ? result.iterator().next() : null; } @Override - public Integer statusCode(Response response) { - return response.status(); + public void header(String name, String value) { + if (headers == null) { + headers = new LinkedHashMap<>(delegate.headers()); + } + if (!headers.containsKey(name)) { + headers.put(name, Collections.singletonList(value)); + if (log.isTraceEnabled()) { + log.trace( + "Added key [" + name + "] and header value [" + value + "]"); + } + } + else { + // TODO: this is incorrect to ignore as opposed to overwrite! + if (log.isTraceEnabled()) { + log.trace("Key [" + name + "] already there in the headers"); + } + } + } + + Request build() { + if (headers == null) { + return delegate; + } + String method = delegate.method(); + String url = delegate.url(); + byte[] body = delegate.body(); + Charset charset = delegate.charset(); + return Request.create(method, url, headers, body, charset); + } + + } + + static final class HttpClientResponse extends brave.http.HttpClientResponse { + + final Response delegate; + + HttpClientResponse(Response delegate) { + this.delegate = delegate; + } + + @Override + public Object unwrap() { + return delegate; + } + + @Override + public int statusCode() { + return delegate.status(); } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClientTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClientTests.java index 78a5b3b42..53dc877cd 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClientTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClientTests.java @@ -17,8 +17,9 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; import java.io.IOException; -import java.nio.charset.Charset; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import brave.Span; import brave.Tracer; @@ -36,9 +37,7 @@ import org.mockito.BDDMockito; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; -import org.springframework.beans.factory.BeanFactory; import org.springframework.cloud.sleuth.instrument.web.SleuthHttpParserAccessor; -import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; import static org.assertj.core.api.BDDAssertions.then; @@ -48,15 +47,16 @@ import static org.assertj.core.api.BDDAssertions.then; @RunWith(MockitoJUnitRunner.class) public class TracingFeignClientTests { - ArrayListSpanReporter reporter = new ArrayListSpanReporter(); + Request request = Request.create("GET", "https://foo", new HashMap<>(), null, null); - @Mock - BeanFactory beanFactory; + Request.Options options = new Request.Options(); + + List spans = new ArrayList<>(); Tracing tracing = Tracing.newBuilder() .currentTraceContext(ThreadLocalCurrentTraceContext.newBuilder() .addScopeDecorator(StrictScopeDecorator.create()).build()) - .spanReporter(this.reporter).build(); + .spanReporter(spans::add).build(); Tracer tracer = this.tracing.tracer(); @@ -78,18 +78,14 @@ public class TracingFeignClientTests { Span span = this.tracer.nextSpan().name("foo"); try (Tracer.SpanInScope ws = this.tracer.withSpanInScope(span.start())) { - this.traceFeignClient - .execute( - Request.create("GET", "http://foo", new HashMap<>(), - "".getBytes(), Charset.defaultCharset()), - new Request.Options()); + this.traceFeignClient.execute(this.request, this.options); } finally { span.finish(); } - then(this.reporter.getSpans().get(0)).extracting("kind.ordinal") - .contains(Span.Kind.CLIENT.ordinal()); + then(spans.get(0)).extracting("kind.ordinal") + .isEqualTo(Span.Kind.CLIENT.ordinal()); } @Test @@ -99,11 +95,7 @@ public class TracingFeignClientTests { .willThrow(new RuntimeException("exception has occurred")); try (Tracer.SpanInScope ws = this.tracer.withSpanInScope(span.start())) { - this.traceFeignClient - .execute( - Request.create("GET", "http://foo", new HashMap<>(), - "".getBytes(), Charset.defaultCharset()), - new Request.Options()); + this.traceFeignClient.execute(this.request, this.options); BDDAssertions.fail("Exception should have been thrown"); } catch (Exception e) { @@ -112,21 +104,17 @@ public class TracingFeignClientTests { span.finish(); } - then(this.reporter.getSpans().get(0)).extracting("kind.ordinal") - .contains(Span.Kind.CLIENT.ordinal()); - then(this.reporter.getSpans().get(0).tags()).containsEntry("error", - "exception has occurred"); + then(this.spans.get(0)).extracting("kind.ordinal") + .isEqualTo(Span.Kind.CLIENT.ordinal()); + then(this.spans.get(0).tags()).containsEntry("error", "exception has occurred"); } @Test public void should_shorten_the_span_name() throws IOException { - this.traceFeignClient - .execute( - Request.create("GET", "http://foo/" + bigName(), new HashMap<>(), - "".getBytes(), Charset.defaultCharset()), - new Request.Options()); + this.traceFeignClient.execute(Request.create("GET", "https://foo/" + bigName(), + new HashMap<>(), null, null), this.options); - then(this.reporter.getSpans().get(0).name()).hasSize(50); + then(this.spans.get(0).name()).hasSize(50); } private String bigName() {