Added additional check for standard 4xx errors; fixes gh-859

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

@@ -251,15 +251,19 @@ private void detachOrCloseSpans(HttpServletRequest request,
 				if (log.isDebugEnabled()) {
 					log.debug("Will close span " + span + " since " + (shouldCloseSpan(request) ? "some component marked it for closure" : "response was unsuccessful for the root span"));
 				}
-				tracer().close(span);
+				if (!httpStatusSuccessful(response) && !hasErrorController()) {
+					tracer().close(span);
+				}
 				if (exception == null || !hasErrorController()) {
 					clearTraceAttribute(request);
 				}
 			} else if (tracer().isTracing()) {
 				if (log.isDebugEnabled()) {
 					log.debug("Detaching the span " + span + " since the response was unsuccessful");
 				}
-				clearTraceAttribute(request);
+				if (!httpStatusSuccessful(response) && !hasErrorController()) {
+					clearTraceAttribute(request);
+				}
 				if (exception == null || !hasErrorController()) {
 					tracer().detach(span);
 				}

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

@@ -3,7 +3,6 @@
 import java.util.Optional;
 import java.util.Random;
 import java.util.concurrent.CompletableFuture;
-
 import javax.servlet.http.HttpServletResponse;
 
 import org.apache.commons.logging.Log;
@@ -39,7 +38,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.util.NestedServletException;
 
+import static org.assertj.core.api.Assertions.fail;
 import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.asyncDispatch;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
@@ -137,12 +138,34 @@ public void should_add_a_custom_tag_to_the_span_created_in_controller() throws E
 	}
 
 	@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();
 
 		MvcResult mvcResult = whenSentToNonExistentEndpointWithTraceId(expectedTraceId);
 
 		then(this.tracer.getCurrentSpan()).isNull();
+		then(this.spanAccumulator.getSpans()).hasSize(1);
+		then(ExceptionUtils.getLastException()).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);
+		}
+
+		// not null cause TraceFilter has also error dispatch and
+		// the ErrorController would close the span
+		then(this.tracer.getCurrentSpan()).isNotNull();
+		this.tracer.close(this.tracer.getCurrentSpan());
+		then(this.spanAccumulator.getSpans()).hasSize(1);
+		then(this.spanAccumulator.getSpans().get(0).tags())
+				.containsKey("error");
 		then(ExceptionUtils.getLastException()).isNull();
 	}
 
@@ -203,6 +226,10 @@ private MvcResult whenSentToNonExistentEndpointWithTraceId(Long passedTraceId) t
 		return sendRequestWithTraceId("/exception/nonExistent", Span.TRACE_ID_NAME, passedTraceId, HttpStatus.NOT_FOUND);
 	}
 
+	private MvcResult whenSentToExceptionThrowingEndpoint(Long passedTraceId) throws Exception {
+		return sendRequestWithTraceId("/throwsException", Span.TRACE_ID_NAME, passedTraceId, HttpStatus.INTERNAL_SERVER_ERROR);
+	}
+
 	private MvcResult sendPingWithTraceId(String headerName, Long traceId)
 			throws Exception {
 		return sendRequestWithTraceId("/ping", headerName, traceId);