Skip to content

Commit

Permalink
Set error status in Observation Servlet filter
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bclozel committed Nov 18, 2022
1 parent 45dc1d2 commit 1960666
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 2 deletions.
Expand Up @@ -27,6 +27,7 @@
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;
Expand Down Expand Up @@ -108,7 +109,8 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
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 {
Expand Down
Expand Up @@ -97,7 +97,18 @@ void filterShouldUnwrapServletException() {
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() {
Expand Down

0 comments on commit 1960666

Please sign in to comment.