Not closing a span if exception happened and error controller is pres…

…ent; fixes gh-859
spring-cloud · Feb 15, 2018 · 0a5fa8b · 0a5fa8b
1 parent 3224cc6
commit 0a5fa8b
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 10 deletions.
diff --git a/...leuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java b/...leuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java
@@ -32,6 +32,7 @@
 import org.apache.commons.logging.LogFactory;
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.NoSuchBeanDefinitionException;
+import org.springframework.boot.autoconfigure.web.ErrorController;
 import org.springframework.cloud.sleuth.ErrorParser;
 import org.springframework.cloud.sleuth.Span;
 import org.springframework.cloud.sleuth.SpanReporter;
@@ -104,6 +105,7 @@ public class TraceFilter extends GenericFilterBean {
 	private HttpTraceKeysInjector httpTraceKeysInjector;
 	private ErrorParser errorParser;
 	private final BeanFactory beanFactory;
+	private Boolean hasErrorController;
 
 	private final UrlPathHelper urlPathHelper = new UrlPathHelper();
 
@@ -236,8 +238,10 @@ private void detachOrCloseSpans(HttpServletRequest request,
 				if (log.isDebugEnabled()) {
 					log.debug("Closing the span " + span + " since the response was successful");
 				}
-				tracer().close(span);
-				clearTraceAttribute(request);
+				if (exception == null || !hasErrorController()) {
+					tracer().close(span);
+					clearTraceAttribute(request);
+				}
 			} else if (errorAlreadyHandled(request) && tracer().isTracing() && !shouldCloseSpan(request)) {
 				if (log.isDebugEnabled()) {
 					log.debug(
@@ -248,15 +252,31 @@ private void detachOrCloseSpans(HttpServletRequest request,
 					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);
-				clearTraceAttribute(request);
+				if (exception == null || !hasErrorController()) {
+					clearTraceAttribute(request);
+				}
 			} else if (tracer().isTracing()) {
 				if (log.isDebugEnabled()) {
 					log.debug("Detaching the span " + span + " since the response was unsuccessful");
 				}
-				tracer().detach(span);
 				clearTraceAttribute(request);
+				if (exception == null || !hasErrorController()) {
+					tracer().detach(span);
+				}
+			}
+		}
+	}
+
+	// null check is only for tests
+	private boolean hasErrorController() {
+		if (this.hasErrorController == null) {
+			try {
+				this.hasErrorController = this.beanFactory.getBean(ErrorController.class) != null;
+			} catch (NoSuchBeanDefinitionException e) {
+				this.hasErrorController = false;
 			}
 		}
+		return this.hasErrorController;
 	}
 
 	private void addResponseTagsForSpanWithoutParent(HttpServletRequest request,

diff --git a/...th-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAspect.java b/...th-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAspect.java
@@ -16,6 +16,11 @@
 
 package org.springframework.cloud.sleuth.instrument.web;
 
+import java.lang.reflect.Field;
+import java.util.concurrent.Callable;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
 import org.apache.commons.logging.Log;
 import org.aspectj.lang.ProceedingJoinPoint;
 import org.aspectj.lang.annotation.Around;
@@ -29,11 +34,6 @@
 import org.springframework.cloud.sleuth.instrument.async.SpanContinuingTraceCallable;
 import org.springframework.web.context.request.async.WebAsyncTask;
 
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import java.lang.reflect.Field;
-import java.util.concurrent.Callable;
-
 /**
  * Aspect that adds tracing to
  * <p/>
@@ -154,7 +154,7 @@ public Object markRequestForSpanClosing(ProceedingJoinPoint pjp,
 			return pjp.proceed();
 		} finally {
 			if (log.isDebugEnabled()) {
-				log.debug("Marking span " + currentSpan + " for closure by Trace Filter");
+				log.debug("Marking span " + currentSpan + " for closure by Trace Filter or Error Controller");
 			}
 			request.setAttribute(TraceFilter.TRACE_CLOSE_SPAN_REQUEST_ATTR, true);
 		}