Closing on error decoding

when an exception was thrown by Feign it was not caught by our Feign customizations and wasn't properly closed. By adding a custom implementation we're closing span whenever an exception is thrown.

Fixes #257

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;
 	}

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));
 	}
 }

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();
+		}
+	}
+}

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<String> internalError();
+
+        @RequestMapping(method = RequestMethod.GET, value = "/notfound")
+        ResponseEntity<String> notFound();
+
+    }
+
+    @Component
+    public static class Listener implements SpanReporter {
+        private List<Span> events = new ArrayList<>();
+
+        public List<Span> 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<String> 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<String> 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;
+        }
+    }
+
+}