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

[tmachows]

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.

[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.

[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 micrometer-metrics/micrometer#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). #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.

[wilkinsona]

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

[tmachows]

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