diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignBeanPostProcessor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignBeanPostProcessor.java index 588b30326..deece503a 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignBeanPostProcessor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignBeanPostProcessor.java @@ -24,6 +24,7 @@ import org.springframework.cloud.sleuth.Tracer; import feign.Client; import feign.Retryer; import feign.codec.Decoder; +import feign.codec.ErrorDecoder; /** * Post processor that wraps Feign related classes {@link Decoder}, @@ -51,6 +52,8 @@ final class FeignBeanPostProcessor implements BeanPostProcessor { return new TraceFeignRetryer(getTracer(), (Retryer) bean); } else if (bean instanceof Client && !(bean instanceof TraceFeignClient)) { return new TraceFeignClient(getTracer(), (Client) bean); + } else if (bean instanceof ErrorDecoder && !(bean instanceof TraceFeignErrorDecoder)) { + return new TraceFeignErrorDecoder(getTracer(), (ErrorDecoder) bean); } return bean; } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java index 1f67e428e..80f96d251 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java @@ -23,7 +23,7 @@ import feign.hystrix.HystrixFeign; /** * Contains {@link feign.Feign.Builder} implementation that delegates execution - * {@link feign.hystrix.HystrixFeign} with custom retryer and decoder + * {@link feign.hystrix.HystrixFeign} with tracing components * that close spans on exceptions / success and continues them on retries. * * @author Marcin Grzejszczak @@ -38,6 +38,7 @@ final class SleuthFeignBuilder { return HystrixFeign.builder() .client(new TraceFeignClient(tracer)) .retryer(new TraceFeignRetryer(tracer)) - .decoder(new TraceFeignDecoder(tracer)); + .decoder(new TraceFeignDecoder(tracer)) + .errorDecoder(new TraceFeignErrorDecoder(tracer)); } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignErrorDecoder.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignErrorDecoder.java new file mode 100644 index 000000000..dc5104901 --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignErrorDecoder.java @@ -0,0 +1,52 @@ +/* + * Copyright 2013-2016 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 + * + * http://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.instrument.web.client.feign; + +import org.springframework.cloud.sleuth.Tracer; + +import feign.Response; +import feign.codec.ErrorDecoder; + +/** + * An {@link ErrorDecoder} that closes a span before returning the exception type. + * + * @author Marcin Grzejszczak + * + * @since 1.0.0 + */ +final class TraceFeignErrorDecoder extends FeignEventPublisher implements ErrorDecoder { + + private final ErrorDecoder delegate; + + TraceFeignErrorDecoder(Tracer tracer) { + super(tracer); + this.delegate = new ErrorDecoder.Default(); + } + + TraceFeignErrorDecoder(Tracer tracer, ErrorDecoder delegate) { + super(tracer); + this.delegate = delegate; + } + + @Override public Exception decode(String methodKey, Response response) { + try { + return this.delegate.decode(methodKey, response); + } finally { + finish(); + } + } +} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignClientServerErrorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignClientServerErrorTests.java new file mode 100644 index 000000000..9d927a42c --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignClientServerErrorTests.java @@ -0,0 +1,185 @@ +/* + * Copyright 2013-2016 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 + * + * http://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.instrument.web.client.feign; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.OutputCapture; +import org.springframework.boot.test.SpringApplicationConfiguration; +import org.springframework.boot.test.WebIntegrationTest; +import org.springframework.cloud.client.loadbalancer.LoadBalanced; +import org.springframework.cloud.netflix.feign.EnableFeignClients; +import org.springframework.cloud.netflix.feign.FeignClient; +import org.springframework.cloud.netflix.ribbon.RibbonClient; +import org.springframework.cloud.sleuth.Span; +import org.springframework.cloud.sleuth.SpanReporter; +import org.springframework.cloud.sleuth.Tracer; +import org.springframework.cloud.sleuth.assertions.SleuthAssertions; +import org.springframework.cloud.sleuth.util.ExceptionUtils; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.stereotype.Component; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.web.bind.annotation.RequestHeader; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.client.RestTemplate; + +import com.netflix.hystrix.exception.HystrixRuntimeException; +import com.netflix.loadbalancer.BaseLoadBalancer; +import com.netflix.loadbalancer.ILoadBalancer; +import com.netflix.loadbalancer.Server; + +import static org.assertj.core.api.BDDAssertions.then; + +/** + * Related to https://github.com/spring-cloud/spring-cloud-sleuth/issues/257 + * @author ryarabori + */ +@RunWith(SpringJUnit4ClassRunner.class) +@SpringApplicationConfiguration(classes = { FeignClientServerErrorTests.TestConfiguration.class }) +@WebIntegrationTest(value = { "spring.application.name=fooservice" }, randomPort = true) +public class FeignClientServerErrorTests { + + @Autowired TestFeignInterface feignInterface; + @Rule public OutputCapture capture = new OutputCapture(); + + @Before + public void setup() { + ExceptionUtils.setFail(true); + } + + @Test + public void shouldCloseSpanOnInternalServerError() throws InterruptedException { + try { + this.feignInterface.internalError(); + } catch (HystrixRuntimeException e) { + } + + // ugly :/ waiting for rx thread to complete + Thread.sleep(100); + then(this.capture.toString()).doesNotContain("Tried to close span but it is not the current span"); + } + + @Test + public void shouldCloseSpanOnNotFound() throws InterruptedException { + try { + this.feignInterface.notFound(); + } catch (HystrixRuntimeException e) { + } + + // ugly :/ waiting for rx thread to complete + Thread.sleep(100); + then(this.capture.toString()).doesNotContain("Tried to close span but it is not the current span"); + } + + @Configuration + @EnableAutoConfiguration + @EnableFeignClients + @RibbonClient(value = "fooservice", configuration = SimpleRibbonClientConfiguration.class) + public static class TestConfiguration { + + @Bean + FooController fooController() { + return new FooController(); + } + + @Bean + Listener listener() { + return new Listener(); + } + + @LoadBalanced + @Bean + public RestTemplate restTemplate() { + return new RestTemplate(); + } + + } + + @FeignClient(value = "fooservice") + public interface TestFeignInterface { + + @RequestMapping(method = RequestMethod.GET, value = "/internalerror") + ResponseEntity internalError(); + + @RequestMapping(method = RequestMethod.GET, value = "/notfound") + ResponseEntity notFound(); + + } + + @Component + public static class Listener implements SpanReporter { + private List events = new ArrayList<>(); + + public List getEvents() { + return this.events; + } + + @Override + public void report(Span span) { + this.events.add(span); + } + } + + @RestController + public static class FooController { + + @Autowired Tracer tracer; + + @RequestMapping("/internalerror") + public ResponseEntity internalError(@RequestHeader(Span.TRACE_ID_NAME) String traceId, + @RequestHeader(Span.SPAN_ID_NAME) String spanId, + @RequestHeader(Span.PARENT_ID_NAME) String parentId) { + return new ResponseEntity<>("internal error", HttpStatus.INTERNAL_SERVER_ERROR); + } + + @RequestMapping("/notfound") + public ResponseEntity notFound(@RequestHeader(Span.TRACE_ID_NAME) String traceId, + @RequestHeader(Span.SPAN_ID_NAME) String spanId, + @RequestHeader(Span.PARENT_ID_NAME) String parentId) { + return new ResponseEntity<>("not found", HttpStatus.NOT_FOUND); + } + } + + @Configuration + public static class SimpleRibbonClientConfiguration { + + @Value("${local.server.port}") + private int port = 0; + + @Bean + public ILoadBalancer ribbonLoadBalancer() { + BaseLoadBalancer balancer = new BaseLoadBalancer(); + balancer.setServersList(Collections.singletonList(new Server("localhost", this.port))); + return balancer; + } + } + +}