Skip to content
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

Servlet 3.0 instrumentation does not fully cover 3.1 and 4.0 #1083

Closed
mateuszrzeszutek opened this issue Aug 21, 2020 · 4 comments · Fixed by #1167
Closed

Servlet 3.0 instrumentation does not fully cover 3.1 and 4.0 #1083

mateuszrzeszutek opened this issue Aug 21, 2020 · 4 comments · Fixed by #1167
Assignees
Labels
bug Something isn't working

Comments

@mateuszrzeszutek
Copy link
Member

mateuszrzeszutek commented Aug 21, 2020

Servlet 3.1 introduced a new method in ServletOutputStream: setWriteListener(WriteListener). This method is not implemented in our current servlet 3.0 instrumentation, so when it's called an AbstractMethodError is thrown instead:

 org.jboss.resteasy.spi.UnhandledException: java.lang.AbstractMethodError: Receiver class io.opentelemetry.instrumentation.auto.servlet.v3_0.CountingHttpServletResponse$CountingServletOutputStream does not define or inherit an implementation of the resolved method 'abstract void setWriteListener(javax.servlet.WriteListener)' of abstract class javax.servlet.ServletOutputStream.
	at org.jboss.resteasy.core.ExceptionHandler.handleException(ExceptionHandler.java:381)
	at org.jboss.resteasy.core.SynchronousDispatcher.writeException(SynchronousDispatcher.java:216)
	at org.jboss.resteasy.core.SynchronousDispatcher.asynchronousExceptionDelivery(SynchronousDispatcher.java:571)
...
Caused by: java.lang.AbstractMethodError: Receiver class io.opentelemetry.instrumentation.auto.servlet.v3_0.CountingHttpServletResponse$CountingServletOutputStream does not define or inherit an implementation of the resolved method 'abstract void setWriteListener(javax.servlet.WriteListener)' of abstract class javax.servlet.ServletOutputStream.
	at org.jboss.resteasy.plugins.server.servlet.HttpServletResponseWrapper$DeferredOutputStream.queue(HttpServletResponseWrapper.java:204)
	at org.jboss.resteasy.plugins.server.servlet.HttpServletResponseWrapper$DeferredOutputStream.asyncWrite(HttpServletResponseWrapper.java:179)
	at org.jboss.resteasy.util.CommitHeaderAsyncOutputStream.asyncWrite(CommitHeaderAsyncOutputStream.java:94)
	at org.jboss.resteasy.spi.AsyncOutputStream.asyncWrite(AsyncOutputStream.java:23)
...

I ran into this issue when I was unit testing RESTEasy 4.5.6: it looks like they actually use this new method when dealing with an AsyncResponse: https://github.com/resteasy/Resteasy/blob/master/resteasy-core/src/main/java/org/jboss/resteasy/plugins/server/servlet/HttpServletResponseWrapper.java#L204
This issue is easy to reproduce by using code from this PR and running

./gradlew :instrumentation:jaxrs:jaxrs-2.0:jaxrs-2.0-resteasy-3.1:test -PtestLatestDeps=true

The worst thing about this is that it affects the instrumented application (because we overwrite request & response with our wrappers) - in my case, I got a 500 HTTP response instead of 1 200 one:

Response{protocol=http/1.1, code=500, message=Internal Server Error, url=http://localhost:56412/async?action=succeed}
@trask trask added bug Something isn't working priority:p1 labels Aug 21, 2020
@mateuszrzeszutek
Copy link
Member Author

I've started working on this issue, PR will be ready this week for sure

@iNikem
Copy link
Contributor

iNikem commented Sep 2, 2020

I see this task, #1096, the complexity of PRs trying to solve this task and I am wondering if we should rollback CountingHttpServletResponse and come up with some other way to set those 2 semantic convention attributes...

@trask @anuraaga

@mateuszrzeszutek
Copy link
Member Author

I agree -- I've tried to fix #1149 by creating a proxy ServletOutputStream class in runtime (so that we don't have to hard-code the usage of WriteListener) but this solution is also starting to look pretty crazy.

@mateuszrzeszutek
Copy link
Member Author

mateuszrzeszutek commented Sep 3, 2020

I've submitted a PR that removes CountingOutputStream: #1167

I'll close the other two, since they both seem to be inane solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment