Fix Status Code in RequestTelemetry when Servlet Throws Exception #603
Conversation
…icationInsightsHttpResponseWrapper
|
All broken tests are fixed. It needed Servlet API as testCompile also. |
littleaj
left a comment
littleaj
left a comment
There was a problem hiding this comment.
I'm going to write some integration tests for this and commit them to this branch.
| //later in the the chain inside the catch block of org.apache.catalina.core.StandardWrapperValue class | ||
| //The call to this method happens after the Filter Chain has been completed and thus response object | ||
| //can never have this value at this time. | ||
| response.setStatus(500); |
There was a problem hiding this comment.
We can't do this. We must discover what the app is returning to the client.
There was a problem hiding this comment.
@littleaj if you have better alternatives I am happy to take that. But in all my test I did not found any way to control this behavior. Filter chain gets released way before 500 is set.
AFAIK it's easy to verify this by checking the source code of the StandardWrapperValue these three exceptions will always lead to "500". Every other response code is set before the filter sets the value so I do not think that we would be doing any harm here.
There was a problem hiding this comment.
In case of non-tomcat app servers where some other class will govern setting this value, it would also follow the same patter of setting 500. Now ordering might change but the value of response code should be the same in any app server regardless for this three exceptions.
|
Please take a look at the last comment here : https://stackoverflow.com/questions/20772837/java-servlet-throw-http-500-error-on-purpose |
|
@littleaj I got your explanation now. As a work around as discussed we can set the resultcode to un known and the success = false. Ideally as suggested we might need to hook at lower level then filters. We can rest of the goodies from the branch like deprecating ApplicationInsightsHttpResponseWrapper |
|
Closing as we are researching on better alternatives. |
This PR fixes the issue where the where the
WebRequestTelemetryFilterstamps the response code as 200 OK when the Servlet Throws.The fix includes explicitly setting the response code as 500 on the
RequestTelemetryitem when filter catchesRunTimeException,IOExceptionorServletException. This needs to be set explicitly because the response code officially get's set fromorg.apache.catalina.core. StandardWrapparValueclass'sinvoke()method in it's catch block. This method is called way later after the FilterChain completes it's execution.This PR also Deprecates the
ApplicationInsightsHttpResponseWrapperclass and all the places where casting was done to this, as we no longer require this class as of Servlet API 3.0.1 onwards. It already hasgetStatus()method for which this wrapper was designed.Fix #600
For significant contributions please make sure you have completed the following items: