From c2361aeb04b0871c97365d12f732e1a524ecfea4 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Thu, 16 Sep 2021 11:36:02 +0100 Subject: [PATCH] Make WebMvcMetricsFilter set status consistently for all exceptions Closes gh-27988 --- .../web/servlet/WebMvcMetricsFilter.java | 8 +-- .../web/servlet/WebMvcMetricsFilterTests.java | 55 +++++++++++++------ 2 files changed, 39 insertions(+), 24 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilter.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilter.java index b57baed75fd7..f604f493c73a 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilter.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilter.java @@ -104,13 +104,9 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse record(timingContext, request, response, exception); } } - catch (NestedServletException ex) { + catch (Exception ex) { response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR.value()); - record(timingContext, request, response, ex.getCause()); - throw ex; - } - catch (ServletException | IOException | RuntimeException ex) { - record(timingContext, request, response, ex); + record(timingContext, request, response, (ex instanceof NestedServletException) ? ex.getCause() : ex); throw ex; } } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java index db2af3bd3e9b..54aa8e80c56c 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java @@ -38,6 +38,7 @@ import io.micrometer.core.annotation.Timed; import io.micrometer.core.instrument.Clock; import io.micrometer.core.instrument.Meter; +import io.micrometer.core.instrument.Meter.Id; import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.MockClock; import io.micrometer.core.instrument.Tag; @@ -126,8 +127,8 @@ class WebMvcMetricsFilterTests { @BeforeEach void setupMockMvc() { - this.mvc = MockMvcBuilders.webAppContextSetup(this.context) - .addFilters(this.filter, new RedirectAndNotFoundFilter()).build(); + this.mvc = MockMvcBuilders.webAppContextSetup(this.context).addFilters(this.filter, new CustomBehaviorFilter()) + .build(); } @Test @@ -164,7 +165,7 @@ void badClientRequest() throws Exception { @Test void redirectRequest() throws Exception { - this.mvc.perform(get("/api/redirect").header(RedirectAndNotFoundFilter.TEST_MISBEHAVE_HEADER, "302")) + this.mvc.perform(get("/api/redirect").header(CustomBehaviorFilter.TEST_STATUS_HEADER, "302")) .andExpect(status().is3xxRedirection()); assertThat(this.registry.get("http.server.requests").tags("uri", "REDIRECTION").tags("status", "302").timer()) .isNotNull(); @@ -172,7 +173,7 @@ void redirectRequest() throws Exception { @Test void notFoundRequest() throws Exception { - this.mvc.perform(get("/api/not/found").header(RedirectAndNotFoundFilter.TEST_MISBEHAVE_HEADER, "404")) + this.mvc.perform(get("/api/not/found").header(CustomBehaviorFilter.TEST_STATUS_HEADER, "404")) .andExpect(status().is4xxClientError()); assertThat(this.registry.get("http.server.requests").tags("uri", "NOT_FOUND").tags("status", "404").timer()) .isNotNull(); @@ -186,13 +187,23 @@ void unhandledError() { .isEqualTo(1L); } + @Test + void unhandledServletException() { + assertThatCode(() -> this.mvc + .perform(get("/api/filterError").header(CustomBehaviorFilter.TEST_SERVLET_EXCEPTION_HEADER, "throw")) + .andExpect(status().isOk())).isInstanceOf(ServletException.class); + Id meterId = this.registry.get("http.server.requests").tags("exception", "ServletException").timer().getId(); + assertThat(meterId.getTag("status")).isEqualTo("500"); + } + @Test void streamingError() throws Exception { MvcResult result = this.mvc.perform(get("/api/c1/streamingError")).andExpect(request().asyncStarted()) .andReturn(); assertThatIOException().isThrownBy(() -> this.mvc.perform(asyncDispatch(result)).andReturn()); - assertThat(this.registry.get("http.server.requests").tags("exception", "IOException").timer().count()) - .isEqualTo(1L); + Id meterId = this.registry.get("http.server.requests").tags("exception", "IOException").timer().getId(); + // Response is committed before error occurs so status is 200 (OK) + assertThat(meterId.getTag("status")).isEqualTo("200"); } @Test @@ -208,8 +219,10 @@ void anonymousError() { } catch (Throwable ignore) { } - assertThat(this.registry.get("http.server.requests").tag("uri", "/api/c1/anonymousError/{id}").timer().getId() - .getTag("exception")).endsWith("$1"); + Id meterId = this.registry.get("http.server.requests").tag("uri", "/api/c1/anonymousError/{id}").timer() + .getId(); + assertThat(meterId.getTag("exception")).endsWith("$1"); + assertThat(meterId.getTag("status")).isEqualTo("500"); } @Test @@ -357,8 +370,8 @@ public MeterFilterReply accept(@NonNull Meter.Id id) { } @Bean - RedirectAndNotFoundFilter redirectAndNotFoundFilter() { - return new RedirectAndNotFoundFilter(); + CustomBehaviorFilter redirectAndNotFoundFilter() { + return new CustomBehaviorFilter(); } @Bean(name = "callableBarrier") @@ -472,8 +485,10 @@ String alwaysThrowsUnhandledException(@PathVariable Long id) { } @GetMapping("/streamingError") - ResponseBodyEmitter streamingError() { + ResponseBodyEmitter streamingError() throws IOException { ResponseBodyEmitter emitter = new ResponseBodyEmitter(); + emitter.send("some data"); + emitter.send("some more data"); emitter.completeWithError(new IOException("error while writing to the response")); return emitter; } @@ -522,20 +537,24 @@ String successful(@PathVariable Long id) { } - static class RedirectAndNotFoundFilter extends OncePerRequestFilter { + static class CustomBehaviorFilter extends OncePerRequestFilter { + + static final String TEST_STATUS_HEADER = "x-test-status"; - static final String TEST_MISBEHAVE_HEADER = "x-test-misbehave-status"; + static final String TEST_SERVLET_EXCEPTION_HEADER = "x-test-servlet-exception"; @Override protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { - String misbehave = request.getHeader(TEST_MISBEHAVE_HEADER); - if (misbehave != null) { - response.setStatus(Integer.parseInt(misbehave)); + String misbehaveStatus = request.getHeader(TEST_STATUS_HEADER); + if (misbehaveStatus != null) { + response.setStatus(Integer.parseInt(misbehaveStatus)); + return; } - else { - filterChain.doFilter(request, response); + if (request.getHeader(TEST_SERVLET_EXCEPTION_HEADER) != null) { + throw new ServletException(); } + filterChain.doFilter(request, response); } }