From 2c3528e35ba25981c5a7780f26190584a047fd30 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Mon, 7 Sep 2015 12:20:18 +0100 Subject: [PATCH] Fix NPE in Feign interceptor Fixes gh-37 --- .../TraceFeignClientAutoConfiguration.java | 27 ++++++++------- .../instrument/web/client/FeignTraceTest.java | 33 ++++++++++++++++--- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceFeignClientAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceFeignClientAutoConfiguration.java index e8ff821c2..d5a84bb35 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceFeignClientAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceFeignClientAutoConfiguration.java @@ -17,6 +17,7 @@ package org.springframework.cloud.sleuth.instrument.web.client; import static java.util.Collections.singletonList; +import static org.springframework.cloud.sleuth.Trace.NOT_SAMPLED_NAME; import static org.springframework.cloud.sleuth.Trace.PARENT_ID_NAME; import static org.springframework.cloud.sleuth.Trace.SPAN_ID_NAME; import static org.springframework.cloud.sleuth.Trace.TRACE_ID_NAME; @@ -73,8 +74,7 @@ public class TraceFeignClientAutoConfiguration { public Decoder feignDecoder() { return new ResponseEntityDecoder(new SpringDecoder(messageConverters)) { @Override - public Object decode(Response response, Type type) throws IOException, - FeignException { + public Object decode(Response response, Type type) throws IOException, FeignException { return super.decode(Response.create(response.status(), response.reason(), headersWithTraceId(response.headers()), response.body()), type); } @@ -86,11 +86,14 @@ public class TraceFeignClientAutoConfiguration { return new RequestInterceptor() { @Override public void apply(RequestTemplate template) { - template.header(TRACE_ID_NAME, getCurrentSpan().getTraceId()); - setHeader(template, TRACE_ID_NAME, getCurrentSpan().getTraceId()); - setHeader(template, SPAN_ID_NAME, getCurrentSpan().getSpanId()); - setHeader(template, PARENT_ID_NAME, getParentId(getCurrentSpan())); - publish(new ClientSentEvent(this, getCurrentSpan())); + Span span = getCurrentSpan(); + if (span != null) { + template.header(TRACE_ID_NAME, span.getTraceId()); + setHeader(template, TRACE_ID_NAME, span.getTraceId()); + setHeader(template, SPAN_ID_NAME, span.getSpanId()); + setHeader(template, PARENT_ID_NAME, getParentId(span)); + publish(new ClientSentEvent(this, span)); + } } }; } @@ -102,8 +105,7 @@ public class TraceFeignClientAutoConfiguration { } private String getParentId(Span span) { - return span.getParents() != null && !span.getParents().isEmpty() ? span - .getParents().get(0) : null; + return span.getParents() != null && !span.getParents().isEmpty() ? span.getParents().get(0) : null; } public void setHeader(RequestTemplate request, String name, String value) { @@ -112,10 +114,13 @@ public class TraceFeignClientAutoConfiguration { } } - private Map> headersWithTraceId( - Map> headers) { + private Map> headersWithTraceId(Map> headers) { Map> newHeaders = new HashMap<>(); newHeaders.putAll(headers); + if (getCurrentSpan() == null) { + setHeader(newHeaders, NOT_SAMPLED_NAME, ""); + return newHeaders; + } setHeader(newHeaders, TRACE_ID_NAME, getCurrentSpan().getTraceId()); setHeader(newHeaders, SPAN_ID_NAME, getCurrentSpan().getSpanId()); setHeader(newHeaders, PARENT_ID_NAME, getParentId(getCurrentSpan())); diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/FeignTraceTest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/FeignTraceTest.java index 0ae409a3c..ffd06a578 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/FeignTraceTest.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/FeignTraceTest.java @@ -8,9 +8,7 @@ import static org.springframework.cloud.sleuth.Trace.TRACE_ID_NAME; import java.util.Arrays; import java.util.List; -import com.netflix.loadbalancer.BaseLoadBalancer; -import com.netflix.loadbalancer.ILoadBalancer; -import com.netflix.loadbalancer.Server; +import org.junit.After; import org.junit.Test; import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; @@ -33,6 +31,10 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.bind.annotation.RestController; +import com.netflix.loadbalancer.BaseLoadBalancer; +import com.netflix.loadbalancer.ILoadBalancer; +import com.netflix.loadbalancer.Server; + @RunWith(SpringJUnit4ClassRunner.class) @SpringApplicationConfiguration(classes = { TraceWebAutoConfiguration.class, FeignTraceTest.TestConfiguration.class }) @@ -41,6 +43,20 @@ public class FeignTraceTest { @Autowired TestFeignInterface testFeignInterface; + + @After + public void close() { + TraceContextHolder.removeCurrentSpan(); + } + + @Test + public void shouldWorkWhenNotTracing() { + // when + ResponseEntity response = testFeignInterface.getNoTrace(); + + // then + assertThat(getHeader(response, TRACE_ID_NAME)).isNull(); + } @Test public void shouldAttachTraceIdWhenUsingFeignClient() { @@ -62,14 +78,15 @@ public class FeignTraceTest { private String getHeader(ResponseEntity response, String name) { List headers = response.getHeaders().get(name); - assertThat(headers).asList().isNotEmpty(); - return headers.get(0); + return headers==null || headers.isEmpty() ? null : headers.get(0); } @FeignClient("fooservice") public interface TestFeignInterface { @RequestMapping(method = RequestMethod.GET, value = "/traceid") ResponseEntity getTraceId(); + @RequestMapping(method = RequestMethod.GET, value = "/notrace") + ResponseEntity getNoTrace(); } @Configuration @@ -87,6 +104,12 @@ public class FeignTraceTest { @RestController public static class FooController { + @RequestMapping(value = "/notrace", method = RequestMethod.GET) + public String notrace(@RequestHeader(name=TRACE_ID_NAME, required=false) String traceId) { + assertThat(traceId).isNull(); + return "OK"; + } + @RequestMapping(value = "/traceid", method = RequestMethod.GET) public String traceId(@RequestHeader(TRACE_ID_NAME) String traceId, @RequestHeader(SPAN_ID_NAME) String spanId,