From 1960666765a6bf93500f3bf8f2eeb29e428b68e9 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 18 Nov 2022 14:15:49 +0100 Subject: [PATCH] Set error status in Observation Servlet filter Prior to this commit, the Observation Servlet filter would record unhandled exceptions on the observation context but would leave the default HTTP response status as is. Servlet containers do set the response status in that case to 500 by default. Not doing that at the Servlet filter level results in invalid observations, stating that the HTTP response status is 200 (because the error status hasn't been set yet by the container) and as a result, the outcome is SUCCESS. This commit ensures that the error status is set in those cases, aligning the behavior with Servlet containers. Fixes gh-29512 --- .../web/filter/ServerHttpObservationFilter.java | 4 +++- .../filter/ServerHttpObservationFilterTests.java | 13 ++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/filter/ServerHttpObservationFilter.java b/spring-web/src/main/java/org/springframework/web/filter/ServerHttpObservationFilter.java index d0be09619f..3ca50f609b 100644 --- a/spring-web/src/main/java/org/springframework/web/filter/ServerHttpObservationFilter.java +++ b/spring-web/src/main/java/org/springframework/web/filter/ServerHttpObservationFilter.java @@ -27,6 +27,7 @@ import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.springframework.http.HttpStatus; import org.springframework.http.server.observation.DefaultServerRequestObservationConvention; import org.springframework.http.server.observation.ServerHttpObservationDocumentation; import org.springframework.http.server.observation.ServerRequestObservationContext; @@ -108,7 +109,8 @@ public class ServerHttpObservationFilter extends OncePerRequestFilter { filterChain.doFilter(request, response); } catch (Exception ex) { - observation.error(unwrapServletException(ex)).stop(); + observation.error(unwrapServletException(ex)); + response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR.value()); throw ex; } finally { diff --git a/spring-web/src/test/java/org/springframework/web/filter/ServerHttpObservationFilterTests.java b/spring-web/src/test/java/org/springframework/web/filter/ServerHttpObservationFilterTests.java index 30648c8bfb..354e5c50a4 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/ServerHttpObservationFilterTests.java +++ b/spring-web/src/test/java/org/springframework/web/filter/ServerHttpObservationFilterTests.java @@ -97,7 +97,18 @@ class ServerHttpObservationFilterTests { ServerRequestObservationContext context = (ServerRequestObservationContext) this.request .getAttribute(ServerHttpObservationFilter.CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE); assertThat(context.getError()).isEqualTo(customError); - assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "SUCCESS"); + assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "SERVER_ERROR"); + } + + @Test + void filterShouldSetDefaultErrorStatusForBubblingExceptions() { + assertThatThrownBy(() -> { + this.filter.doFilter(this.request, this.response, (request, response) -> { + throw new ServletException(new IllegalArgumentException("custom error")); + }); + }).isInstanceOf(ServletException.class); + assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "SERVER_ERROR") + .hasLowCardinalityKeyValue("status", "500"); } private TestObservationRegistryAssert.TestObservationRegistryAssertReturningObservationContextAssert assertThatHttpObservation() {