SpringBoot Web MVC metrics may have the wrong status when a filter throws an exception other than NestedServletException

Hi, I have a question, this might be a bug or maybe I don't know sth. My question concerns these lines from WebMvcMetricsFilter class:

catch (NestedServletException 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);
    throw ex;
}

Why for NestedServletException the response status is set to INTERNAL_SERVER_ERROR but for the latter exceptions (ServletException | IOException | RuntimeException) it is not? I had a case where one of my custom filters was throwing I/O error (ResourceAccessException with IOException as a cause), resulting in 500 response from an endpoint, but in metrics it wasn't recorded with status 500, but rather with status 200 - because it got caught by the second catch clause.
Such behaviour may lead to incorrect metrics and undetected issues in applications.

Comment From: mbhave

This issue added the second catch block. I think we set the status in the first catch in anticipation of what the servlet container would do if the exception got to it. This way we can match the status code in the metric to the status code of the actual response. I am not sure why we don't do that in the second catch as well. Flagging for team attention to get the rest of the team's thoughts.

There's another issue related to handling of the 500 response status, although it is slightly different.

Comment From: wilkinsona

The setStatus call was added as part of a Micrometer upgrade which was aligning Boot with this change made in the equivalent Micrometer code made for https://github.com/micrometer-metrics/micrometer/pull/264.

I don't think there's a good reason to treat the two catch blocks differently as they both re-throw the exception. If any exception reaches the servlet container it will set the status to 500 (as long as the response has not already been committed). https://github.com/spring-projects/spring-boot/issues/13300, that @mbhave linked to above, will still be a problem, but I don't think that should stop us from making things consistent in the two catch blocks inWebMvcMetricsFilter.

Comment From: wilkinsona

Based on the above, I think that these changes will fix the problem.

Comment From: tmachows

Thanks for your replies. The change looks good IMO @wilkinsona 👍