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

Update OTel snapshot to 723 for recordException #786

Merged
merged 15 commits into from
Jul 27, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ public void setStatus(final Status status) {
delegate.setStatus(status);
}

@Override
public void recordException(Throwable throwable) {
delegate.recordException(throwable);
}

@Override
public void updateName(final String name) {
delegate.updateName(name);
Expand Down
14 changes: 8 additions & 6 deletions gradle/dependencies.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ configurations.all {
ext {
versions = [
// Check https://oss.jfrog.org/libs-snapshot/io/opentelemetry/ for latest snapshot version.
opentelemetry: '0.7.0-20200721.021000-20',
opentelemetry: '0.7.0-20200723.154929-27',
// Snapshot versions can often get split into two suffixes
opentelemetryOther: '0.7.0-20200723.154929-26',

slf4j : "1.7.30",
guava : "20.0", // Last version to support Java 7
Expand All @@ -26,13 +28,13 @@ ext {
deps = [
// OpenTelemetry
opentelemetryApi : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-api', version: versions.opentelemetry),
opentelemetryApiAutoAnnotations: dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-extension-auto-annotations', version: versions.opentelemetry),
opentelemetryTraceProps : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-extension-trace-propagators', version: versions.opentelemetry),
opentelemetryApiAutoAnnotations: dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-extension-auto-annotations', version: versions.opentelemetryOther),
opentelemetryTraceProps : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-extension-trace-propagators', version: versions.opentelemetryOther),
opentelemetrySdk : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-sdk', version: versions.opentelemetry),
opentelemetrySdkAutoConfig : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-sdk-extension-auto-config', version: versions.opentelemetry),
opentelemetrySdkAutoConfig : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-sdk-extension-auto-config', version: versions.opentelemetryOther),
opentelemetryJaeger : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-exporters-jaeger', version: versions.opentelemetry),
opentelemetryOtlp : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-exporters-otlp', version: versions.opentelemetry),
opentelemetryZipkin : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-exporters-zipkin', version: versions.opentelemetry),
opentelemetryOtlp : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-exporters-otlp', version: versions.opentelemetryOther),
opentelemetryZipkin : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-exporters-zipkin', version: versions.opentelemetryOther),

// General
slf4j : "org.slf4j:slf4j-api:${versions.slf4j}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,6 @@ public void setPrincipal(Span span, HttpServletRequest request) {
}

public void setContentLength(Span span, int length) {
SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.set(span, String.valueOf(length));
SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.set(span, length);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class DropwizardTest extends HttpServerTest<DropwizardTestSupport> {
"${SemanticAttributes.HTTP_METHOD.key()}" method
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" endpoint.status
// exception bodies are not yet recorded
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" { "${responseContentLength ?: 0}" || endpoint == EXCEPTION }
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" { it == responseContentLength || /* async */ it == null }
"servlet.context" ""
"servlet.path" ""
if (endpoint.errored) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import io.opentelemetry.trace.attributes.SemanticAttributes

import java.util.concurrent.TimeoutException

import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.SUCCESS
import static io.opentelemetry.trace.Span.Kind.INTERNAL
Expand Down Expand Up @@ -117,7 +116,6 @@ class FinatraServerTest extends HttpServerTest<HttpServer> {
"${SemanticAttributes.HTTP_METHOD.key()}" method
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" endpoint.status
// exception bodies are not yet recorded
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" { "${responseContentLength ?: 0}" || endpoint == EXCEPTION }
if (endpoint.query) {
"$MoreAttributes.HTTP_QUERY" endpoint.query
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,14 @@
import static io.opentelemetry.auto.instrumentation.jetty.JettyHttpServerTracer.TRACER;

import io.grpc.Context;
import io.opentelemetry.auto.instrumentation.servlet.v3_0.CountingHttpServletRequest;
import io.opentelemetry.auto.instrumentation.servlet.v3_0.CountingHttpServletResponse;
import io.opentelemetry.auto.instrumentation.servlet.v3_0.TagSettingAsyncListener;
import io.opentelemetry.context.Scope;
import io.opentelemetry.trace.Span;
import java.lang.reflect.Method;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import net.bytebuddy.asm.Advice;
Expand All @@ -34,18 +37,24 @@ public class JettyHandlerAdvice {
public static void onEnter(
@Advice.Origin final Method method,
@Advice.This final Object source,
@Advice.Argument(2) final HttpServletRequest httpServletRequest,
@Advice.Argument(value = 2, readOnly = false) HttpServletRequest request,
@Advice.Argument(value = 3, readOnly = false) HttpServletResponse response,
@Advice.Local("otelSpan") Span span,
@Advice.Local("otelScope") Scope scope) {

Context attachedContext = TRACER.getServerContext(httpServletRequest);
Context attachedContext = TRACER.getServerContext(request);
if (attachedContext != null) {
// We are inside nested handler, don't create new span
return;
}

span = TRACER.startSpan(httpServletRequest, httpServletRequest, method);
scope = TRACER.startScope(span, httpServletRequest);
span = TRACER.startSpan(request, request, method);
scope = TRACER.startScope(span, request);

if (!(response instanceof CountingHttpServletResponse)) {
response = new CountingHttpServletResponse(response);
request = new CountingHttpServletRequest(request, response);
}
}

@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
Expand Down Expand Up @@ -86,7 +95,14 @@ public static void stopSpan(

// Check again in case the request finished before adding the listener.
if (!request.isAsyncStarted() && responseHandled.compareAndSet(false, true)) {
contentLengthHelper(span, response);
TRACER.end(span, response.getStatus());
}
}

public static void contentLengthHelper(Span span, ServletResponse response) {
if (response instanceof CountingHttpServletResponse) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice JettyHandler didn't seem to have implemented counting, so thought it'd be a quick fix to add it, but some conflict between unnamed classloader and app classloader here :( Tomorrow will give another small look and then revert this and remove the assertion for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it was because I was missing this advice from the helper classes (was referring to servlet-3.0 while writing this). Filed #796 since this was confusing.

TRACER.setContentLength(span, ((CountingHttpServletResponse) response).getContentLength());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public String[] helperClassNames() {
"io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer",
"io.opentelemetry.auto.instrumentation.servlet.v3_0.Servlet3HttpServerTracer",
"io.opentelemetry.auto.instrumentation.servlet.v3_0.TagSettingAsyncListener",
"io.opentelemetry.auto.instrumentation.servlet.v3_0.TagSettingAsyncListener",
"io.opentelemetry.auto.instrumentation.servlet.v3_0.CountingHttpServletRequest",
"io.opentelemetry.auto.instrumentation.servlet.v3_0.CountingHttpServletResponse",
"io.opentelemetry.auto.instrumentation.servlet.v3_0.CountingHttpServletResponse$CountingServletOutputStream",
"io.opentelemetry.auto.instrumentation.servlet.v3_0.CountingHttpServletResponse$CountingPrintWriter",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import io.opentelemetry.auto.instrumentation.api.MoreAttributes
import io.opentelemetry.auto.test.asserts.TraceAssert
import io.opentelemetry.auto.test.base.HttpServerTest
import io.opentelemetry.trace.attributes.SemanticAttributes
import spock.lang.Shared

import javax.servlet.DispatcherType
import javax.servlet.ServletException
Expand All @@ -38,7 +39,8 @@ import static io.opentelemetry.trace.Span.Kind.SERVER

class JettyHandlerTest extends HttpServerTest<Server> {

static errorHandler = new ErrorHandler() {
@Shared
ErrorHandler errorHandler = new ErrorHandler() {
@Override
protected void handleErrorPage(HttpServletRequest request, Writer writer, int code, String message) throws IOException {
Throwable th = (Throwable) request.getAttribute("javax.servlet.error.exception")
Expand All @@ -49,6 +51,9 @@ class JettyHandlerTest extends HttpServerTest<Server> {
}
}

@Shared
TestHandler testHandler = new TestHandler()

@Override
Server startServer(int port) {
def server = new Server(port)
Expand All @@ -59,7 +64,7 @@ class JettyHandlerTest extends HttpServerTest<Server> {
}

AbstractHandler handler() {
TestHandler.INSTANCE
testHandler
}

@Override
Expand Down Expand Up @@ -102,8 +107,6 @@ class JettyHandlerTest extends HttpServerTest<Server> {
}

static class TestHandler extends AbstractHandler {
static final TestHandler INSTANCE = new TestHandler()

@Override
void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
if (baseRequest.dispatcherType != DispatcherType.ERROR) {
Expand Down Expand Up @@ -134,7 +137,7 @@ class JettyHandlerTest extends HttpServerTest<Server> {
"${SemanticAttributes.HTTP_METHOD.key()}" method
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" endpoint.status
// exception bodies are not yet recorded
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" { "${responseContentLength ?: 0}" || endpoint == EXCEPTION }
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" responseContentLength
"servlet.path" ''
if (endpoint.errored) {
"error.msg" { it == null || it == EXCEPTION.body }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/$jspFileName"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 200
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/$jspFileName"
}
Expand Down Expand Up @@ -175,7 +175,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/getQuery.jsp?$queryString"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 200
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/getQuery.jsp"
}
Expand Down Expand Up @@ -233,7 +233,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/post.jsp"
"${SemanticAttributes.HTTP_METHOD.key()}" "POST"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 200
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/post.jsp"
}
Expand Down Expand Up @@ -288,7 +288,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/$jspFileName"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 500
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/$jspFileName"
"error.type" { String tagExceptionType ->
Expand Down Expand Up @@ -362,7 +362,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/includes/includeHtml.jsp"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 200
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/includes/includeHtml.jsp"
}
Expand Down Expand Up @@ -416,7 +416,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/includes/includeMulti.jsp"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 200
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/includes/includeMulti.jsp"
}
Expand Down Expand Up @@ -508,7 +508,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/$jspFileName"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 500
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/$jspFileName"
errorAttributes(JasperException, String)
Expand Down Expand Up @@ -564,7 +564,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/$staticFile"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 200
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/$staticFile"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class JSPInstrumentationForwardTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/$forwardFromFileName"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 200
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/$forwardFromFileName"
}
Expand Down Expand Up @@ -191,7 +191,7 @@ class JSPInstrumentationForwardTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/forwards/forwardToHtml.jsp"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 200
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/forwards/forwardToHtml.jsp"
}
Expand Down Expand Up @@ -245,7 +245,7 @@ class JSPInstrumentationForwardTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/forwards/forwardToIncludeMulti.jsp"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 200
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/forwards/forwardToIncludeMulti.jsp"
}
Expand Down Expand Up @@ -359,7 +359,7 @@ class JSPInstrumentationForwardTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/forwards/forwardToJspForward.jsp"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 200
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/forwards/forwardToJspForward.jsp"
}
Expand Down Expand Up @@ -453,7 +453,7 @@ class JSPInstrumentationForwardTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/forwards/forwardToCompileError.jsp"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 500
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/forwards/forwardToCompileError.jsp"
errorAttributes(JasperException, String)
Expand Down Expand Up @@ -520,7 +520,7 @@ class JSPInstrumentationForwardTests extends AgentTestRunner {
"${SemanticAttributes.HTTP_URL.key()}" "http://localhost:$port/$jspWebappContext/forwards/forwardToNonExistent.jsp"
"${SemanticAttributes.HTTP_METHOD.key()}" "GET"
"${SemanticAttributes.HTTP_STATUS_CODE.key()}" 404
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" String
"${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" Long
"servlet.context" "/$jspWebappContext"
"servlet.path" "/forwards/forwardToNonExistent.jsp"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ public void setStatus(final Status status) {
}
}

@Override
public void recordException(Throwable throwable) {
shadedSpan.recordException(throwable);
}

@Override
public void updateName(final String name) {
shadedSpan.updateName(name);
Expand Down
Loading