From 303d8f427fe4afce939ce0c8b1491aaac3669036 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Mon, 3 Aug 2020 12:52:55 -0700 Subject: [PATCH] Add response generic parameter to HttpServerTracer (#870) --- .../decorator/HttpServerTracer.java | 66 ++++++++++++------- .../servlet/ServletHttpServerTracer.java | 9 +-- .../SpringWebMvcServerTracer.java | 9 ++- .../springwebmvc/WebMVCTracingFilter.java | 4 +- .../AkkaHttpServerInstrumentation.java | 4 +- .../akkahttp/AkkaHttpServerTracer.java | 9 ++- .../v1_0/server/ArmeriaServerTracer.java | 8 ++- .../v1_0/server/OpenTelemetryService.java | 6 +- .../grizzly/GrizzlyHttpServerTracer.java | 9 ++- .../grizzly/HttpServerFilterAdvice.java | 2 +- .../jetty/JettyHandlerAdvice.java | 5 +- .../jetty/JettyHttpServerTracer.java | 9 --- .../HttpServerResponseTracingHandler.java | 2 +- .../v3_8/server/NettyHttpServerTracer.java | 8 ++- .../HttpServerResponseTracingHandler.java | 2 +- .../v4_0/server/NettyHttpServerTracer.java | 9 ++- .../HttpServerResponseTracingHandler.java | 2 +- .../v4_1/server/NettyHttpServerTracer.java | 9 ++- .../servlet/v2_2/ResponseWithStatus.java | 38 +++++++++++ .../servlet/v2_2/Servlet2Advice.java | 6 +- .../v2_2/Servlet2HttpServerTracer.java | 7 +- .../servlet/v2_2/Servlet2Instrumentation.java | 1 + .../servlet/v3_0/Servlet3Advice.java | 12 +--- .../v3_0/Servlet3HttpServerTracer.java | 39 ++++++++++- .../servlet/v3_0/TagSettingAsyncListener.java | 19 +----- 25 files changed, 201 insertions(+), 93 deletions(-) create mode 100644 instrumentation/servlet/servlet-2.2/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_2/ResponseWithStatus.java diff --git a/agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerTracer.java b/agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerTracer.java index ca994e8bbc..63134524eb 100644 --- a/agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerTracer.java +++ b/agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerTracer.java @@ -39,7 +39,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; // TODO In search for a better home package -public abstract class HttpServerTracer extends BaseTracer { +public abstract class HttpServerTracer extends BaseTracer { private static final Logger log = LoggerFactory.getLogger(HttpServerTracer.class); @@ -93,39 +93,49 @@ public abstract class HttpServerTracer extends Bas return withScopedContext(newContext); } + /** + * Convenience method. Delegates to {@link #end(Span, Object, long)}, passing {@code timestamp} + * value of {@code -1}. + */ // TODO should end methods remove SPAN attribute from request as well? - public void end(Span span, int responseStatus) { - end(span, responseStatus, -1); + public void end(Span span, RESPONSE response) { + end(span, response, -1); } // TODO should end methods remove SPAN attribute from request as well? - public void end(Span span, int responseStatus, long timestamp) { - setStatus(span, responseStatus); - if (timestamp >= 0) { - span.end(EndSpanOptions.builder().setEndTimestamp(timestamp).build()); - } else { - span.end(); - } + public void end(Span span, RESPONSE response, long timestamp) { + setStatus(span, responseStatus(response)); + endSpan(span, timestamp); } - /** Ends given span exceptionally with default response status code 500. */ + /** + * Convenience method. Delegates to {@link #endExceptionally(Span, Throwable, RESPONSE)}, passing + * {@code response} value of {@code null}. + */ public void endExceptionally(Span span, Throwable throwable) { - endExceptionally(span, throwable, 500); + endExceptionally(span, throwable, null); } - public void endExceptionally(Span span, Throwable throwable, int responseStatus) { - endExceptionally(span, throwable, responseStatus, -1); + /** + * Convenience method. Delegates to {@link #endExceptionally(Span, Throwable, RESPONSE, long)}, + * passing {@code timestamp} value of {@code -1}. + */ + public void endExceptionally(Span span, Throwable throwable, RESPONSE response) { + endExceptionally(span, throwable, response, -1); } - public void endExceptionally(Span span, Throwable throwable, int responseStatus, long timestamp) { - if (responseStatus == 200) { - // TODO I think this is wrong. - // We must report that response status that was actually sent to end user - // We may change span status, but not http_status attribute - responseStatus = 500; - } + /** + * If {@code response} is {@code null}, the {@code http.status_code} will be set to {@code 500} + * and the {@link Span} status will be set to {@link io.opentelemetry.trace.Status#INTERNAL}. + */ + public void endExceptionally(Span span, Throwable throwable, RESPONSE response, long timestamp) { onError(span, unwrapThrowable(throwable)); - end(span, responseStatus, timestamp); + if (response == null) { + setStatus(span, 500); + } else { + setStatus(span, responseStatus(response)); + } + endSpan(span, timestamp); } public Span getServerSpan(STORAGE storage) { @@ -267,12 +277,20 @@ public abstract class HttpServerTracer extends Bas return span.getContext(); } - private void setStatus(Span span, int status) { + private static void setStatus(Span span, int status) { SemanticAttributes.HTTP_STATUS_CODE.set(span, status); // TODO status_message span.setStatus(HttpStatusConverter.statusFromHttpStatus(status)); } + private static void endSpan(Span span, long timestamp) { + if (timestamp >= 0) { + span.end(EndSpanOptions.builder().setEndTimestamp(timestamp).build()); + } else { + span.end(); + } + } + protected abstract Integer peerPort(CONNECTION connection); protected abstract String peerHostIP(CONNECTION connection); @@ -287,6 +305,8 @@ public abstract class HttpServerTracer extends Bas protected abstract String requestHeader(REQUEST request, String name); + protected abstract int responseStatus(RESPONSE response); + /** * Stores given context in the given request-response-loop storage in implementation specific way. */ diff --git a/instrumentation-core/servlet/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation-core/servlet/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index 4b7b56c85e..68e289291a 100644 --- a/instrumentation-core/servlet/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation-core/servlet/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -21,15 +21,14 @@ import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerTrace import io.opentelemetry.auto.instrumentation.api.MoreAttributes; import io.opentelemetry.context.propagation.HttpTextFormat.Getter; import io.opentelemetry.trace.Span; -import io.opentelemetry.trace.attributes.SemanticAttributes; import java.net.URI; import java.net.URISyntaxException; import java.security.Principal; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; -public abstract class ServletHttpServerTracer - extends HttpServerTracer { +public abstract class ServletHttpServerTracer + extends HttpServerTracer { @Override protected String getVersion() { @@ -109,10 +108,6 @@ public abstract class ServletHttpServerTracer } } - public void setContentLength(Span span, int length) { - SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.set(span, length); - } - @Override protected String flavor(HttpServletRequest connection, HttpServletRequest request) { return connection.getProtocol(); diff --git a/instrumentation-core/spring/spring-webmvc-3.1/src/main/java/io/opentelemetry/instrumentation/springwebmvc/SpringWebMvcServerTracer.java b/instrumentation-core/spring/spring-webmvc-3.1/src/main/java/io/opentelemetry/instrumentation/springwebmvc/SpringWebMvcServerTracer.java index 44e617bc7a..cd5619565f 100644 --- a/instrumentation-core/spring/spring-webmvc-3.1/src/main/java/io/opentelemetry/instrumentation/springwebmvc/SpringWebMvcServerTracer.java +++ b/instrumentation-core/spring/spring-webmvc-3.1/src/main/java/io/opentelemetry/instrumentation/springwebmvc/SpringWebMvcServerTracer.java @@ -24,9 +24,11 @@ import io.opentelemetry.trace.Tracer; import java.net.URI; import java.net.URISyntaxException; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; class SpringWebMvcServerTracer - extends HttpServerTracer { + extends HttpServerTracer< + HttpServletRequest, HttpServletResponse, HttpServletRequest, HttpServletRequest> { public SpringWebMvcServerTracer(Tracer tracer) { super(tracer); @@ -62,6 +64,11 @@ class SpringWebMvcServerTracer return httpServletRequest.getHeader(name); } + @Override + protected int responseStatus(HttpServletResponse httpServletResponse) { + return httpServletResponse.getStatus(); + } + @Override protected void attachServerContext(Context context, HttpServletRequest request) { request.setAttribute(CONTEXT_ATTRIBUTE, context); diff --git a/instrumentation-core/spring/spring-webmvc-3.1/src/main/java/io/opentelemetry/instrumentation/springwebmvc/WebMVCTracingFilter.java b/instrumentation-core/spring/spring-webmvc-3.1/src/main/java/io/opentelemetry/instrumentation/springwebmvc/WebMVCTracingFilter.java index b58a404e7e..1afe35e3d4 100644 --- a/instrumentation-core/spring/spring-webmvc-3.1/src/main/java/io/opentelemetry/instrumentation/springwebmvc/WebMVCTracingFilter.java +++ b/instrumentation-core/spring/spring-webmvc-3.1/src/main/java/io/opentelemetry/instrumentation/springwebmvc/WebMVCTracingFilter.java @@ -45,9 +45,9 @@ public class WebMVCTracingFilter extends OncePerRequestFilter implements Ordered try (Scope ignored = tracer.startScope(serverSpan, request)) { filterChain.doFilter(request, response); - tracer.end(serverSpan, response.getStatus()); + tracer.end(serverSpan, response); } catch (Throwable t) { - tracer.endExceptionally(serverSpan, t, response.getStatus()); + tracer.endExceptionally(serverSpan, t, response); throw t; } } diff --git a/instrumentation/akka-http-10.0/src/main/java/io/opentelemetry/auto/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java b/instrumentation/akka-http-10.0/src/main/java/io/opentelemetry/auto/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java index dcb39bf942..6cb7c2cb8a 100644 --- a/instrumentation/akka-http-10.0/src/main/java/io/opentelemetry/auto/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java +++ b/instrumentation/akka-http-10.0/src/main/java/io/opentelemetry/auto/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java @@ -110,7 +110,7 @@ public final class AkkaHttpServerInstrumentation extends Instrumenter.Default { Span span = TRACER.startSpan(request, request, "akka.request"); try (Scope ignored = TRACER.startScope(span, null)) { HttpResponse response = userHandler.apply(request); - TRACER.end(span, response.status().intValue()); + TRACER.end(span, response); return response; } catch (final Throwable t) { TRACER.endExceptionally(span, t); @@ -140,7 +140,7 @@ public final class AkkaHttpServerInstrumentation extends Instrumenter.Default { new AbstractFunction1() { @Override public HttpResponse apply(final HttpResponse response) { - TRACER.end(span, response.status().intValue()); + TRACER.end(span, response); return response; } }, diff --git a/instrumentation/akka-http-10.0/src/main/java/io/opentelemetry/auto/instrumentation/akkahttp/AkkaHttpServerTracer.java b/instrumentation/akka-http-10.0/src/main/java/io/opentelemetry/auto/instrumentation/akkahttp/AkkaHttpServerTracer.java index 942d8ee99d..5e1dc4e45d 100644 --- a/instrumentation/akka-http-10.0/src/main/java/io/opentelemetry/auto/instrumentation/akkahttp/AkkaHttpServerTracer.java +++ b/instrumentation/akka-http-10.0/src/main/java/io/opentelemetry/auto/instrumentation/akkahttp/AkkaHttpServerTracer.java @@ -18,13 +18,15 @@ package io.opentelemetry.auto.instrumentation.akkahttp; import akka.http.javadsl.model.HttpHeader; import akka.http.scaladsl.model.HttpRequest; +import akka.http.scaladsl.model.HttpResponse; import io.grpc.Context; import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerTracer; import io.opentelemetry.context.propagation.HttpTextFormat.Getter; import java.net.URI; import java.net.URISyntaxException; -public class AkkaHttpServerTracer extends HttpServerTracer { +public class AkkaHttpServerTracer + extends HttpServerTracer { public static final AkkaHttpServerTracer TRACER = new AkkaHttpServerTracer(); @Override @@ -37,6 +39,11 @@ public class AkkaHttpServerTracer extends HttpServerTracer { + extends HttpServerTracer { ArmeriaServerTracer() {} @@ -95,6 +96,11 @@ public class ArmeriaServerTracer return httpRequest.headers().get(name); } + @Override + protected int responseStatus(RequestLog httpResponse) { + return httpResponse.responseHeaders().status().code(); + } + @Override protected void attachServerContext(Context context, Void ctx) {} diff --git a/instrumentation/armeria-1.0/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_0/server/OpenTelemetryService.java b/instrumentation/armeria-1.0/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_0/server/OpenTelemetryService.java index 2b1032e35a..c5fa3e8e3b 100644 --- a/instrumentation/armeria-1.0/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_0/server/OpenTelemetryService.java +++ b/instrumentation/armeria-1.0/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_0/server/OpenTelemetryService.java @@ -18,7 +18,6 @@ package io.opentelemetry.instrumentation.armeria.v1_0.server; import com.linecorp.armeria.common.HttpRequest; import com.linecorp.armeria.common.HttpResponse; -import com.linecorp.armeria.common.HttpStatus; import com.linecorp.armeria.common.logging.RequestLogProperty; import com.linecorp.armeria.server.HttpService; import com.linecorp.armeria.server.Route; @@ -74,13 +73,12 @@ public class OpenTelemetryService extends SimpleDecoratingHttpService { .whenComplete() .thenAccept( log -> { - HttpStatus status = log.responseHeaders().status(); long requestEndTimeNanos = requestStartTimeNanos + log.responseDurationNanos(); if (log.responseCause() != null) { serverTracer.endExceptionally( - span, log.responseCause(), status.code(), requestEndTimeNanos); + span, log.responseCause(), log, requestEndTimeNanos); } else { - serverTracer.end(span, status.code(), requestEndTimeNanos); + serverTracer.end(span, log, requestEndTimeNanos); } }); } diff --git a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpServerTracer.java b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpServerTracer.java index 0eabb242fd..db30251162 100644 --- a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpServerTracer.java +++ b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpServerTracer.java @@ -23,9 +23,11 @@ import java.net.URI; import java.net.URISyntaxException; import org.glassfish.grizzly.filterchain.FilterChainContext; import org.glassfish.grizzly.http.HttpRequestPacket; +import org.glassfish.grizzly.http.HttpResponsePacket; public class GrizzlyHttpServerTracer - extends HttpServerTracer { + extends HttpServerTracer< + HttpRequestPacket, HttpResponsePacket, HttpRequestPacket, FilterChainContext> { public static final GrizzlyHttpServerTracer TRACER = new GrizzlyHttpServerTracer(); @@ -39,6 +41,11 @@ public class GrizzlyHttpServerTracer return httpRequestPacket.getHeader(name); } + @Override + protected int responseStatus(HttpResponsePacket httpResponsePacket) { + return httpResponsePacket.getStatus(); + } + @Override protected void attachServerContext(Context context, FilterChainContext filterChainContext) { filterChainContext.getAttributes().setAttribute(CONTEXT_ATTRIBUTE, context); diff --git a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/HttpServerFilterAdvice.java b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/HttpServerFilterAdvice.java index 4b3638ffbc..2b5ee8a2b7 100644 --- a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/HttpServerFilterAdvice.java +++ b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/HttpServerFilterAdvice.java @@ -30,7 +30,7 @@ public class HttpServerFilterAdvice { @Advice.Argument(2) final HttpResponsePacket response) { Span span = TRACER.getServerSpan(ctx); if (span != null) { - TRACER.end(span, response.getStatus()); + TRACER.end(span, response); } } } diff --git a/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/auto/instrumentation/jetty/JettyHandlerAdvice.java b/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/auto/instrumentation/jetty/JettyHandlerAdvice.java index ec52600d88..c1e2c2e44a 100644 --- a/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/auto/instrumentation/jetty/JettyHandlerAdvice.java +++ b/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/auto/instrumentation/jetty/JettyHandlerAdvice.java @@ -76,7 +76,7 @@ public class JettyHandlerAdvice { TRACER.setPrincipal(span, request); if (throwable != null) { - TRACER.endExceptionally(span, throwable, response.getStatus()); + TRACER.endExceptionally(span, throwable, response); return; } @@ -94,8 +94,7 @@ public class JettyHandlerAdvice { // Check again in case the request finished before adding the listener. if (!request.isAsyncStarted() && responseHandled.compareAndSet(false, true)) { - JettyHttpServerTracer.contentLengthHelper(span, response); - TRACER.end(span, response.getStatus()); + TRACER.end(span, response); } } } diff --git a/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/auto/instrumentation/jetty/JettyHttpServerTracer.java b/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/auto/instrumentation/jetty/JettyHttpServerTracer.java index 224501437e..668b4bb665 100644 --- a/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/auto/instrumentation/jetty/JettyHttpServerTracer.java +++ b/instrumentation/jetty-8.0/src/main/java/io/opentelemetry/auto/instrumentation/jetty/JettyHttpServerTracer.java @@ -16,20 +16,11 @@ package io.opentelemetry.auto.instrumentation.jetty; -import io.opentelemetry.auto.instrumentation.servlet.v3_0.CountingHttpServletResponse; import io.opentelemetry.auto.instrumentation.servlet.v3_0.Servlet3HttpServerTracer; -import io.opentelemetry.trace.Span; -import javax.servlet.ServletResponse; public class JettyHttpServerTracer extends Servlet3HttpServerTracer { public static final JettyHttpServerTracer TRACER = new JettyHttpServerTracer(); - public static void contentLengthHelper(Span span, ServletResponse response) { - if (response instanceof CountingHttpServletResponse) { - TRACER.setContentLength(span, ((CountingHttpServletResponse) response).getContentLength()); - } - } - @Override protected String getInstrumentationName() { return "io.opentelemetry.auto.jetty-8.0"; diff --git a/instrumentation/netty/netty-3.8/src/main/java/io/opentelemetry/auto/instrumentation/netty/v3_8/server/HttpServerResponseTracingHandler.java b/instrumentation/netty/netty-3.8/src/main/java/io/opentelemetry/auto/instrumentation/netty/v3_8/server/HttpServerResponseTracingHandler.java index 5d9477f49a..6c7f9e5bca 100644 --- a/instrumentation/netty/netty-3.8/src/main/java/io/opentelemetry/auto/instrumentation/netty/v3_8/server/HttpServerResponseTracingHandler.java +++ b/instrumentation/netty/netty-3.8/src/main/java/io/opentelemetry/auto/instrumentation/netty/v3_8/server/HttpServerResponseTracingHandler.java @@ -58,6 +58,6 @@ public class HttpServerResponseTracingHandler extends SimpleChannelDownstreamHan TRACER.endExceptionally(span, throwable); throw throwable; } - TRACER.end(span, ((HttpResponse) msg.getMessage()).getStatus().getCode()); + TRACER.end(span, (HttpResponse) msg.getMessage()); } } diff --git a/instrumentation/netty/netty-3.8/src/main/java/io/opentelemetry/auto/instrumentation/netty/v3_8/server/NettyHttpServerTracer.java b/instrumentation/netty/netty-3.8/src/main/java/io/opentelemetry/auto/instrumentation/netty/v3_8/server/NettyHttpServerTracer.java index 5c59be420d..780469db9d 100644 --- a/instrumentation/netty/netty-3.8/src/main/java/io/opentelemetry/auto/instrumentation/netty/v3_8/server/NettyHttpServerTracer.java +++ b/instrumentation/netty/netty-3.8/src/main/java/io/opentelemetry/auto/instrumentation/netty/v3_8/server/NettyHttpServerTracer.java @@ -28,9 +28,10 @@ import java.net.URI; import java.net.URISyntaxException; import org.jboss.netty.channel.Channel; import org.jboss.netty.handler.codec.http.HttpRequest; +import org.jboss.netty.handler.codec.http.HttpResponse; public class NettyHttpServerTracer - extends HttpServerTracer { + extends HttpServerTracer { public static final NettyHttpServerTracer TRACER = new NettyHttpServerTracer(); @Override @@ -43,6 +44,11 @@ public class NettyHttpServerTracer return httpRequest.headers().get(name); } + @Override + protected int responseStatus(HttpResponse httpResponse) { + return httpResponse.getStatus().getCode(); + } + @Override protected void attachServerContext(Context context, ChannelTraceContext channelTraceContext) { channelTraceContext.setContext(context); diff --git a/instrumentation/netty/netty-4.0/src/main/java/io/opentelemetry/auto/instrumentation/netty/v4_0/server/HttpServerResponseTracingHandler.java b/instrumentation/netty/netty-4.0/src/main/java/io/opentelemetry/auto/instrumentation/netty/v4_0/server/HttpServerResponseTracingHandler.java index 2a95f3889f..d58adc1674 100644 --- a/instrumentation/netty/netty-4.0/src/main/java/io/opentelemetry/auto/instrumentation/netty/v4_0/server/HttpServerResponseTracingHandler.java +++ b/instrumentation/netty/netty-4.0/src/main/java/io/opentelemetry/auto/instrumentation/netty/v4_0/server/HttpServerResponseTracingHandler.java @@ -45,6 +45,6 @@ public class HttpServerResponseTracingHandler extends ChannelOutboundHandlerAdap TRACER.endExceptionally(span, throwable); throw throwable; } - TRACER.end(span, ((HttpResponse) msg).getStatus().code()); + TRACER.end(span, (HttpResponse) msg); } } diff --git a/instrumentation/netty/netty-4.0/src/main/java/io/opentelemetry/auto/instrumentation/netty/v4_0/server/NettyHttpServerTracer.java b/instrumentation/netty/netty-4.0/src/main/java/io/opentelemetry/auto/instrumentation/netty/v4_0/server/NettyHttpServerTracer.java index 1b95e79511..8b78453486 100644 --- a/instrumentation/netty/netty-4.0/src/main/java/io/opentelemetry/auto/instrumentation/netty/v4_0/server/NettyHttpServerTracer.java +++ b/instrumentation/netty/netty-4.0/src/main/java/io/opentelemetry/auto/instrumentation/netty/v4_0/server/NettyHttpServerTracer.java @@ -21,6 +21,7 @@ import static io.netty.handler.codec.http.HttpHeaders.Names.HOST; import io.grpc.Context; import io.netty.channel.Channel; import io.netty.handler.codec.http.HttpRequest; +import io.netty.handler.codec.http.HttpResponse; import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerTracer; import io.opentelemetry.auto.instrumentation.netty.v4_0.AttributeKeys; import io.opentelemetry.context.propagation.HttpTextFormat.Getter; @@ -29,7 +30,8 @@ import java.net.SocketAddress; import java.net.URI; import java.net.URISyntaxException; -public class NettyHttpServerTracer extends HttpServerTracer { +public class NettyHttpServerTracer + extends HttpServerTracer { public static final NettyHttpServerTracer TRACER = new NettyHttpServerTracer(); @Override @@ -42,6 +44,11 @@ public class NettyHttpServerTracer extends HttpServerTracer { +public class NettyHttpServerTracer + extends HttpServerTracer { public static final NettyHttpServerTracer TRACER = new NettyHttpServerTracer(); @Override @@ -42,6 +44,11 @@ public class NettyHttpServerTracer extends HttpServerTracer { public static final Servlet2HttpServerTracer TRACER = new Servlet2HttpServerTracer(); protected String getInstrumentationName() { return "io.opentelemetry.auto.servlet-2.2"; } + + @Override + protected int responseStatus(ResponseWithStatus responseWithStatus) { + return responseWithStatus.getStatus(); + } } diff --git a/instrumentation/servlet/servlet-2.2/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_2/Servlet2Instrumentation.java b/instrumentation/servlet/servlet-2.2/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_2/Servlet2Instrumentation.java index 8266a780ed..67ca463768 100644 --- a/instrumentation/servlet/servlet-2.2/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_2/Servlet2Instrumentation.java +++ b/instrumentation/servlet/servlet-2.2/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_2/Servlet2Instrumentation.java @@ -63,6 +63,7 @@ public final class Servlet2Instrumentation extends Instrumenter.Default { return new String[] { "io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer", "io.opentelemetry.instrumentation.servlet.HttpServletRequestGetter", + packageName + ".ResponseWithStatus", packageName + ".Servlet2HttpServerTracer" }; } diff --git a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java index 35494ae38a..99db3bc9fd 100644 --- a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java +++ b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java @@ -83,8 +83,7 @@ public class Servlet3Advice { TRACER.setPrincipal(span, (HttpServletRequest) request); if (throwable != null) { - contentLengthHelper(span, response); - TRACER.endExceptionally(span, throwable, ((HttpServletResponse) response).getStatus()); + TRACER.endExceptionally(span, throwable, (HttpServletResponse) response); return; } @@ -102,14 +101,7 @@ public class Servlet3Advice { // Check again in case the request finished before adding the listener. if (!request.isAsyncStarted() && responseHandled.compareAndSet(false, true)) { - contentLengthHelper(span, response); - TRACER.end(span, ((HttpServletResponse) response).getStatus()); - } - } - - public static void contentLengthHelper(Span span, ServletResponse response) { - if (response instanceof CountingHttpServletResponse) { - TRACER.setContentLength(span, ((CountingHttpServletResponse) response).getContentLength()); + TRACER.end(span, (HttpServletResponse) response); } } } diff --git a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java index df3ee0c4ec..c751da3de1 100644 --- a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java +++ b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java @@ -22,9 +22,11 @@ import io.grpc.Context; import io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer; import io.opentelemetry.trace.Span; import io.opentelemetry.trace.Status; +import io.opentelemetry.trace.attributes.SemanticAttributes; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; -public class Servlet3HttpServerTracer extends ServletHttpServerTracer { +public class Servlet3HttpServerTracer extends ServletHttpServerTracer { public static final Servlet3HttpServerTracer TRACER = new Servlet3HttpServerTracer(); @@ -38,12 +40,45 @@ public class Servlet3HttpServerTracer extends ServletHttpServerTracer { return connection.getRemotePort(); } + @Override + protected int responseStatus(HttpServletResponse httpServletResponse) { + return httpServletResponse.getStatus(); + } + + @Override + public void endExceptionally( + Span span, Throwable throwable, HttpServletResponse response, long timestamp) { + captureContentLength(span, response); + if (response.isCommitted()) { + super.endExceptionally(span, throwable, response, timestamp); + } else { + // passing null response to super, in order to capture as 500 / INTERNAL, due to servlet spec + // https://javaee.github.io/servlet-spec/downloads/servlet-4.0/servlet-4_0_FINAL.pdf: + // "If a servlet generates an error that is not handled by the error page mechanism as + // described above, the container must ensure to send a response with status 500." + super.endExceptionally(span, throwable, null, timestamp); + } + } + + @Override + public void end(Span span, HttpServletResponse response, long timestamp) { + captureContentLength(span, response); + super.end(span, response, timestamp); + } + public void onTimeout(Span span, long timeout) { span.setStatus(Status.DEADLINE_EXCEEDED); span.setAttribute("timeout", timeout); span.end(); } + private static void captureContentLength(Span span, HttpServletResponse response) { + if (response instanceof CountingHttpServletResponse) { + SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.set( + span, ((CountingHttpServletResponse) response).getContentLength()); + } + } + /* Given request already has a context associated with it. As there should not be nested spans of kind SERVER, we should NOT create a new span here. @@ -56,7 +91,7 @@ public class Servlet3HttpServerTracer extends ServletHttpServerTracer { In this case we have to put the span from the request into current context before continuing. */ - public boolean needsRescoping(Context attachedContext) { + public static boolean needsRescoping(Context attachedContext) { return !sameTrace(getSpan(Context.current()), getSpan(attachedContext)); } diff --git a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java index 45b0fb5d68..05832170c2 100644 --- a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java +++ b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java @@ -20,7 +20,6 @@ import io.opentelemetry.trace.Span; import java.util.concurrent.atomic.AtomicBoolean; import javax.servlet.AsyncEvent; import javax.servlet.AsyncListener; -import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletResponse; public class TagSettingAsyncListener implements AsyncListener { @@ -38,16 +37,13 @@ public class TagSettingAsyncListener implements AsyncListener { @Override public void onComplete(final AsyncEvent event) { if (responseHandled.compareAndSet(false, true)) { - contentLengthHelper(span, event); - servletHttpServerTracer.end( - span, ((HttpServletResponse) event.getSuppliedResponse()).getStatus()); + servletHttpServerTracer.end(span, (HttpServletResponse) event.getSuppliedResponse()); } } @Override public void onTimeout(final AsyncEvent event) { if (responseHandled.compareAndSet(false, true)) { - contentLengthHelper(span, event); servletHttpServerTracer.onTimeout(span, event.getAsyncContext().getTimeout()); } } @@ -55,11 +51,8 @@ public class TagSettingAsyncListener implements AsyncListener { @Override public void onError(final AsyncEvent event) { if (responseHandled.compareAndSet(false, true)) { - contentLengthHelper(span, event); servletHttpServerTracer.endExceptionally( - span, - event.getThrowable(), - ((HttpServletResponse) event.getSuppliedResponse()).getStatus()); + span, event.getThrowable(), (HttpServletResponse) event.getSuppliedResponse()); } } @@ -68,12 +61,4 @@ public class TagSettingAsyncListener implements AsyncListener { public void onStartAsync(final AsyncEvent event) { event.getAsyncContext().addListener(this); } - - public static void contentLengthHelper(Span span, AsyncEvent event) { - ServletResponse response = event.getSuppliedResponse(); - if (response instanceof CountingHttpServletResponse) { - servletHttpServerTracer.setContentLength( - span, ((CountingHttpServletResponse) response).getContentLength()); - } - } }