Merge branch '1.3.x'

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java

@@ -247,16 +247,23 @@ public class TraceFilter extends GenericFilterBean {
 				if (log.isDebugEnabled()) {
 					log.debug("Detaching the span " + span + " since the response was unsuccessful");
 				}
-				clearTraceAttribute(request);
+				if (!hasErrorController()) {
+					clearTraceAttribute(request);
+				}
 				if (exception == null || !hasErrorController()) {
 					handler().handleSend(response, exception, span);
 				} else {
-					span.abandon();
+					abandonSpan(span);
 				}
 			}
 		}
 	}
 
+	// visible for tests
+	void abandonSpan(Span span) {
+		span.abandon();
+	}
+
 	private void addResponseTagsForSpanWithoutParent(Throwable exception,
 			HttpServletRequest request, HttpServletResponse response, Span span) {
 		if (exception == null && spanWithoutParent(request) && response.getStatus() >= 100) {

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegrationTests.java

@@ -37,8 +37,10 @@ import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.slf4j.MDC;
+import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.actuate.autoconfigure.web.server.ManagementServerProperties;
+import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
 import org.springframework.boot.test.context.SpringBootTest;
 import org.springframework.cloud.sleuth.TraceKeys;
 import org.springframework.cloud.sleuth.instrument.DefaultTestAutoConfiguration;
@@ -59,7 +61,9 @@ import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.RestController;
 import org.springframework.web.context.request.async.DeferredResult;
 import org.springframework.web.filter.GenericFilterBean;
+import org.springframework.web.util.NestedServletException;
 
+import static org.assertj.core.api.Assertions.fail;
 import static org.assertj.core.api.BDDAssertions.then;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.asyncDispatch;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
@@ -164,7 +168,7 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest {
 	}
 
 	@Test
-	public void should_log_tracing_information_when_exception_was_thrown() throws Exception {
+	public void should_log_tracing_information_when_404_exception_was_thrown() throws Exception {
 		Long expectedTraceId = new Random().nextLong();
 
 		whenSentToNonExistentEndpointWithTraceId(expectedTraceId);
@@ -173,6 +177,23 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest {
 		then(this.tracer.currentSpan()).isNull();
 	}
 
+	@Test
+	public void should_log_tracing_information_when_500_exception_was_thrown() throws Exception {
+		Long expectedTraceId = new Random().nextLong();
+
+		try {
+			whenSentToExceptionThrowingEndpoint(expectedTraceId);
+			fail("Should fail");
+		} catch (NestedServletException e) {
+			then(e).hasRootCauseInstanceOf(RuntimeException.class);
+		}
+
+		// we need to dump the span cause it's not in TraceFilter since TF
+		// has also error dispatch and the ErrorController would report the span
+		then(this.reporter.getSpans()).hasSize(1);
+		then(this.reporter.getSpans().get(0).tags()).containsKey("error");
+	}
+
 	@Test
 	public void should_assume_that_a_request_without_span_and_with_trace_is_a_root_span() throws Exception {
 		Long expectedTraceId = new Random().nextLong();
@@ -230,6 +251,10 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest {
 		return sendRequestWithTraceId("/exception/nonExistent", TRACE_ID_NAME, passedTraceId, HttpStatus.NOT_FOUND);
 	}
 
+	private MvcResult whenSentToExceptionThrowingEndpoint(Long passedTraceId) throws Exception {
+		return sendRequestWithTraceId("/throwsException", TRACE_ID_NAME, passedTraceId, HttpStatus.INTERNAL_SERVER_ERROR);
+	}
+
 	private MvcResult sendPingWithTraceId(String headerName, Long traceId)
 			throws Exception {
 		return sendRequestWithTraceId("/ping", headerName, traceId);
@@ -275,6 +300,8 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest {
 	@Configuration
 	protected static class Config {
 
+		private static final Log log = LogFactory.getLog(Config.class);
+
 		@RestController
 		public static class TestController {
 			@Autowired
@@ -329,9 +356,20 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest {
 			return Sampler.ALWAYS_SAMPLE;
 		}
 
+		@Bean
+		TraceFilter myTraceFilter(BeanFactory beanFactory,
+				SkipPatternProvider skipPatternProvider) {
+			return new TraceFilter(beanFactory, skipPatternProvider.skipPattern()) {
+				@Override void abandonSpan(Span span) {
+					log.info("Simulating Error Controller");
+					span.finish();
+				}
+			};
+		}
+
 		@Bean
 		@Order(TraceFilter.ORDER + 1)
-		Filter myTraceFilter(Tracer tracer) {
+		Filter myFilter(Tracer tracer) {
 			return new MyFilter(tracer);
 		}
 	}