-
Notifications
You must be signed in to change notification settings - Fork 791
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
Tomcat server handlers #1902
Tomcat server handlers #1902
Conversation
…g to mimic it from the server handler.
...nt/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/ServerHandlerAdvice.java
Outdated
Show resolved
Hide resolved
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.
smoke-tests/src/test/groovy/io/opentelemetry/smoketest/TomcatSmokeTest.groovy
Show resolved
Hide resolved
instrumentation/tomcat-7.0/javaagent/tomcat-7.0-javaagent.gradle
Outdated
Show resolved
Hide resolved
...nt/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/ServerHandlerAdvice.java
Show resolved
Hide resolved
...nt/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/ServerHandlerAdvice.java
Outdated
Show resolved
Hide resolved
...nt/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/ServerHandlerAdvice.java
Show resolved
Hide resolved
...nt/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java
Outdated
Show resolved
Hide resolved
...entation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java
Outdated
Show resolved
Hide resolved
I think so. This is the trace I get from the demo case in that ticket in my branch: And there is also an unhandled servlet exception reported in the trace: |
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.
I don't like AppServerBridge
at all. I have no better suggestions right now, but I will think about it...
...entation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java
Outdated
Show resolved
Hide resolved
...nt/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/ServerHandlerAdvice.java
Outdated
Show resolved
Hide resolved
...nt/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java
Show resolved
Hide resolved
...javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatTracer.java
Outdated
Show resolved
Hide resolved
...entation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java
Show resolved
Hide resolved
…pServerBridge to the ServletHttpServerTracer.
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.
👍
...vlet-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java
Show resolved
Hide resolved
...nt/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java
Outdated
Show resolved
Hide resolved
...nt/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/ServerHandlerAdvice.java
Outdated
Show resolved
Hide resolved
...nt/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/ServerHandlerAdvice.java
Show resolved
Hide resolved
...javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatTracer.java
Outdated
Show resolved
Hide resolved
instrumentation/tomcat-7.0/javaagent/tomcat-7.0-javaagent.gradle
Outdated
Show resolved
Hide resolved
* Tomcat server handler with passing integration and smoke tests. * Update server span name from the servlet integration instead of trying to mimic it from the server handler. * Cleanup and more javadocs. * Use Java8BytecodeBridge in advice code. * Use Java8BytecodeBridge in advice code. * Use earliest version for compilation and muzzling. * Use consistent instrumentation name. * Record throwables only if the context is managed by something else. * Getting scope handling right. Moved server span renaming logic via AppServerBridge to the ServletHttpServerTracer. * codenarc * more PR comments addressed * Pass Method to startServerSpan
This is a Tomcat version of a server handler, as described and discussed in #1886
Some notable findings:
Server instrumentation needs uncaught exceptions from outermost servlets/filters. These are passed viaOutermost servlets/filters set their unhandled exceptions with the current span.AppServerBridge.THROWABLE_CONTEXT_KEY
context attribute.AppServerBridge
context attribute and not the intuitivecallDepth
counter.