From 55cd5afcb6f211251dfc550dd7e03aba445c1994 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Thu, 31 Dec 2015 13:36:43 +0000 Subject: [PATCH] Catch exception in filter chain and use it to set status in span When a controller throws an exception the servlet container will eventually set the response status to 500, but it is still 200 generally when the filter chain finishes, unless we catch the exception and do something with it. Fixes gh-57 --- .../sleuth/instrument/web/TraceFilter.java | 23 +++++++++++--- ...t.java => TraceAsyncIntegrationTests.java} | 4 +-- ....java => TraceFilterIntegartionTests.java} | 4 +-- .../instrument/web/TraceFilterTests.java | 31 +++++++++++++++++-- 4 files changed, 50 insertions(+), 12 deletions(-) rename spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/{TraceAsyncITest.java => TraceAsyncIntegrationTests.java} (96%) rename spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/{TraceFilterITest.java => TraceFilterIntegartionTests.java} (95%) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java index 864d1d454..45e01d6ed 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java @@ -143,12 +143,17 @@ public class TraceFilter extends OncePerRequestFilter request.setAttribute(TRACE_REQUEST_ATTR, trace); } + Throwable exception = null; try { addRequestAnnotations(request); filterChain.doFilter(request, response); } + catch (Throwable e) { + exception = e; + throw e; + } finally { if (isAsyncStarted(request) || request.isAsyncStarted()) { // TODO: how to deal with response annotations and async? @@ -158,8 +163,8 @@ public class TraceFilter extends OncePerRequestFilter addToResponseIfNotPresent(response, Trace.NOT_SAMPLED_NAME, ""); } if (trace != null) { + addResponseAnnotations(response, exception); addResponseHeaders(response, trace.getSpan()); - addResponseAnnotations(response); if (trace.getSavedTrace() != null) { publish(new ServerSentEvent(this, trace.getSavedTrace().getSpan(), trace.getSpan())); @@ -204,9 +209,17 @@ public class TraceFilter extends OncePerRequestFilter } } - private void addResponseAnnotations(HttpServletResponse response) { - this.traceManager.addAnnotation("/http/response/status_code", - String.valueOf(response.getStatus())); + private void addResponseAnnotations(HttpServletResponse response, Throwable e) { + if (response.getStatus() == HttpServletResponse.SC_OK && e != null) { + // Filter chain threw exception but the response status may not have been set + // yet, so we have to guess. + this.traceManager.addAnnotation("/http/response/status_code", + String.valueOf(HttpServletResponse.SC_INTERNAL_SERVER_ERROR)); + } + else { + this.traceManager.addAnnotation("/http/response/status_code", + String.valueOf(response.getStatus())); + } for (String name : response.getHeaderNames()) { for (String value : response.getHeaders(name)) { @@ -219,7 +232,7 @@ public class TraceFilter extends OncePerRequestFilter private String getHeader(HttpServletRequest request, HttpServletResponse response, String name) { String value = request.getHeader(name); - return value!=null ? value : response.getHeader(name); + return value != null ? value : response.getHeader(name); } private void addToResponseIfNotPresent(HttpServletResponse response, String name, diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncITest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncIntegrationTests.java similarity index 96% rename from spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncITest.java rename to spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncIntegrationTests.java index 896c2854e..8111f6f33 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncITest.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncIntegrationTests.java @@ -24,8 +24,8 @@ import com.jayway.awaitility.Awaitility; @RunWith(SpringJUnit4ClassRunner.class) @SpringApplicationConfiguration(classes = { - TraceAsyncITest.TraceAsyncITestConfiguration.class }) -public class TraceAsyncITest { + TraceAsyncIntegrationTests.TraceAsyncITestConfiguration.class }) +public class TraceAsyncIntegrationTests { @Autowired ClassPerformingAsyncLogic classPerformingAsyncLogic; @Autowired TraceManager traceManager; diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterITest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegartionTests.java similarity index 95% rename from spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterITest.java rename to spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegartionTests.java index 45fb1ea1f..8e77edf22 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterITest.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegartionTests.java @@ -19,9 +19,9 @@ import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; import org.springframework.test.web.servlet.setup.DefaultMockMvcBuilder; @RunWith(SpringJUnit4ClassRunner.class) -@SpringApplicationConfiguration(TraceFilterITest.class) +@SpringApplicationConfiguration(TraceFilterIntegartionTests.class) @DefaultTestAutoConfiguration -public class TraceFilterITest extends MvcITest { +public class TraceFilterIntegartionTests extends MvcITest { @Autowired TraceManager traceManager; diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java index 1a10466a9..1ae00dc8c 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java @@ -70,7 +70,7 @@ public class TraceFilterTests { new JdkIdGenerator(), this.publisher) { @Override protected Trace createTrace(Trace trace, Span span) { - TraceFilterTests.this.span= span; + TraceFilterTests.this.span = span; return super.createTrace(trace, span); } }; @@ -135,7 +135,33 @@ public class TraceFilterTests { assertNull(TraceContextHolder.getCurrentTrace()); } + @Test + public void catchesException() throws Exception { + TraceFilter filter = new TraceFilter(this.traceManager); + this.filterChain = new MockFilterChain() { + @Override + public void doFilter(javax.servlet.ServletRequest request, + javax.servlet.ServletResponse response) + throws java.io.IOException, javax.servlet.ServletException { + throw new RuntimeException("Planned"); + }; + }; + try { + filter.doFilter(this.request, this.response, this.filterChain); + } + catch (RuntimeException e) { + assertEquals("Planned", e.getMessage()); + } + verifyHttpAnnotations(HttpStatus.INTERNAL_SERVER_ERROR); + + assertNull(TraceContextHolder.getCurrentTrace()); + } + public void verifyHttpAnnotations() { + verifyHttpAnnotations(HttpStatus.OK); + } + + public void verifyHttpAnnotations(HttpStatus status) { hasAnnotation(this.span, "/http/request/uri", "http://localhost/"); hasAnnotation(this.span, "/http/request/endpoint", "/"); hasAnnotation(this.span, "/http/request/method", "GET"); @@ -143,8 +169,7 @@ public class TraceFilterTests { MediaType.APPLICATION_JSON_VALUE); hasAnnotation(this.span, "/http/request/headers/user-agent", "MockMvc"); - hasAnnotation(this.span, "/http/response/status_code", - HttpStatus.OK.toString()); + hasAnnotation(this.span, "/http/response/status_code", status.toString()); hasAnnotation(this.span, "/http/response/headers/content-type", MediaType.APPLICATION_JSON_VALUE); }