-
Notifications
You must be signed in to change notification settings - Fork 871
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Servlet instrumentation for servlet-api 3.1+ - remove output stream wrapper #1167
Fix Servlet instrumentation for servlet-api 3.1+ - remove output stream wrapper #1167
Conversation
99eca84
to
e48396c
Compare
instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy
Outdated
Show resolved
Hide resolved
cd90a0b
to
9386e20
Compare
Can you add a unit test that succeeds now but if the wrapped response were to be used would fail so that there will be no regressions? |
I can do a simple |
Taking into account #1096 (comment) and #1083 (comment) I propose to completely remove generic response counting support from servlet instrumentation. I feel bad about this, but if we are left with a partial solution (as @FrankSpitulski noticed above "it misses spring boot's default configuration"), then added complexity is not worth it. History has shown, that this is not as easy as we would like :( Unless somebody has a good proposal how to solve this otherwise, I ask @mateuszrzeszutek to remove |
9386e20
to
f1b01d9
Compare
f1b01d9
to
5341441
Compare
OK, I've removed all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mateuszrzeszutek!
@FrankSpitulski Really sorry about this >< Thanks a lot for the help and giving us the opportunity to think through some of the problems regards to instrumentation here, we may not have noticed that Muzzle needs additional checks related to base-class matching without this exercise so will hopefully get better.
There's an issue for that now: #1123 . I have an idea how to do implement that, so hopefully this will get done soon and we'll avoid similar issues in the future. |
Fixes #1083
The third time's the charm, I hope. This PR contains removal of the
ServletoutputStream
wrapper according to this comment.This might be not so bad anyway, because according to
ServletResponse#getOutputStream()
JavaDoc:so only those implementations that use
getOutputStream()
will get inaccurate content sizes (always 0), while those usinggetWriter()
will be perfectly fine.