From 8fef624fd01bb39a9b8b6e6e3aaf8ac8796bb6df Mon Sep 17 00:00:00 2001 From: Nikita Salnikov-Tarnovski Date: Tue, 26 May 2020 12:31:25 +0300 Subject: [PATCH 1/4] Servlet instrumentations unified --- .../decorator/HttpServerTracer.java | 174 ++++++++++++++ gradle/instrumentation.gradle | 6 +- .../servlet/request-2.3/request-2.3.gradle | 1 + .../HttpServletRequestExtractAdapter.java | 38 --- .../servlet/v2_3/ResponseWithStatus.java | 13 ++ .../servlet/v2_3/Servlet2Advice.java | 91 ++------ .../servlet/v2_3/Servlet2Decorator.java | 80 ------- .../v2_3/Servlet2HttpServerTracer.java | 15 ++ .../servlet/request-3.0/request-3.0.gradle | 1 + .../v3_0/AsyncContextInstrumentation.java | 7 +- .../HttpServletRequestExtractAdapter.java | 30 --- .../servlet/v3_0/Servlet3Advice.java | 108 +-------- .../servlet/v3_0/Servlet3Decorator.java | 118 ---------- .../v3_0/Servlet3HttpServerTracer.java | 52 +++++ .../servlet/v3_0/Servlet3Instrumentation.java | 2 - .../servlet/v3_0/TagSettingAsyncListener.java | 49 ++-- .../servlet-common/servlet-common.gradle | 28 +++ .../servlet/ServletHttpServerTracer.java | 216 ++++++++++++++++++ settings.gradle | 1 + 19 files changed, 548 insertions(+), 482 deletions(-) create mode 100644 agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerTracer.java delete mode 100644 instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/HttpServletRequestExtractAdapter.java create mode 100644 instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/ResponseWithStatus.java delete mode 100644 instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Decorator.java create mode 100644 instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2HttpServerTracer.java delete mode 100644 instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/HttpServletRequestExtractAdapter.java delete mode 100644 instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Decorator.java create mode 100644 instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java create mode 100644 instrumentation/servlet/servlet-common/servlet-common.gradle create mode 100644 instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.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 new file mode 100644 index 000000000000..143614fb82bb --- /dev/null +++ b/agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerTracer.java @@ -0,0 +1,174 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.opentelemetry.auto.bootstrap.instrumentation.decorator; + +import static io.opentelemetry.OpenTelemetry.getPropagators; +import static io.opentelemetry.trace.TracingContextUtils.getSpan; + +import io.grpc.Context; +import io.opentelemetry.OpenTelemetry; +import io.opentelemetry.auto.config.Config; +import io.opentelemetry.auto.instrumentation.api.MoreTags; +import io.opentelemetry.auto.instrumentation.api.Tags; +import io.opentelemetry.context.propagation.HttpTextFormat; +import io.opentelemetry.trace.Span; +import io.opentelemetry.trace.SpanContext; +import io.opentelemetry.trace.Status; +import io.opentelemetry.trace.Tracer; +import io.opentelemetry.trace.attributes.SemanticAttributes; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.lang.reflect.Method; +import java.net.URI; +import java.net.URISyntaxException; +import java.util.concurrent.ExecutionException; +import lombok.extern.slf4j.Slf4j; + +@Slf4j +public abstract class HttpServerTracer { + public static final String SPAN_ATTRIBUTE = "io.opentelemetry.auto.span"; + + protected final Tracer tracer; + + public HttpServerTracer() { + tracer = OpenTelemetry.getTracerProvider().get(getInstrumentationName(), getVersion()); + } + + protected abstract String getVersion(); + + protected abstract String getInstrumentationName(); + + protected void onConnection(REQUEST request, Span span) { + SemanticAttributes.NET_PEER_IP.set(span, peerHostIP(request)); + final Integer port = peerPort(request); + // Negative or Zero ports might represent an unset/null value for an int type. Skip setting. + if (port != null && port > 0) { + SemanticAttributes.NET_PEER_PORT.set(span, port); + } + } + + protected abstract Integer peerPort(REQUEST request); + + protected abstract String peerHostIP(REQUEST request); + + //TODO use semantic attributes + public void onRequest(final Span span, final REQUEST request) { + span.setAttribute(Tags.HTTP_METHOD, method(request)); + + // Copy of HttpClientDecorator url handling + try { + final URI url = url(request); + if (url != null) { + final StringBuilder urlBuilder = new StringBuilder(); + if (url.getScheme() != null) { + urlBuilder.append(url.getScheme()); + urlBuilder.append("://"); + } + if (url.getHost() != null) { + urlBuilder.append(url.getHost()); + if (url.getPort() > 0 && url.getPort() != 80 && url.getPort() != 443) { + urlBuilder.append(":"); + urlBuilder.append(url.getPort()); + } + } + final String path = url.getPath(); + if (path.isEmpty()) { + urlBuilder.append("/"); + } else { + urlBuilder.append(path); + } + final String query = url.getQuery(); + if (query != null) { + urlBuilder.append("?").append(query); + } + final String fragment = url.getFragment(); + if (fragment != null) { + urlBuilder.append("#").append(fragment); + } + + span.setAttribute(Tags.HTTP_URL, urlBuilder.toString()); + + if (Config.get().isHttpServerTagQueryString()) { + span.setAttribute(MoreTags.HTTP_QUERY, url.getQuery()); + span.setAttribute(MoreTags.HTTP_FRAGMENT, url.getFragment()); + } + } + } catch (final Exception e) { + log.debug("Error tagging url", e); + } + // TODO set resource name from URL. + } + + protected abstract URI url(REQUEST request) throws URISyntaxException; + + protected abstract String method(REQUEST request); + + /** + * This method is used to generate an acceptable span (operation) name based on a given method + * reference. Anonymous classes are named based on their parent. + */ + public String spanNameForMethod(final Method method) { + return spanNameForClass(method.getDeclaringClass()) + "." + method.getName(); + } + + /** + * This method is used to generate an acceptable span (operation) name based on a given class + * reference. Anonymous classes are named based on their parent. + */ + public String spanNameForClass(final Class clazz) { + if (!clazz.isAnonymousClass()) { + return clazz.getSimpleName(); + } + String className = clazz.getName(); + if (clazz.getPackage() != null) { + final String pkgName = clazz.getPackage().getName(); + if (!pkgName.isEmpty()) { + className = clazz.getName().replace(pkgName, "").substring(1); + } + } + return className; + } + + public void onError(final Span span, final Throwable throwable) { + span.setStatus(Status.UNKNOWN); + addThrowable(span, unwrapThrowable(throwable)); + } + + //TODO semantic attributes + public static void addThrowable(final Span span, final Throwable throwable) { + span.setAttribute(MoreTags.ERROR_MSG, throwable.getMessage()); + span.setAttribute(MoreTags.ERROR_TYPE, throwable.getClass().getName()); + + final StringWriter errorString = new StringWriter(); + throwable.printStackTrace(new PrintWriter(errorString)); + span.setAttribute(MoreTags.ERROR_STACK, errorString.toString()); + } + + public Span getCurrentSpan() { + return tracer.getCurrentSpan(); + } + + protected Throwable unwrapThrowable(Throwable throwable) { + return throwable instanceof ExecutionException ? throwable.getCause() : throwable; + } + + public static SpanContext extract(final C carrier, final HttpTextFormat.Getter getter) { + final Context context = + getPropagators().getHttpTextFormat().extract(Context.current(), carrier, getter); + final Span span = getSpan(context); + return span.getContext(); + } +} diff --git a/gradle/instrumentation.gradle b/gradle/instrumentation.gradle index 126e9ac3a551..5509f0f8019b 100644 --- a/gradle/instrumentation.gradle +++ b/gradle/instrumentation.gradle @@ -13,9 +13,9 @@ byteBuddy { apply from: "${rootDir}/gradle/java.gradle" -tasks.withType(Test) { - forkEvery = 1 -} +//tasks.withType(Test) { +// forkEvery = 1 +//} afterEvaluate { byteBuddy { diff --git a/instrumentation/servlet/request-2.3/request-2.3.gradle b/instrumentation/servlet/request-2.3/request-2.3.gradle index fddad8af2f96..5ffb4814b39a 100644 --- a/instrumentation/servlet/request-2.3/request-2.3.gradle +++ b/instrumentation/servlet/request-2.3/request-2.3.gradle @@ -24,6 +24,7 @@ testSets { dependencies { compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.2' + compile(project(':instrumentation:servlet:servlet-common')) testCompile(project(':testing')) { exclude group: 'org.eclipse.jetty', module: 'jetty-server' diff --git a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/HttpServletRequestExtractAdapter.java b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/HttpServletRequestExtractAdapter.java deleted file mode 100644 index ba2c8057e261..000000000000 --- a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/HttpServletRequestExtractAdapter.java +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.opentelemetry.auto.instrumentation.servlet.v2_3; - -import io.opentelemetry.context.propagation.HttpTextFormat; -import javax.servlet.http.HttpServletRequest; - -public class HttpServletRequestExtractAdapter implements HttpTextFormat.Getter { - - public static final HttpServletRequestExtractAdapter GETTER = - new HttpServletRequestExtractAdapter(); - - @Override - public String get(final HttpServletRequest carrier, final String key) { - /* - * Read from the attributes and override the headers. - * This is used by HttpServletRequestSetter when a request is async-dispatched. - */ - final Object attribute = carrier.getAttribute(key); - if (attribute instanceof String) { - return (String) attribute; - } - return carrier.getHeader(key); - } -} diff --git a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/ResponseWithStatus.java b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/ResponseWithStatus.java new file mode 100644 index 000000000000..431dcdd37aa4 --- /dev/null +++ b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/ResponseWithStatus.java @@ -0,0 +1,13 @@ +package io.opentelemetry.auto.instrumentation.servlet.v2_3; + +import javax.servlet.http.HttpServletResponse; + +public class ResponseWithStatus { + public final HttpServletResponse response; + public final Integer status; + + public ResponseWithStatus(HttpServletResponse response, Integer status) { + this.response = response; + this.status = status; + } +} diff --git a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java index 7f0fba512b54..bf69e5bb5bf2 100644 --- a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java +++ b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java @@ -15,21 +15,8 @@ */ package io.opentelemetry.auto.instrumentation.servlet.v2_3; -import static io.opentelemetry.auto.bootstrap.instrumentation.decorator.BaseDecorator.extract; -import static io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerDecorator.SPAN_ATTRIBUTE; -import static io.opentelemetry.auto.instrumentation.servlet.v2_3.HttpServletRequestExtractAdapter.GETTER; -import static io.opentelemetry.auto.instrumentation.servlet.v2_3.Servlet2Decorator.DECORATE; -import static io.opentelemetry.auto.instrumentation.servlet.v2_3.Servlet2Decorator.TRACER; -import static io.opentelemetry.trace.Span.Kind.SERVER; -import static io.opentelemetry.trace.TracingContextUtils.currentContextWith; - import io.opentelemetry.auto.bootstrap.InstrumentationContext; -import io.opentelemetry.auto.instrumentation.api.MoreTags; import io.opentelemetry.auto.instrumentation.api.SpanWithScope; -import io.opentelemetry.auto.instrumentation.api.Tags; -import io.opentelemetry.trace.Span; -import io.opentelemetry.trace.Status; -import java.security.Principal; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; @@ -38,47 +25,26 @@ import net.bytebuddy.implementation.bytecode.assign.Assigner; public class Servlet2Advice { + public static final Servlet2HttpServerTracer TRACER = new Servlet2HttpServerTracer(); @Advice.OnMethodEnter(suppress = Throwable.class) public static SpanWithScope onEnter( @Advice.This final Object servlet, @Advice.Argument(0) final ServletRequest request, - @Advice.Argument(value = 1, typing = Assigner.Typing.DYNAMIC) - final ServletResponse response) { - final boolean hasServletTrace = request.getAttribute(SPAN_ATTRIBUTE) instanceof Span; - final boolean invalidRequest = !(request instanceof HttpServletRequest); - if (invalidRequest || hasServletTrace) { - // Tracing might already be applied by the FilterChain or a parent request (forward/include). + @Advice.Argument(value = 1, typing = Assigner.Typing.DYNAMIC) final ServletResponse response) { + + if (!(request instanceof HttpServletRequest)) { return null; } final HttpServletRequest httpServletRequest = (HttpServletRequest) request; - if (response instanceof HttpServletResponse) { - // For use by HttpServletResponseInstrumentation: - InstrumentationContext.get(HttpServletResponse.class, HttpServletRequest.class) - .put((HttpServletResponse) response, httpServletRequest); - - // Default value for checking for uncaught error later - InstrumentationContext.get(ServletResponse.class, Integer.class).put(response, 200); - } - - final Span.Builder builder = - TRACER.spanBuilder(DECORATE.spanNameForRequest(httpServletRequest)).setSpanKind(SERVER); - builder.setParent(extract(httpServletRequest, GETTER)); - - final Span span = - builder.setAttribute("span.origin.type", servlet.getClass().getName()).startSpan(); + // For use by HttpServletResponseInstrumentation: + InstrumentationContext.get(HttpServletResponse.class, HttpServletRequest.class) + .put((HttpServletResponse) response, httpServletRequest); - DECORATE.afterStart(span); - DECORATE.onConnection(span, httpServletRequest); - DECORATE.onRequest(span, httpServletRequest); + return TRACER.startSpan(httpServletRequest, servlet.getClass().getName()); - httpServletRequest.setAttribute(SPAN_ATTRIBUTE, span); - httpServletRequest.setAttribute("traceId", span.getContext().getTraceId().toLowerBase16()); - httpServletRequest.setAttribute("spanId", span.getContext().getSpanId().toLowerBase16()); - - return new SpanWithScope(span, currentContextWith(span)); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) @@ -87,39 +53,16 @@ public static void stopSpan( @Advice.Argument(1) final ServletResponse response, @Advice.Enter final SpanWithScope spanWithScope, @Advice.Thrown final Throwable throwable) { - // Set user.principal regardless of who created this span. - final Object spanAttr = request.getAttribute(SPAN_ATTRIBUTE); - if (spanAttr instanceof Span && request instanceof HttpServletRequest) { - final Principal principal = ((HttpServletRequest) request).getUserPrincipal(); - if (principal != null) { - ((Span) spanAttr).setAttribute(MoreTags.USER_NAME, principal.getName()); - } - } - - if (spanWithScope == null) { - return; - } - final Span span = spanWithScope.getSpan(); - - if (response instanceof HttpServletResponse) { - DECORATE.onResponse( - span, InstrumentationContext.get(ServletResponse.class, Integer.class).get(response)); - } else { - DECORATE.onResponse(span, null); - } - if (throwable != null) { - if (response instanceof HttpServletResponse - && InstrumentationContext.get(ServletResponse.class, Integer.class).get(response) - == HttpServletResponse.SC_OK) { - // exception was thrown but status code wasn't set - span.setAttribute(Tags.HTTP_STATUS, 500); - span.setStatus(Status.UNKNOWN); - } - DECORATE.onError(span, throwable); + if (request instanceof HttpServletRequest && response instanceof HttpServletResponse) { + Integer responseStatus = InstrumentationContext + .get(ServletResponse.class, Integer.class) + .get(response); + TRACER.stopSpan( + (HttpServletRequest) request, + new ResponseWithStatus((HttpServletResponse) response, responseStatus), + spanWithScope, + throwable); } - DECORATE.beforeFinish(span); - span.end(); - spanWithScope.closeScope(); } } diff --git a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Decorator.java b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Decorator.java deleted file mode 100644 index 897ee668c744..000000000000 --- a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Decorator.java +++ /dev/null @@ -1,80 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.opentelemetry.auto.instrumentation.servlet.v2_3; - -import io.opentelemetry.OpenTelemetry; -import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerDecorator; -import io.opentelemetry.trace.Span; -import io.opentelemetry.trace.Tracer; -import java.net.URI; -import java.net.URISyntaxException; -import javax.servlet.http.HttpServletRequest; - -public class Servlet2Decorator - extends HttpServerDecorator { - public static final Servlet2Decorator DECORATE = new Servlet2Decorator(); - public static final Tracer TRACER = - OpenTelemetry.getTracerProvider().get("io.opentelemetry.auto.servlet-2.3"); - - @Override - protected String method(final HttpServletRequest httpServletRequest) { - return httpServletRequest.getMethod(); - } - - @Override - protected URI url(final HttpServletRequest httpServletRequest) throws URISyntaxException { - return new URI( - httpServletRequest.getScheme(), - null, - httpServletRequest.getServerName(), - httpServletRequest.getServerPort(), - httpServletRequest.getRequestURI(), - httpServletRequest.getQueryString(), - null); - } - - @Override - protected String peerHostIP(final HttpServletRequest httpServletRequest) { - return httpServletRequest.getRemoteAddr(); - } - - @Override - protected Integer peerPort(final HttpServletRequest httpServletRequest) { - // HttpServletResponse doesn't have accessor for remote port. - return null; - } - - @Override - protected Integer status(final Integer status) { - return status; - } - - @Override - public Span onRequest(final Span span, final HttpServletRequest request) { - assert span != null; - if (request != null) { - final String sc = request.getContextPath(); - if (sc != null && !sc.isEmpty()) { - span.setAttribute("servlet.context", sc); - } - final String sp = request.getServletPath(); - if (sp != null && !sc.isEmpty()) { - span.setAttribute("servlet.path", sp); - } - } - return super.onRequest(span, request); - } -} diff --git a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2HttpServerTracer.java b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2HttpServerTracer.java new file mode 100644 index 000000000000..fc19fe017cd9 --- /dev/null +++ b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2HttpServerTracer.java @@ -0,0 +1,15 @@ +package io.opentelemetry.auto.instrumentation.servlet.v2_3; + +import io.opentelemetry.auto.instrumentation.servlet.ServletHttpServerTracer; + +public class Servlet2HttpServerTracer extends ServletHttpServerTracer { + + protected String getInstrumentationName() { + return "io.opentelemetry.auto.servlet-2.3"; + } + + @Override + protected int status(ResponseWithStatus response) { + return response.status; + } +} diff --git a/instrumentation/servlet/request-3.0/request-3.0.gradle b/instrumentation/servlet/request-3.0/request-3.0.gradle index dceb2d189148..5ae60d4f4eab 100644 --- a/instrumentation/servlet/request-3.0/request-3.0.gradle +++ b/instrumentation/servlet/request-3.0/request-3.0.gradle @@ -23,6 +23,7 @@ testSets { dependencies { compileOnly group: 'javax.servlet', name: 'javax.servlet-api', version: '3.0.1' + compile(project(':instrumentation:servlet:servlet-common')) testCompile(project(':testing')) { exclude group: 'org.eclipse.jetty', module: 'jetty-server' diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/AsyncContextInstrumentation.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/AsyncContextInstrumentation.java index 1405088eedd8..42e9dd8c6f27 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/AsyncContextInstrumentation.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/AsyncContextInstrumentation.java @@ -16,7 +16,6 @@ package io.opentelemetry.auto.instrumentation.servlet.v3_0; import static io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerDecorator.SPAN_ATTRIBUTE; -import static io.opentelemetry.auto.instrumentation.servlet.v3_0.Servlet3Decorator.TRACER; import static io.opentelemetry.auto.tooling.ClassLoaderMatcher.hasClassesNamed; import static io.opentelemetry.auto.tooling.bytebuddy.matcher.AgentElementMatchers.implementsInterface; import static java.util.Collections.singletonMap; @@ -54,11 +53,6 @@ public ElementMatcher typeMatcher() { return implementsInterface(named("javax.servlet.AsyncContext")); } - @Override - public String[] helperClassNames() { - return new String[] {packageName + ".Servlet3Decorator"}; - } - @Override public Map, String> transformers() { return singletonMap( @@ -72,6 +66,7 @@ public Map, String> transfor * TagSettingAsyncListener#onStartAsync} */ public static class DispatchAdvice { + public static final Servlet3HttpServerTracer TRACER = new Servlet3HttpServerTracer(); @Advice.OnMethodEnter(suppress = Throwable.class) public static boolean enter( diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/HttpServletRequestExtractAdapter.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/HttpServletRequestExtractAdapter.java deleted file mode 100644 index d77256372973..000000000000 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/HttpServletRequestExtractAdapter.java +++ /dev/null @@ -1,30 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.opentelemetry.auto.instrumentation.servlet.v3_0; - -import io.opentelemetry.context.propagation.HttpTextFormat; -import javax.servlet.http.HttpServletRequest; - -public class HttpServletRequestExtractAdapter implements HttpTextFormat.Getter { - - public static final HttpServletRequestExtractAdapter GETTER = - new HttpServletRequestExtractAdapter(); - - @Override - public String get(final HttpServletRequest carrier, final String key) { - return carrier.getHeader(key); - } -} diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java index 8f1cca4f87fb..2e4cf434cedf 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java @@ -15,22 +15,8 @@ */ package io.opentelemetry.auto.instrumentation.servlet.v3_0; -import static io.opentelemetry.auto.bootstrap.instrumentation.decorator.BaseDecorator.extract; -import static io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerDecorator.SPAN_ATTRIBUTE; -import static io.opentelemetry.auto.instrumentation.servlet.v3_0.HttpServletRequestExtractAdapter.GETTER; -import static io.opentelemetry.auto.instrumentation.servlet.v3_0.Servlet3Decorator.DECORATE; -import static io.opentelemetry.auto.instrumentation.servlet.v3_0.Servlet3Decorator.TRACER; -import static io.opentelemetry.trace.Span.Kind.SERVER; -import static io.opentelemetry.trace.TracingContextUtils.currentContextWith; - import io.opentelemetry.auto.bootstrap.InstrumentationContext; -import io.opentelemetry.auto.instrumentation.api.MoreTags; import io.opentelemetry.auto.instrumentation.api.SpanWithScope; -import io.opentelemetry.auto.instrumentation.api.Tags; -import io.opentelemetry.trace.Span; -import io.opentelemetry.trace.Status; -import java.security.Principal; -import java.util.concurrent.atomic.AtomicBoolean; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; @@ -38,6 +24,7 @@ import net.bytebuddy.asm.Advice; public class Servlet3Advice { + public static final Servlet3HttpServerTracer TRACER = new Servlet3HttpServerTracer(); @Advice.OnMethodEnter(suppress = Throwable.class) public static SpanWithScope onEnter( @@ -47,26 +34,6 @@ public static SpanWithScope onEnter( if (!(request instanceof HttpServletRequest)) { return null; } - final Object spanAttr = request.getAttribute(SPAN_ATTRIBUTE); - if (spanAttr instanceof Span) { - // inside of an existing servlet span already, possibly a dispatched servlet/filter - - final Span span = (Span) spanAttr; - final boolean spanContextWasLost = - !TRACER.getCurrentSpan().getContext().getTraceId().equals(span.getContext().getTraceId()); - if (spanContextWasLost) { - // either there is no current span, or there is a current span but it is left over from some - // other trace - - // re-scope the current work using the span in the request attribute - return new SpanWithScope(null, currentContextWith(span)); - } else { - // everything is good, just inside of a nested servlet/filter - - // do not capture anything - return null; - } - } final HttpServletRequest httpServletRequest = (HttpServletRequest) request; @@ -74,21 +41,7 @@ public static SpanWithScope onEnter( InstrumentationContext.get(HttpServletResponse.class, HttpServletRequest.class) .put((HttpServletResponse) response, httpServletRequest); - final Span.Builder builder = - TRACER.spanBuilder(DECORATE.spanNameForRequest(httpServletRequest)).setSpanKind(SERVER); - builder.setParent(extract(httpServletRequest, GETTER)); - final Span span = - builder.setAttribute("span.origin.type", servlet.getClass().getName()).startSpan(); - - DECORATE.afterStart(span); - DECORATE.onConnection(span, httpServletRequest); - DECORATE.onRequest(span, httpServletRequest); - - httpServletRequest.setAttribute(SPAN_ATTRIBUTE, span); - httpServletRequest.setAttribute("traceId", span.getContext().getTraceId().toLowerBase16()); - httpServletRequest.setAttribute("spanId", span.getContext().getSpanId().toLowerBase16()); - - return new SpanWithScope(span, currentContextWith(span)); + return TRACER.startSpan(httpServletRequest, servlet.getClass().getName()); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) @@ -97,59 +50,12 @@ public static void stopSpan( @Advice.Argument(1) final ServletResponse response, @Advice.Enter final SpanWithScope spanWithScope, @Advice.Thrown final Throwable throwable) { - // Set user.principal regardless of who created this span. - final Object spanAttr = request.getAttribute(SPAN_ATTRIBUTE); - if (spanAttr instanceof Span && request instanceof HttpServletRequest) { - final Principal principal = ((HttpServletRequest) request).getUserPrincipal(); - if (principal != null) { - ((Span) spanAttr).setAttribute(MoreTags.USER_NAME, principal.getName()); - } - } - - if (spanWithScope == null) { - return; - } - - final Span span = spanWithScope.getSpan(); - if (span == null) { - // this was just a re-scoping of the current thread using the span in the request attribute - spanWithScope.closeScope(); - return; - } - if (request instanceof HttpServletRequest && response instanceof HttpServletResponse) { - final HttpServletRequest req = (HttpServletRequest) request; - final HttpServletResponse resp = (HttpServletResponse) response; - - if (throwable != null) { - DECORATE.onResponse(span, resp); - if (resp.getStatus() == HttpServletResponse.SC_OK) { - // exception is thrown in filter chain, but status code is incorrect - // exception was thrown but status code wasn't set - span.setAttribute(Tags.HTTP_STATUS, 500); - span.setStatus(Status.UNKNOWN); - } - DECORATE.onError(span, throwable); - DECORATE.beforeFinish(span); - span.end(); // Finish the span manually since finishSpanOnClose was false - } else { - final AtomicBoolean activated = new AtomicBoolean(false); - if (req.isAsyncStarted()) { - try { - req.getAsyncContext().addListener(new TagSettingAsyncListener(activated, span)); - } catch (final IllegalStateException e) { - // org.eclipse.jetty.server.Request may throw an exception here if request became - // finished after check above. We just ignore that exception and move on. - } - } - // Check again in case the request finished before adding the listener. - if (!req.isAsyncStarted() && activated.compareAndSet(false, true)) { - DECORATE.onResponse(span, resp); - DECORATE.beforeFinish(span); - span.end(); // Finish the span manually since finishSpanOnClose was false - } - } - spanWithScope.closeScope(); + TRACER.stopSpan( + (HttpServletRequest) request, + (HttpServletResponse) response, + spanWithScope, + throwable); } } } diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Decorator.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Decorator.java deleted file mode 100644 index 05ea6e95cc6b..000000000000 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Decorator.java +++ /dev/null @@ -1,118 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.opentelemetry.auto.instrumentation.servlet.v3_0; - -import io.opentelemetry.OpenTelemetry; -import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerDecorator; -import io.opentelemetry.trace.Span; -import io.opentelemetry.trace.Tracer; -import java.io.IOException; -import java.net.URI; -import java.net.URISyntaxException; -import javax.servlet.Filter; -import javax.servlet.ServletContext; -import javax.servlet.ServletException; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import lombok.extern.slf4j.Slf4j; - -@Slf4j -public class Servlet3Decorator - extends HttpServerDecorator { - public static final Tracer TRACER = - OpenTelemetry.getTracerProvider().get("io.opentelemetry.auto.servlet-3.0"); - - public static final Servlet3Decorator DECORATE = new Servlet3Decorator(); - - @Override - protected String method(final HttpServletRequest httpServletRequest) { - return httpServletRequest.getMethod(); - } - - @Override - protected URI url(final HttpServletRequest httpServletRequest) throws URISyntaxException { - return new URI( - httpServletRequest.getScheme(), - null, - httpServletRequest.getServerName(), - httpServletRequest.getServerPort(), - httpServletRequest.getRequestURI(), - httpServletRequest.getQueryString(), - null); - } - - @Override - protected String peerHostIP(final HttpServletRequest httpServletRequest) { - return httpServletRequest.getRemoteAddr(); - } - - @Override - protected Integer peerPort(final HttpServletRequest httpServletRequest) { - return httpServletRequest.getRemotePort(); - } - - @Override - protected Integer status(final HttpServletResponse httpServletResponse) { - return httpServletResponse.getStatus(); - } - - @Override - public Span onRequest(final Span span, final HttpServletRequest request) { - assert span != null; - if (request != null) { - span.setAttribute("servlet.path", request.getServletPath()); - span.setAttribute("servlet.context", request.getContextPath()); - onContext(span, request, request.getServletContext()); - } - return super.onRequest(span, request); - } - - /** - * This method executes the filter created by - * io.opentelemetry.auto.instrumentation.springwebmvc.DispatcherServletInstrumentation$HandlerMappingAdvice. - * This was easier and less "hacky" than other ways to add the filter to the front of the filter - * chain. - */ - private void onContext( - final Span span, final HttpServletRequest request, final ServletContext context) { - if (context == null) { - // some frameworks (jetty) may return a null context. - return; - } - final Object attribute = context.getAttribute("io.opentelemetry.auto.dispatcher-filter"); - if (attribute instanceof Filter) { - final Object priorAttr = request.getAttribute(SPAN_ATTRIBUTE); - request.setAttribute(SPAN_ATTRIBUTE, span); - try { - ((Filter) attribute).doFilter(request, null, null); - } catch (final IOException | ServletException e) { - log.debug("Exception unexpectedly thrown by filter", e); - } finally { - request.setAttribute(SPAN_ATTRIBUTE, priorAttr); - } - } - } - - @Override - public Span onError(final Span span, final Throwable throwable) { - if (throwable instanceof ServletException && throwable.getCause() != null) { - super.onError(span, throwable.getCause()); - } else { - super.onError(span, throwable); - } - return span; - } -} diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java new file mode 100644 index 000000000000..beea90c42faf --- /dev/null +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java @@ -0,0 +1,52 @@ +package io.opentelemetry.auto.instrumentation.servlet.v3_0; + +import io.opentelemetry.auto.instrumentation.servlet.ServletHttpServerTracer; +import io.opentelemetry.trace.Span; +import java.util.concurrent.atomic.AtomicBoolean; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +public class Servlet3HttpServerTracer extends ServletHttpServerTracer { + + protected String getInstrumentationName() { + return "io.opentelemetry.auto.servlet-3.0"; + } + + @Override + protected Integer peerPort(HttpServletRequest request) { + return request.getRemotePort(); + } + + protected int status(HttpServletResponse response) { + return response.getStatus(); + } + + @Override + protected void onResponse(HttpServletRequest request, HttpServletResponse response, + Throwable throwable, Span span) { + final AtomicBoolean responseHandled = new AtomicBoolean(false); + + //In case of async servlets wait for the actual response to be ready + if (request.isAsyncStarted()) { + try { + request.getAsyncContext() + .addListener(new TagSettingAsyncListener(responseHandled, span, this)); + } catch (final IllegalStateException e) { + // org.eclipse.jetty.server.Request may throw an exception here if request became + // finished after check above. We just ignore that exception and move on. + } + } + + // Check again in case the request finished before adding the listener. + if (!request.isAsyncStarted()) { + onResponse(response, throwable, span, responseHandled); + } + } + + public void onResponse(HttpServletResponse response, Throwable throwable, Span span, + AtomicBoolean responseHandled) { + if (responseHandled.compareAndSet(false, true)) { + super.onResponse(response, throwable, span); + } + } +} diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Instrumentation.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Instrumentation.java index 05ea8ad229ed..ba4a5231c4b7 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Instrumentation.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Instrumentation.java @@ -50,8 +50,6 @@ public ElementMatcher typeMatcher() { @Override public String[] helperClassNames() { return new String[] { - packageName + ".Servlet3Decorator", - packageName + ".HttpServletRequestExtractAdapter", packageName + ".TagSettingAsyncListener" }; } diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java index 7b99869bb2b7..84181df3e0e4 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java @@ -15,58 +15,47 @@ */ package io.opentelemetry.auto.instrumentation.servlet.v3_0; -import static io.opentelemetry.auto.instrumentation.servlet.v3_0.Servlet3Decorator.DECORATE; - -import io.opentelemetry.auto.instrumentation.api.Tags; import io.opentelemetry.trace.Span; -import io.opentelemetry.trace.Status; import java.util.concurrent.atomic.AtomicBoolean; import javax.servlet.AsyncEvent; import javax.servlet.AsyncListener; import javax.servlet.http.HttpServletResponse; public class TagSettingAsyncListener implements AsyncListener { - private final AtomicBoolean activated; + private final AtomicBoolean responseHandled; private final Span span; + private final Servlet3HttpServerTracer servletHttpServerTracer; - public TagSettingAsyncListener(final AtomicBoolean activated, final Span span) { - this.activated = activated; + public TagSettingAsyncListener(final AtomicBoolean responseHandled, final Span span, + Servlet3HttpServerTracer servletHttpServerTracer) { + this.responseHandled = responseHandled; this.span = span; + this.servletHttpServerTracer = servletHttpServerTracer; } @Override public void onComplete(final AsyncEvent event) { - if (activated.compareAndSet(false, true)) { - DECORATE.onResponse(span, (HttpServletResponse) event.getSuppliedResponse()); - DECORATE.beforeFinish(span); - span.end(); - } + servletHttpServerTracer + .onResponse( + (HttpServletResponse) event.getSuppliedResponse(), + null, + span, + responseHandled); } @Override public void onTimeout(final AsyncEvent event) { - if (activated.compareAndSet(false, true)) { - span.setStatus(Status.UNKNOWN); - span.setAttribute("timeout", event.getAsyncContext().getTimeout()); - DECORATE.beforeFinish(span); - span.end(); - } + servletHttpServerTracer.onTimeout(responseHandled, span, event.getAsyncContext().getTimeout()); } @Override public void onError(final AsyncEvent event) { - if (event.getThrowable() != null && activated.compareAndSet(false, true)) { - DECORATE.onResponse(span, (HttpServletResponse) event.getSuppliedResponse()); - if (((HttpServletResponse) event.getSuppliedResponse()).getStatus() - == HttpServletResponse.SC_OK) { - // exception is thrown in filter chain, but status code is incorrect - span.setAttribute(Tags.HTTP_STATUS, 500); - span.setStatus(Status.UNKNOWN); - } - DECORATE.onError(span, event.getThrowable()); - DECORATE.beforeFinish(span); - span.end(); - } + servletHttpServerTracer + .onResponse( + (HttpServletResponse) event.getSuppliedResponse(), + event.getThrowable(), + span, + responseHandled); } /** Transfer the listener over to the new context. */ diff --git a/instrumentation/servlet/servlet-common/servlet-common.gradle b/instrumentation/servlet/servlet-common/servlet-common.gradle new file mode 100644 index 000000000000..1c760f3faf6e --- /dev/null +++ b/instrumentation/servlet/servlet-common/servlet-common.gradle @@ -0,0 +1,28 @@ +apply from: "${rootDir}/gradle/instrumentation.gradle" +apply plugin: 'org.unbroken-dome.test-sets' + +muzzle { + pass { + group = "javax.servlet" + module = "servlet-api" + versions = "[2.2)" + assertInverse = true + } +} + +testSets { + latestDepTest +} + +dependencies { + compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.2' + + testCompile(project(':testing')) { + exclude group: 'org.eclipse.jetty', module: 'jetty-server' + } + testCompile group: 'org.eclipse.jetty', name: 'jetty-server', version: '7.0.0.v20091005' + testCompile group: 'org.eclipse.jetty', name: 'jetty-servlet', version: '7.0.0.v20091005' + + latestDepTestCompile group: 'org.eclipse.jetty', name: 'jetty-server', version: '+' + latestDepTestCompile group: 'org.eclipse.jetty', name: 'jetty-servlet', version: '+' +} diff --git a/instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java new file mode 100644 index 000000000000..1314fad10ea8 --- /dev/null +++ b/instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java @@ -0,0 +1,216 @@ +package io.opentelemetry.auto.instrumentation.servlet; + +import static io.opentelemetry.trace.Span.Kind.SERVER; +import static io.opentelemetry.trace.TracingContextUtils.currentContextWith; + +import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerTracer; +import io.opentelemetry.auto.config.Config; +import io.opentelemetry.auto.instrumentation.api.MoreTags; +import io.opentelemetry.auto.instrumentation.api.SpanWithScope; +import io.opentelemetry.context.propagation.HttpTextFormat; +import io.opentelemetry.trace.Span; +import io.opentelemetry.trace.Status; +import io.opentelemetry.trace.attributes.SemanticAttributes; +import java.lang.reflect.Method; +import java.net.URI; +import java.net.URISyntaxException; +import java.security.Principal; +import java.util.concurrent.atomic.AtomicBoolean; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import lombok.extern.slf4j.Slf4j; + +@Slf4j +public abstract class ServletHttpServerTracer extends + HttpServerTracer { + protected static final String DEFAULT_SPAN_NAME = "HTTP request"; + + protected String getVersion() { + return null; + } + + @Override + //TODO this violates convention + protected URI url(HttpServletRequest httpServletRequest) throws URISyntaxException { + return new URI( + httpServletRequest.getScheme(), + null, + httpServletRequest.getServerName(), + httpServletRequest.getServerPort(), + httpServletRequest.getRequestURI(), + httpServletRequest.getQueryString(), + null); + } + + public SpanWithScope startSpan(HttpServletRequest request, String originType) { + final Object spanAttr = request.getAttribute(SPAN_ATTRIBUTE); + if (spanAttr instanceof Span) { + /* + Given request already has a span associated with it. + As there should not be nested spans of kind SERVER, we should NOT create a new span here. + + But it may happen that there is no span in current Context or it is from a different trace. + E.g. in case of async servlet request processing we create span for incoming request in one thread, + but actual request continues processing happens in another thread. + Depending on servlet container implementation, this processing may again arrive into this method. + E.g. Jetty handles async requests in a way that calls HttpServlet.service method twice. + + In this case we have to put the span from the request into current context before continuing. + */ + final Span span = (Span) spanAttr; + final boolean spanContextWasLost = !sameTrace(tracer.getCurrentSpan(), span); + if (spanContextWasLost) { + //Put span from request attribute into current context. + //We did not create a new span here, so return null instead + return new SpanWithScope(null, currentContextWith(span)); + } else { + //We are inside nested servlet/filter, don't create new span + return null; + } + } + + final Span.Builder builder = tracer.spanBuilder(getSpanName(request)) + .setSpanKind(SERVER) + .setParent(extract(request, getGetter())) + //TODO Where span.origin.type is defined? + .setAttribute("span.origin.type", originType); + + Span span = builder.startSpan(); + //TODO fix parameter order + onConnection(request, span); + onRequest(span, request); + + request.setAttribute(SPAN_ATTRIBUTE, span); + log.info("Set request attribute SPAN_ATTRIBUTE"); + request.setAttribute("traceId", span.getContext().getTraceId().toLowerBase16()); + request.setAttribute("spanId", span.getContext().getSpanId().toLowerBase16()); + + return new SpanWithScope(span, currentContextWith(span)); + } + + private String getSpanName(Method method) { + return spanNameForMethod(method); + } + + private boolean sameTrace(Span oneSpan, Span otherSpan) { + return oneSpan.getContext().getTraceId().equals(otherSpan.getContext().getTraceId()); + } + + @Override + protected Integer peerPort(HttpServletRequest request) { + // HttpServletResponse doesn't have accessor for remote port prior to Servlet spec 3.0 + return null; + } + + @Override + protected String peerHostIP(HttpServletRequest request) { + return request.getRemoteAddr(); + } + + public void onRequest(Span span, HttpServletRequest request) { + span.setAttribute("servlet.path", request.getServletPath()); + span.setAttribute("servlet.context", request.getContextPath()); + //TODO what does the following line do? +// DECORATE.onContext(span, request, request.getServletContext()); + super.onRequest(span, request); + } + + private String getSpanName(HttpServletRequest request) { + if (request == null) { + return DEFAULT_SPAN_NAME; + } + final String method = method(request); + return method != null ? "HTTP " + method : DEFAULT_SPAN_NAME; + } + + protected String method(HttpServletRequest httpServletRequest) { + return httpServletRequest.getMethod(); + } + + protected HttpTextFormat.Getter getGetter() { + return new HttpServletRequestGetter(); + } + + public static class HttpServletRequestGetter implements + HttpTextFormat.Getter { + @Override + public String get(HttpServletRequest carrier, String key) { + return carrier.getHeader(key); + } + } + + public void stopSpan(HttpServletRequest request, RESPONSE response, + SpanWithScope spanWithScope, Throwable throwable) { + // Set user.principal regardless of who created this span. + final Object spanAttr = request.getAttribute(SPAN_ATTRIBUTE); + if (spanAttr instanceof Span) { + final Principal principal = request.getUserPrincipal(); + if (principal != null) { + ((Span) spanAttr).setAttribute(MoreTags.USER_NAME, principal.getName()); + } + } + + if (spanWithScope == null) { + return; + } + + final Span span = spanWithScope.getSpan(); + if (span == null) { + // This was just a re-scoping of the current thread using the span in the request attribute + // See comment in startSpan method above + spanWithScope.closeScope(); + return; + } + + onResponse(request, response, throwable, span); + spanWithScope.closeScope(); + } + + //TODO ugly, think of better way + protected void onResponse(HttpServletRequest request, RESPONSE response, + Throwable throwable, Span span) { + onResponse(response, throwable, span); + } + + protected void onResponse(RESPONSE response, Throwable throwable, Span span) { + int responseStatus = status(response); + setStatus(span, responseStatus); + if (throwable != null) { + 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 + setStatus(span, 500); + } + onError(span, unwrapThrowable(throwable)); + } + span.end(); + } + + protected abstract int status(RESPONSE response); + + public void onTimeout(AtomicBoolean responseHandled, Span span, long timeout) { + if (responseHandled.compareAndSet(false, true)) { + span.setStatus(Status.DEADLINE_EXCEEDED); + span.setAttribute("timeout", timeout); + span.end(); + } + } + + protected Throwable unwrapThrowable(Throwable throwable) { + Throwable result = throwable; + if (throwable instanceof ServletException && throwable.getCause() != null) { + result = throwable.getCause(); + } + return super.unwrapThrowable(result); + } + + private void setStatus(Span span, int status) { + SemanticAttributes.HTTP_STATUS_CODE.set(span, status); + //TODO status_message + if (Config.get().getHttpServerErrorStatuses().contains(status)) { + span.setStatus(Status.UNKNOWN); + } + } + +} diff --git a/settings.gradle b/settings.gradle index da890be76290..7f005ffec6f7 100644 --- a/settings.gradle +++ b/settings.gradle @@ -130,6 +130,7 @@ include ':instrumentation:rmi' include ':instrumentation:rxjava-1.0' include ':instrumentation:servlet' include ':instrumentation:servlet:glassfish-testing' +include ':instrumentation:servlet:servlet-common' include ':instrumentation:servlet:request-2.3' include ':instrumentation:servlet:request-3.0' // FIXME this instrumentation relied on scope listener From 6f0c6e7e146cd3d014ffe4b713b405110a983835 Mon Sep 17 00:00:00 2001 From: Nikita Salnikov-Tarnovski Date: Tue, 26 May 2020 21:31:35 +0300 Subject: [PATCH 2/4] Grizzly migrated --- .../decorator/HttpServerTracer.java | 127 +++++++++++++- .../grizzly/GrizzlyDecorator.java | 63 ------- .../GrizzlyHttpHandlerInstrumentation.java | 50 ++---- .../grizzly/GrizzlyHttpServerTracer.java | 77 ++++++++ .../servlet/v2_3/Servlet2Advice.java | 16 +- .../servlet/v2_3/Servlet2Instrumentation.java | 7 - .../servlet/v3_0/Servlet3Advice.java | 45 ++++- .../v3_0/Servlet3HttpServerTracer.java | 33 +--- .../servlet/v3_0/TagSettingAsyncListener.java | 26 +-- .../servlet/ServletHttpServerTracer.java | 165 ++++-------------- .../auto/test/InMemoryExporter.java | 5 +- 11 files changed, 314 insertions(+), 300 deletions(-) delete mode 100644 instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyDecorator.java create mode 100644 instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpServerTracer.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 143614fb82bb..5d0a421d9de8 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 @@ -16,12 +16,15 @@ package io.opentelemetry.auto.bootstrap.instrumentation.decorator; import static io.opentelemetry.OpenTelemetry.getPropagators; +import static io.opentelemetry.trace.Span.Kind.SERVER; +import static io.opentelemetry.trace.TracingContextUtils.currentContextWith; import static io.opentelemetry.trace.TracingContextUtils.getSpan; import io.grpc.Context; import io.opentelemetry.OpenTelemetry; import io.opentelemetry.auto.config.Config; import io.opentelemetry.auto.instrumentation.api.MoreTags; +import io.opentelemetry.auto.instrumentation.api.SpanWithScope; import io.opentelemetry.auto.instrumentation.api.Tags; import io.opentelemetry.context.propagation.HttpTextFormat; import io.opentelemetry.trace.Span; @@ -38,9 +41,11 @@ import lombok.extern.slf4j.Slf4j; @Slf4j -public abstract class HttpServerTracer { +public abstract class HttpServerTracer { public static final String SPAN_ATTRIBUTE = "io.opentelemetry.auto.span"; + protected static final String DEFAULT_SPAN_NAME = "HTTP request"; + protected final Tracer tracer; public HttpServerTracer() { @@ -60,12 +65,61 @@ protected void onConnection(REQUEST request, Span span) { } } + public SpanWithScope startSpan(REQUEST request, String originType) { + final Span existingSpan = findExistingSpan(request); + if (existingSpan != null) { + /* + Given request already has a span associated with it. + As there should not be nested spans of kind SERVER, we should NOT create a new span here. + + But it may happen that there is no span in current Context or it is from a different trace. + E.g. in case of async servlet request processing we create span for incoming request in one thread, + but actual request continues processing happens in another thread. + Depending on servlet container implementation, this processing may again arrive into this method. + E.g. Jetty handles async requests in a way that calls HttpServlet.service method twice. + + In this case we have to put the span from the request into current context before continuing. + */ + final boolean spanContextWasLost = !sameTrace(tracer.getCurrentSpan(), existingSpan); + if (spanContextWasLost) { + //Put span from request attribute into current context. + //We did not create a new span here, so return null instead + return new SpanWithScope(null, currentContextWith(existingSpan)); + } else { + //We are inside nested servlet/filter, don't create new span + return null; + } + } + + final Span.Builder builder = tracer.spanBuilder(getSpanName(request)) + .setSpanKind(SERVER) + .setParent(extract(request, getGetter())) + //TODO Where span.origin.type is defined? + .setAttribute("span.origin.type", originType); + + Span span = builder.startSpan(); + //TODO fix parameter order + onConnection(request, span); + onRequest(span, request); + + return new SpanWithScope(span, currentContextWith(span)); + } + + protected Span findExistingSpan(REQUEST request) { + return null; + } + + private boolean sameTrace(Span oneSpan, Span otherSpan) { + return oneSpan.getContext().getTraceId().equals(otherSpan.getContext().getTraceId()); + } + protected abstract Integer peerPort(REQUEST request); protected abstract String peerHostIP(REQUEST request); //TODO use semantic attributes public void onRequest(final Span span, final REQUEST request) { + persistSpanToRequest(span, request); span.setAttribute(Tags.HTTP_METHOD, method(request)); // Copy of HttpClientDecorator url handling @@ -112,8 +166,18 @@ public void onRequest(final Span span, final REQUEST request) { // TODO set resource name from URL. } + protected abstract void persistSpanToRequest(Span span, REQUEST request); + protected abstract URI url(REQUEST request) throws URISyntaxException; + private String getSpanName(REQUEST request) { + if (request == null) { + return DEFAULT_SPAN_NAME; + } + final String method = method(request); + return method != null ? "HTTP " + method : DEFAULT_SPAN_NAME; + } + protected abstract String method(REQUEST request); /** @@ -142,8 +206,7 @@ public String spanNameForClass(final Class clazz) { return className; } - public void onError(final Span span, final Throwable throwable) { - span.setStatus(Status.UNKNOWN); + protected void onError(final Span span, final Throwable throwable) { addThrowable(span, unwrapThrowable(throwable)); } @@ -161,6 +224,56 @@ public Span getCurrentSpan() { return tracer.getCurrentSpan(); } + protected abstract HttpTextFormat.Getter getGetter(); + + //TODO should end methods remove SPAN attribute from request as well? + public void end(SpanWithScope spanWithScope, RESPONSE response){ + if (spanWithScope == null) { + return; + } + + final Span span = spanWithScope.getSpan(); + if (span != null) { + end(span, response); + } + + spanWithScope.closeScope(); + } + + public void end(Span span, RESPONSE response){ + int responseStatus = status(response); + setStatus(span, responseStatus); + span.end(); + } + + public void endExceptionally(SpanWithScope spanWithScope, Throwable throwable, RESPONSE response){ + if (spanWithScope == null) { + return; + } + + final Span span = spanWithScope.getSpan(); + if (span != null) { + endExceptionally(span, throwable, response); + } + + spanWithScope.closeScope(); + } + + public void endExceptionally(Span span, Throwable throwable, RESPONSE response){ + int responseStatus = status(response); + 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; + } + setStatus(span, responseStatus); + onError(span, unwrapThrowable(throwable)); + span.end(); + } + + protected abstract int status(RESPONSE response); + protected Throwable unwrapThrowable(Throwable throwable) { return throwable instanceof ExecutionException ? throwable.getCause() : throwable; } @@ -171,4 +284,12 @@ public static SpanContext extract(final C carrier, final HttpTextFormat.Gett final Span span = getSpan(context); return span.getContext(); } + + private void setStatus(Span span, int status) { + SemanticAttributes.HTTP_STATUS_CODE.set(span, status); + //TODO status_message + if (Config.get().getHttpServerErrorStatuses().contains(status)) { + span.setStatus(Status.UNKNOWN); + } + } } diff --git a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyDecorator.java b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyDecorator.java deleted file mode 100644 index 1ff5284c0bed..000000000000 --- a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyDecorator.java +++ /dev/null @@ -1,63 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.opentelemetry.auto.instrumentation.grizzly; - -import io.opentelemetry.OpenTelemetry; -import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerDecorator; -import io.opentelemetry.trace.Tracer; -import java.net.URI; -import java.net.URISyntaxException; -import org.glassfish.grizzly.http.server.Request; -import org.glassfish.grizzly.http.server.Response; - -public class GrizzlyDecorator extends HttpServerDecorator { - public static final GrizzlyDecorator DECORATE = new GrizzlyDecorator(); - - public static final Tracer TRACER = - OpenTelemetry.getTracerProvider().get("io.opentelemetry.auto.grizzly-2.0"); - - @Override - protected String method(final Request request) { - return request.getMethod().getMethodString(); - } - - @Override - protected URI url(final Request request) throws URISyntaxException { - return new URI( - request.getScheme(), - null, - request.getServerName(), - request.getServerPort(), - request.getRequestURI(), - request.getQueryString(), - null); - } - - @Override - protected String peerHostIP(final Request request) { - return request.getRemoteAddr(); - } - - @Override - protected Integer peerPort(final Request request) { - return request.getRemotePort(); - } - - @Override - protected Integer status(final Response containerResponse) { - return containerResponse.getStatus(); - } -} diff --git a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java index fb7b61857b41..dbc265496ad8 100644 --- a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java +++ b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java @@ -15,13 +15,7 @@ */ package io.opentelemetry.auto.instrumentation.grizzly; -import static io.opentelemetry.auto.bootstrap.instrumentation.decorator.BaseDecorator.extract; import static io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerDecorator.SPAN_ATTRIBUTE; -import static io.opentelemetry.auto.instrumentation.grizzly.GrizzlyDecorator.DECORATE; -import static io.opentelemetry.auto.instrumentation.grizzly.GrizzlyDecorator.TRACER; -import static io.opentelemetry.auto.instrumentation.grizzly.GrizzlyRequestExtractAdapter.GETTER; -import static io.opentelemetry.trace.Span.Kind.SERVER; -import static io.opentelemetry.trace.TracingContextUtils.currentContextWith; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.named; @@ -38,10 +32,13 @@ import net.bytebuddy.matcher.ElementMatcher; import org.glassfish.grizzly.http.server.AfterServiceListener; import org.glassfish.grizzly.http.server.Request; +import org.glassfish.grizzly.http.server.Response; @AutoService(Instrumenter.class) public class GrizzlyHttpHandlerInstrumentation extends Instrumenter.Default { + public static final GrizzlyHttpServerTracer TRACER = new GrizzlyHttpServerTracer(); + public GrizzlyHttpHandlerInstrumentation() { super("grizzly"); } @@ -59,9 +56,9 @@ public ElementMatcher typeMatcher() { @Override public String[] helperClassNames() { return new String[] { - packageName + ".GrizzlyDecorator", - packageName + ".GrizzlyRequestExtractAdapter", - getClass().getName() + "$SpanClosingListener" + packageName + ".GrizzlyDecorator", + packageName + ".GrizzlyRequestExtractAdapter", + getClass().getName() + "$SpanClosingListener" }; } @@ -79,40 +76,20 @@ public static class HandleAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static SpanWithScope methodEnter(@Advice.Argument(0) final Request request) { - if (request.getAttribute(SPAN_ATTRIBUTE) != null) { - return null; - } - - final Span.Builder spanBuilder = - TRACER.spanBuilder(DECORATE.spanNameForRequest(request)).setSpanKind(SERVER); - spanBuilder.setParent(extract(request, GETTER)); - final Span span = spanBuilder.startSpan(); - DECORATE.afterStart(span); - DECORATE.onConnection(span, request); - DECORATE.onRequest(span, request); - - request.setAttribute(SPAN_ATTRIBUTE, span); - request.setAttribute("traceId", span.getContext().getTraceId().toLowerBase16()); - request.setAttribute("spanId", span.getContext().getSpanId().toLowerBase16()); request.addAfterServiceListener(SpanClosingListener.LISTENER); - return new SpanWithScope(span, currentContextWith(span)); + return TRACER.startSpan(request, null); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void methodExit( - @Advice.Enter final SpanWithScope spanWithScope, @Advice.Thrown final Throwable throwable) { - if (spanWithScope == null) { - return; - } + @Advice.Argument(1) final Response response, + @Advice.Enter final SpanWithScope spanWithScope, + @Advice.Thrown final Throwable throwable) { if (throwable != null) { - final Span span = spanWithScope.getSpan(); - DECORATE.onError(span, throwable); - DECORATE.beforeFinish(span); - span.end(); + TRACER.endExceptionally(spanWithScope, throwable, response); } - spanWithScope.closeScope(); } } @@ -124,10 +101,7 @@ public void onAfterService(final Request request) { final Object spanAttr = request.getAttribute(SPAN_ATTRIBUTE); if (spanAttr instanceof Span) { request.removeAttribute(SPAN_ATTRIBUTE); - final Span span = (Span) spanAttr; - DECORATE.onResponse(span, request.getResponse()); - DECORATE.beforeFinish(span); - span.end(); + TRACER.end((Span) spanAttr, request.getResponse()); } } } 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 new file mode 100644 index 000000000000..4168610eb477 --- /dev/null +++ b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpServerTracer.java @@ -0,0 +1,77 @@ +package io.opentelemetry.auto.instrumentation.grizzly; + +import static io.opentelemetry.auto.instrumentation.grizzly.GrizzlyRequestExtractAdapter.GETTER; + +import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerTracer; +import io.opentelemetry.context.propagation.HttpTextFormat.Getter; +import io.opentelemetry.trace.Span; +import java.net.URI; +import java.net.URISyntaxException; +import org.glassfish.grizzly.http.server.Request; +import org.glassfish.grizzly.http.server.Response; + +public class GrizzlyHttpServerTracer extends HttpServerTracer { + @Override + protected String getVersion() { + return null; + } + + @Override + protected String getInstrumentationName() { + return "io.opentelemetry.auto.grizzly-2.0"; + } + + @Override + protected Integer peerPort(Request request) { + return request.getRemotePort(); + } + + @Override + protected String peerHostIP(Request request) { + return request.getRemoteAddr(); + } + + @Override + protected void persistSpanToRequest(Span span, Request request) { + request.setAttribute(SPAN_ATTRIBUTE, span); + } + + @Override + protected URI url(Request request) throws URISyntaxException { + return new URI( + request.getScheme(), + null, + request.getServerName(), + request.getServerPort(), + request.getRequestURI(), + request.getQueryString(), + null); + } + + @Override + protected String method(Request request) { + return request.getMethod().getMethodString(); + } + + @Override + protected Getter getGetter() { + return GETTER; + } + + @Override + protected int status(Response response) { + return response.getStatus(); + } + + @Override + protected Span findExistingSpan(Request request) { + Object span = request.getAttribute(SPAN_ATTRIBUTE); + return span instanceof Span ? (Span) span : null; + } + + public void onRequest(Span span, Request request) { + request.setAttribute("traceId", span.getContext().getTraceId().toLowerBase16()); + request.setAttribute("spanId", span.getContext().getSpanId().toLowerBase16()); + super.onRequest(span, request); + } +} diff --git a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java index bf69e5bb5bf2..6f1c679f23c6 100644 --- a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java +++ b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java @@ -53,16 +53,20 @@ public static void stopSpan( @Advice.Argument(1) final ServletResponse response, @Advice.Enter final SpanWithScope spanWithScope, @Advice.Thrown final Throwable throwable) { - if (request instanceof HttpServletRequest && response instanceof HttpServletResponse) { + TRACER.setPrincipal((HttpServletRequest) request); + Integer responseStatus = InstrumentationContext .get(ServletResponse.class, Integer.class) .get(response); - TRACER.stopSpan( - (HttpServletRequest) request, - new ResponseWithStatus((HttpServletResponse) response, responseStatus), - spanWithScope, - throwable); + ResponseWithStatus responseWithStatus = new ResponseWithStatus((HttpServletResponse) response, + responseStatus); + + if (throwable == null) { + TRACER.end(spanWithScope, responseWithStatus); + } else { + TRACER.endExceptionally(spanWithScope, throwable, responseWithStatus); + } } } } diff --git a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Instrumentation.java b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Instrumentation.java index 35af63590a96..8eb49bed2d40 100644 --- a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Instrumentation.java +++ b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Instrumentation.java @@ -50,13 +50,6 @@ public ElementMatcher typeMatcher() { named("javax.servlet.FilterChain").or(named("javax.servlet.http.HttpServlet"))); } - @Override - public String[] helperClassNames() { - return new String[] { - packageName + ".Servlet2Decorator", packageName + ".HttpServletRequestExtractAdapter", - }; - } - @Override public Map contextStore() { final Map contextStores = new HashMap<>(); diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java index 2e4cf434cedf..5ef7bcef913b 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java @@ -17,6 +17,8 @@ import io.opentelemetry.auto.bootstrap.InstrumentationContext; import io.opentelemetry.auto.instrumentation.api.SpanWithScope; +import io.opentelemetry.trace.Span; +import java.util.concurrent.atomic.AtomicBoolean; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; @@ -50,12 +52,45 @@ public static void stopSpan( @Advice.Argument(1) final ServletResponse response, @Advice.Enter final SpanWithScope spanWithScope, @Advice.Thrown final Throwable throwable) { + if (spanWithScope == null) { + return; + } + if (request instanceof HttpServletRequest && response instanceof HttpServletResponse) { - TRACER.stopSpan( - (HttpServletRequest) request, - (HttpServletResponse) response, - spanWithScope, - throwable); + TRACER.setPrincipal((HttpServletRequest) request); + + if (throwable != null) { + TRACER.endExceptionally(spanWithScope, throwable, (HttpServletResponse) response); + return; + } + + //Usually Tracer takes care of this checks and of closing scopes. + //But in case of async response processing we have to handle scope in this thread, + //not in some arbitrary thread that may later take care of actual response. + Span span = spanWithScope.getSpan(); + if(span == null){ + spanWithScope.closeScope(); + return; + } + + final AtomicBoolean responseHandled = new AtomicBoolean(false); + + //In case of async servlets wait for the actual response to be ready + if (request.isAsyncStarted()) { + try { + request + .getAsyncContext() + .addListener(new TagSettingAsyncListener(responseHandled, span, TRACER)); + } catch (final IllegalStateException e) { + // org.eclipse.jetty.server.Request may throw an exception here if request became + // finished after check above. We just ignore that exception and move on. + } + } + + // Check again in case the request finished before adding the listener. + if (!request.isAsyncStarted() && responseHandled.compareAndSet(false, true)) { + TRACER.end(span, (HttpServletResponse) response); + } } } } diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java index beea90c42faf..525e061442a7 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java @@ -2,7 +2,7 @@ import io.opentelemetry.auto.instrumentation.servlet.ServletHttpServerTracer; import io.opentelemetry.trace.Span; -import java.util.concurrent.atomic.AtomicBoolean; +import io.opentelemetry.trace.Status; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -21,32 +21,9 @@ protected int status(HttpServletResponse response) { return response.getStatus(); } - @Override - protected void onResponse(HttpServletRequest request, HttpServletResponse response, - Throwable throwable, Span span) { - final AtomicBoolean responseHandled = new AtomicBoolean(false); - - //In case of async servlets wait for the actual response to be ready - if (request.isAsyncStarted()) { - try { - request.getAsyncContext() - .addListener(new TagSettingAsyncListener(responseHandled, span, this)); - } catch (final IllegalStateException e) { - // org.eclipse.jetty.server.Request may throw an exception here if request became - // finished after check above. We just ignore that exception and move on. - } - } - - // Check again in case the request finished before adding the listener. - if (!request.isAsyncStarted()) { - onResponse(response, throwable, span, responseHandled); - } - } - - public void onResponse(HttpServletResponse response, Throwable throwable, Span span, - AtomicBoolean responseHandled) { - if (responseHandled.compareAndSet(false, true)) { - super.onResponse(response, throwable, span); - } + public void onTimeout(Span span, long timeout) { + span.setStatus(Status.DEADLINE_EXCEEDED); + span.setAttribute("timeout", timeout); + span.end(); } } diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java index 84181df3e0e4..255be5b3d556 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java @@ -35,27 +35,27 @@ public TagSettingAsyncListener(final AtomicBoolean responseHandled, final Span s @Override public void onComplete(final AsyncEvent event) { - servletHttpServerTracer - .onResponse( - (HttpServletResponse) event.getSuppliedResponse(), - null, - span, - responseHandled); + if (responseHandled.compareAndSet(false, true)) { + servletHttpServerTracer.end(span, (HttpServletResponse) event.getSuppliedResponse()); + } } @Override public void onTimeout(final AsyncEvent event) { - servletHttpServerTracer.onTimeout(responseHandled, span, event.getAsyncContext().getTimeout()); + if (responseHandled.compareAndSet(false, true)) { + servletHttpServerTracer.onTimeout(span, event.getAsyncContext().getTimeout()); + } } @Override public void onError(final AsyncEvent event) { - servletHttpServerTracer - .onResponse( - (HttpServletResponse) event.getSuppliedResponse(), - event.getThrowable(), - span, - responseHandled); + if (responseHandled.compareAndSet(false, true)) { + servletHttpServerTracer + .endExceptionally( + span, + event.getThrowable(), + (HttpServletResponse) event.getSuppliedResponse()); + } } /** Transfer the listener over to the new context. */ diff --git a/instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java index 1314fad10ea8..240826f2ee83 100644 --- a/instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java @@ -1,29 +1,21 @@ package io.opentelemetry.auto.instrumentation.servlet; -import static io.opentelemetry.trace.Span.Kind.SERVER; -import static io.opentelemetry.trace.TracingContextUtils.currentContextWith; - import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerTracer; -import io.opentelemetry.auto.config.Config; import io.opentelemetry.auto.instrumentation.api.MoreTags; -import io.opentelemetry.auto.instrumentation.api.SpanWithScope; import io.opentelemetry.context.propagation.HttpTextFormat; +import io.opentelemetry.context.propagation.HttpTextFormat.Getter; import io.opentelemetry.trace.Span; -import io.opentelemetry.trace.Status; -import io.opentelemetry.trace.attributes.SemanticAttributes; import java.lang.reflect.Method; import java.net.URI; import java.net.URISyntaxException; import java.security.Principal; -import java.util.concurrent.atomic.AtomicBoolean; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import lombok.extern.slf4j.Slf4j; @Slf4j public abstract class ServletHttpServerTracer extends - HttpServerTracer { - protected static final String DEFAULT_SPAN_NAME = "HTTP request"; + HttpServerTracer { protected String getVersion() { return null; @@ -42,58 +34,19 @@ protected URI url(HttpServletRequest httpServletRequest) throws URISyntaxExcepti null); } - public SpanWithScope startSpan(HttpServletRequest request, String originType) { - final Object spanAttr = request.getAttribute(SPAN_ATTRIBUTE); - if (spanAttr instanceof Span) { - /* - Given request already has a span associated with it. - As there should not be nested spans of kind SERVER, we should NOT create a new span here. - - But it may happen that there is no span in current Context or it is from a different trace. - E.g. in case of async servlet request processing we create span for incoming request in one thread, - but actual request continues processing happens in another thread. - Depending on servlet container implementation, this processing may again arrive into this method. - E.g. Jetty handles async requests in a way that calls HttpServlet.service method twice. - - In this case we have to put the span from the request into current context before continuing. - */ - final Span span = (Span) spanAttr; - final boolean spanContextWasLost = !sameTrace(tracer.getCurrentSpan(), span); - if (spanContextWasLost) { - //Put span from request attribute into current context. - //We did not create a new span here, so return null instead - return new SpanWithScope(null, currentContextWith(span)); - } else { - //We are inside nested servlet/filter, don't create new span - return null; - } - } - - final Span.Builder builder = tracer.spanBuilder(getSpanName(request)) - .setSpanKind(SERVER) - .setParent(extract(request, getGetter())) - //TODO Where span.origin.type is defined? - .setAttribute("span.origin.type", originType); - - Span span = builder.startSpan(); - //TODO fix parameter order - onConnection(request, span); - onRequest(span, request); - - request.setAttribute(SPAN_ATTRIBUTE, span); - log.info("Set request attribute SPAN_ATTRIBUTE"); - request.setAttribute("traceId", span.getContext().getTraceId().toLowerBase16()); - request.setAttribute("spanId", span.getContext().getSpanId().toLowerBase16()); - - return new SpanWithScope(span, currentContextWith(span)); - } - private String getSpanName(Method method) { return spanNameForMethod(method); } - private boolean sameTrace(Span oneSpan, Span otherSpan) { - return oneSpan.getContext().getTraceId().equals(otherSpan.getContext().getTraceId()); + @Override + protected Span findExistingSpan(HttpServletRequest request) { + Object span = request.getAttribute(SPAN_ATTRIBUTE); + return span instanceof Span ? (Span) span : null; + } + + @Override + protected void persistSpanToRequest(Span span, HttpServletRequest request) { + request.setAttribute(SPAN_ATTRIBUTE, span); } @Override @@ -107,7 +60,17 @@ protected String peerHostIP(HttpServletRequest request) { return request.getRemoteAddr(); } + @Override + protected String method(HttpServletRequest request) { + return request.getMethod(); + } + public void onRequest(Span span, HttpServletRequest request) { + //TODO why? + request.setAttribute("traceId", span.getContext().getTraceId().toLowerBase16()); + request.setAttribute("spanId", span.getContext().getSpanId().toLowerBase16()); + + //TODO why? they are not in semantic convention, right? span.setAttribute("servlet.path", request.getServletPath()); span.setAttribute("servlet.context", request.getContextPath()); //TODO what does the following line do? @@ -115,19 +78,8 @@ public void onRequest(Span span, HttpServletRequest request) { super.onRequest(span, request); } - private String getSpanName(HttpServletRequest request) { - if (request == null) { - return DEFAULT_SPAN_NAME; - } - final String method = method(request); - return method != null ? "HTTP " + method : DEFAULT_SPAN_NAME; - } - - protected String method(HttpServletRequest httpServletRequest) { - return httpServletRequest.getMethod(); - } - - protected HttpTextFormat.Getter getGetter() { + @Override + protected Getter getGetter() { return new HttpServletRequestGetter(); } @@ -139,64 +91,6 @@ public String get(HttpServletRequest carrier, String key) { } } - public void stopSpan(HttpServletRequest request, RESPONSE response, - SpanWithScope spanWithScope, Throwable throwable) { - // Set user.principal regardless of who created this span. - final Object spanAttr = request.getAttribute(SPAN_ATTRIBUTE); - if (spanAttr instanceof Span) { - final Principal principal = request.getUserPrincipal(); - if (principal != null) { - ((Span) spanAttr).setAttribute(MoreTags.USER_NAME, principal.getName()); - } - } - - if (spanWithScope == null) { - return; - } - - final Span span = spanWithScope.getSpan(); - if (span == null) { - // This was just a re-scoping of the current thread using the span in the request attribute - // See comment in startSpan method above - spanWithScope.closeScope(); - return; - } - - onResponse(request, response, throwable, span); - spanWithScope.closeScope(); - } - - //TODO ugly, think of better way - protected void onResponse(HttpServletRequest request, RESPONSE response, - Throwable throwable, Span span) { - onResponse(response, throwable, span); - } - - protected void onResponse(RESPONSE response, Throwable throwable, Span span) { - int responseStatus = status(response); - setStatus(span, responseStatus); - if (throwable != null) { - 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 - setStatus(span, 500); - } - onError(span, unwrapThrowable(throwable)); - } - span.end(); - } - - protected abstract int status(RESPONSE response); - - public void onTimeout(AtomicBoolean responseHandled, Span span, long timeout) { - if (responseHandled.compareAndSet(false, true)) { - span.setStatus(Status.DEADLINE_EXCEEDED); - span.setAttribute("timeout", timeout); - span.end(); - } - } - protected Throwable unwrapThrowable(Throwable throwable) { Throwable result = throwable; if (throwable instanceof ServletException && throwable.getCause() != null) { @@ -205,12 +99,13 @@ protected Throwable unwrapThrowable(Throwable throwable) { return super.unwrapThrowable(result); } - private void setStatus(Span span, int status) { - SemanticAttributes.HTTP_STATUS_CODE.set(span, status); - //TODO status_message - if (Config.get().getHttpServerErrorStatuses().contains(status)) { - span.setStatus(Status.UNKNOWN); + public void setPrincipal(HttpServletRequest request) { + final Span existingSpan = findExistingSpan(request); + if (existingSpan != null) { + final Principal principal = request.getUserPrincipal(); + if (principal != null) { + existingSpan.setAttribute(MoreTags.USER_NAME, principal.getName()); + } } } - } diff --git a/testing/src/main/groovy/io/opentelemetry/auto/test/InMemoryExporter.java b/testing/src/main/groovy/io/opentelemetry/auto/test/InMemoryExporter.java index 6994cfb155b2..5700612e9426 100644 --- a/testing/src/main/groovy/io/opentelemetry/auto/test/InMemoryExporter.java +++ b/testing/src/main/groovy/io/opentelemetry/auto/test/InMemoryExporter.java @@ -60,11 +60,12 @@ public class InMemoryExporter implements SpanProcessor { public void onStart(final ReadableSpan readableSpan) { final SpanData sd = readableSpan.toSpanData(); log.debug( - ">>> SPAN START: {} id={} traceid={} parent={}", + ">>> SPAN START: {} id={} traceid={} parent={}, library={}", sd.getName(), sd.getSpanId().toLowerBase16(), sd.getTraceId().toLowerBase16(), - sd.getParentSpanId().toLowerBase16()); + sd.getParentSpanId().toLowerBase16(), + sd.getInstrumentationLibraryInfo()); synchronized (tracesLock) { spanOrders.put(readableSpan.getSpanContext().getSpanId(), nextSpanOrder.getAndIncrement()); } From 83624f935b76b4972a831473689f5936dd66e9a1 Mon Sep 17 00:00:00 2001 From: Nikita Salnikov-Tarnovski Date: Wed, 27 May 2020 14:10:49 +0300 Subject: [PATCH 3/4] Fix tests after merge --- .../instrumentation/decorator/HttpServerTracer.java | 4 ++-- .../grizzly/GrizzlyHttpHandlerInstrumentation.java | 7 +++++-- .../auto/instrumentation/servlet/v2_3/Servlet2Advice.java | 4 +++- .../auto/instrumentation/servlet/v3_0/Servlet3Advice.java | 4 +++- 4 files changed, 13 insertions(+), 6 deletions(-) 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 5d0a421d9de8..5e0676c7cb59 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 @@ -65,7 +65,7 @@ protected void onConnection(REQUEST request, Span span) { } } - public SpanWithScope startSpan(REQUEST request, String originType) { + public SpanWithScope startSpan(REQUEST request, Method origin, String originType) { final Span existingSpan = findExistingSpan(request); if (existingSpan != null) { /* @@ -91,7 +91,7 @@ public SpanWithScope startSpan(REQUEST request, String originType) { } } - final Span.Builder builder = tracer.spanBuilder(getSpanName(request)) + final Span.Builder builder = tracer.spanBuilder(spanNameForMethod(origin)) .setSpanKind(SERVER) .setParent(extract(request, getGetter())) //TODO Where span.origin.type is defined? diff --git a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java index f1e7c877e3fa..c7ead88baad6 100644 --- a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java +++ b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java @@ -25,6 +25,7 @@ import io.opentelemetry.auto.instrumentation.api.SpanWithScope; import io.opentelemetry.auto.tooling.Instrumenter; import io.opentelemetry.trace.Span; +import java.lang.reflect.Method; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -75,10 +76,12 @@ public Map, String> transfor public static class HandleAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) - public static SpanWithScope methodEnter(@Advice.Argument(0) final Request request) { + public static SpanWithScope methodEnter( + @Advice.Origin final Method method, + @Advice.Argument(0) final Request request) { request.addAfterServiceListener(SpanClosingListener.LISTENER); - return TRACER.startSpan(request, null); + return TRACER.startSpan(request, method, null); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) diff --git a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java index 6f1c679f23c6..74269edcd13a 100644 --- a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java +++ b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java @@ -17,6 +17,7 @@ import io.opentelemetry.auto.bootstrap.InstrumentationContext; import io.opentelemetry.auto.instrumentation.api.SpanWithScope; +import java.lang.reflect.Method; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; @@ -30,6 +31,7 @@ public class Servlet2Advice { @Advice.OnMethodEnter(suppress = Throwable.class) public static SpanWithScope onEnter( @Advice.This final Object servlet, + @Advice.Origin final Method method, @Advice.Argument(0) final ServletRequest request, @Advice.Argument(value = 1, typing = Assigner.Typing.DYNAMIC) final ServletResponse response) { @@ -43,7 +45,7 @@ public static SpanWithScope onEnter( InstrumentationContext.get(HttpServletResponse.class, HttpServletRequest.class) .put((HttpServletResponse) response, httpServletRequest); - return TRACER.startSpan(httpServletRequest, servlet.getClass().getName()); + return TRACER.startSpan(httpServletRequest, method, servlet.getClass().getName()); } diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java index 5ef7bcef913b..562a399c2f55 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java @@ -18,6 +18,7 @@ import io.opentelemetry.auto.bootstrap.InstrumentationContext; import io.opentelemetry.auto.instrumentation.api.SpanWithScope; import io.opentelemetry.trace.Span; +import java.lang.reflect.Method; import java.util.concurrent.atomic.AtomicBoolean; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; @@ -31,6 +32,7 @@ public class Servlet3Advice { @Advice.OnMethodEnter(suppress = Throwable.class) public static SpanWithScope onEnter( @Advice.This final Object servlet, + @Advice.Origin final Method method, @Advice.Argument(0) final ServletRequest request, @Advice.Argument(1) final ServletResponse response) { if (!(request instanceof HttpServletRequest)) { @@ -43,7 +45,7 @@ public static SpanWithScope onEnter( InstrumentationContext.get(HttpServletResponse.class, HttpServletRequest.class) .put((HttpServletResponse) response, httpServletRequest); - return TRACER.startSpan(httpServletRequest, servlet.getClass().getName()); + return TRACER.startSpan(httpServletRequest, method, servlet.getClass().getName()); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) From e413de31812e49a49abd3d2ec1950d233b333f30 Mon Sep 17 00:00:00 2001 From: Nikita Salnikov-Tarnovski Date: Thu, 28 May 2020 10:46:24 +0300 Subject: [PATCH 4/4] . --- .../decorator/HttpServerDecorator.java | 1 + .../typed/server/http/HttpServerSpan.java | 147 ++++++++++++++++++ .../server/http/HttpServerSpanWithScope.java | 21 +++ .../typed/server/http}/HttpServerTracer.java | 119 ++++---------- .../GrizzlyHttpHandlerInstrumentation.java | 24 +-- .../grizzly/GrizzlyHttpServerTracer.java | 15 +- .../servlet/v2_3/Servlet2Advice.java | 19 ++- .../servlet/v3_0/Servlet3Advice.java | 21 +-- .../v3_0/Servlet3HttpServerTracer.java | 6 +- .../servlet/v3_0/TagSettingAsyncListener.java | 6 +- .../servlet/ServletHttpServerTracer.java | 19 +-- 11 files changed, 263 insertions(+), 135 deletions(-) create mode 100644 agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http/HttpServerSpan.java create mode 100644 agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http/HttpServerSpanWithScope.java rename {agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator => agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http}/HttpServerTracer.java (70%) diff --git a/agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerDecorator.java b/agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerDecorator.java index 9b153ba20daf..598e19dc56ac 100644 --- a/agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerDecorator.java +++ b/agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerDecorator.java @@ -25,6 +25,7 @@ import lombok.extern.slf4j.Slf4j; @Slf4j +@Deprecated public abstract class HttpServerDecorator extends ServerDecorator { public static final String SPAN_ATTRIBUTE = "io.opentelemetry.auto.span"; public static final String RESPONSE_ATTRIBUTE = "io.opentelemetry.auto.response"; diff --git a/agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http/HttpServerSpan.java b/agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http/HttpServerSpan.java new file mode 100644 index 000000000000..89cbaf1717b6 --- /dev/null +++ b/agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http/HttpServerSpan.java @@ -0,0 +1,147 @@ +package io.opentelemetry.auto.typed.server.http; + +import io.opentelemetry.auto.config.Config; +import io.opentelemetry.auto.instrumentation.api.MoreTags; +import io.opentelemetry.auto.typed.base.DelegatingSpan; +import io.opentelemetry.trace.Span; +import io.opentelemetry.trace.SpanContext; +import io.opentelemetry.trace.SpanId; +import io.opentelemetry.trace.Status; +import io.opentelemetry.trace.TraceId; +import io.opentelemetry.trace.Tracer; +import io.opentelemetry.trace.attributes.SemanticAttributes; +import java.io.PrintWriter; +import java.io.StringWriter; + +public class HttpServerSpan extends DelegatingSpan { + + private HttpServerSpan(Span delegate) { + super(delegate); + } + + public TraceId getTraceId() { + return delegate.getContext().getTraceId(); + } + + public SpanId getSpanId() { + return delegate.getContext().getSpanId(); + } + + //TODO review network attributes + public HttpServerSpan setPeerIp(String netPeerIp) { + SemanticAttributes.NET_PEER_IP.set(delegate, netPeerIp); + return this; + } + + public void setPeerPort(Integer port) { + //TODO check this null handling and write documentation + // Negative or Zero ports might represent an unset/null value for an int type. Skip setting. + if (port != null && port > 0) { + SemanticAttributes.NET_PEER_PORT.set(delegate, port); + } + } + + public void setMethod(String method) { + SemanticAttributes.HTTP_METHOD.set(delegate, method); + } + + //TODO Specification does not recommend using this attribute + //See https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#http-server-semantic-conventions + public void setUrl(String url) { + SemanticAttributes.HTTP_URL.set(delegate, url); + } +//See TODO above +// public void setUrl(String scheme, String host, String target){ +// SemanticAttributes.HTTP_SCHEME.set(delegate, scheme); +// SemanticAttributes.HTTP_HOST.set(delegate, host); +// SemanticAttributes.HTTP_TARGET.set(delegate, target); +// } +//public void setUrl(String scheme, String serverName, Integer port, String target){ +// SemanticAttributes.HTTP_SCHEME.set(delegate, scheme); +// SemanticAttributes.HTTP_SERVER_NAME.set(delegate, serverName); +// SemanticAttributes.NET_HOST_PORT.set(delegate, port); +// SemanticAttributes.HTTP_TARGET.set(delegate, target); +//} + + public void setTarget(String target) { + SemanticAttributes.HTTP_TARGET.set(delegate, target); + } + + public void setHost(String host) { + SemanticAttributes.HTTP_HOST.set(delegate, host); + } + + public void setScheme(String scheme) { + SemanticAttributes.HTTP_SCHEME.set(delegate, scheme); + } + + public void setStatus(Status status) { + delegate.setStatus(status); + } + + public void setStatusCode(int code) { + SemanticAttributes.HTTP_STATUS_CODE.set(delegate, code); + //TODO think about it and at least document + if (Config.get().getHttpServerErrorStatuses().contains(code)) { + setStatus(Status.UNKNOWN); + } + } + + public void setStatusText(String statusLine) { + SemanticAttributes.HTTP_STATUS_TEXT.set(delegate, statusLine); + } + + public void setFlavor(String flavor) { + SemanticAttributes.HTTP_FLAVOR.set(delegate, flavor); + } + + public void setUserAgent(String userAgent) { + SemanticAttributes.HTTP_USER_AGENT.set(delegate, userAgent); + } + + public void setServerName(String serverName) { + SemanticAttributes.HTTP_SERVER_NAME.set(delegate, serverName); + } + + public void setRoute(String route) { + SemanticAttributes.HTTP_ROUTE.set(delegate, route); + } + + public void setClientIp(String clientIp) { + SemanticAttributes.HTTP_CLIENT_IP.set(delegate, clientIp); + } + + public void setError(final Throwable throwable) { + delegate.setAttribute(MoreTags.ERROR_MSG, throwable.getMessage()); + delegate.setAttribute(MoreTags.ERROR_TYPE, throwable.getClass().getName()); + + final StringWriter errorString = new StringWriter(); + throwable.printStackTrace(new PrintWriter(errorString)); + delegate.setAttribute(MoreTags.ERROR_STACK, errorString.toString()); + } + + public static HttpServerSpan create(Tracer tracer, String spanName, SpanContext parent) { + return new HttpServerSpan(tracer + .spanBuilder(spanName) + .setSpanKind(Kind.SERVER) + .setParent(parent) + .startSpan()); + } + + public void end(int responseStatus, Throwable throwable) { + setStatusCode(responseStatus); + //TODO status_message + setError(throwable); + delegate.end(); + } + + public void end(int responseStatus) { + setStatusCode(responseStatus); + delegate.end(); + } + + //TODO Only use if response status is uknown + public void end() { + delegate.end(); + } +} diff --git a/agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http/HttpServerSpanWithScope.java b/agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http/HttpServerSpanWithScope.java new file mode 100644 index 000000000000..54ad9bcd094f --- /dev/null +++ b/agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http/HttpServerSpanWithScope.java @@ -0,0 +1,21 @@ +package io.opentelemetry.auto.typed.server.http; + +import io.opentelemetry.context.Scope; + +public class HttpServerSpanWithScope { + private final HttpServerSpan span; + private final Scope scope; + + public HttpServerSpanWithScope(final HttpServerSpan span, final Scope scope) { + this.span = span; + this.scope = scope; + } + + public HttpServerSpan getSpan() { + return span; + } + + public void closeScope() { + scope.close(); + } +} diff --git a/agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerTracer.java b/agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http/HttpServerTracer.java similarity index 70% rename from agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerTracer.java rename to agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http/HttpServerTracer.java index 5e0676c7cb59..58de321b0dcf 100644 --- a/agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerTracer.java +++ b/agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/http/HttpServerTracer.java @@ -13,10 +13,9 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package io.opentelemetry.auto.bootstrap.instrumentation.decorator; +package io.opentelemetry.auto.typed.server.http; import static io.opentelemetry.OpenTelemetry.getPropagators; -import static io.opentelemetry.trace.Span.Kind.SERVER; import static io.opentelemetry.trace.TracingContextUtils.currentContextWith; import static io.opentelemetry.trace.TracingContextUtils.getSpan; @@ -24,16 +23,10 @@ import io.opentelemetry.OpenTelemetry; import io.opentelemetry.auto.config.Config; import io.opentelemetry.auto.instrumentation.api.MoreTags; -import io.opentelemetry.auto.instrumentation.api.SpanWithScope; -import io.opentelemetry.auto.instrumentation.api.Tags; import io.opentelemetry.context.propagation.HttpTextFormat; import io.opentelemetry.trace.Span; import io.opentelemetry.trace.SpanContext; -import io.opentelemetry.trace.Status; import io.opentelemetry.trace.Tracer; -import io.opentelemetry.trace.attributes.SemanticAttributes; -import java.io.PrintWriter; -import java.io.StringWriter; import java.lang.reflect.Method; import java.net.URI; import java.net.URISyntaxException; @@ -52,21 +45,8 @@ public HttpServerTracer() { tracer = OpenTelemetry.getTracerProvider().get(getInstrumentationName(), getVersion()); } - protected abstract String getVersion(); - - protected abstract String getInstrumentationName(); - - protected void onConnection(REQUEST request, Span span) { - SemanticAttributes.NET_PEER_IP.set(span, peerHostIP(request)); - final Integer port = peerPort(request); - // Negative or Zero ports might represent an unset/null value for an int type. Skip setting. - if (port != null && port > 0) { - SemanticAttributes.NET_PEER_PORT.set(span, port); - } - } - - public SpanWithScope startSpan(REQUEST request, Method origin, String originType) { - final Span existingSpan = findExistingSpan(request); + public HttpServerSpanWithScope startSpan(REQUEST request, Method origin, String originType) { + final HttpServerSpan existingSpan = findExistingSpan(request); if (existingSpan != null) { /* Given request already has a span associated with it. @@ -84,28 +64,25 @@ public SpanWithScope startSpan(REQUEST request, Method origin, String originType if (spanContextWasLost) { //Put span from request attribute into current context. //We did not create a new span here, so return null instead - return new SpanWithScope(null, currentContextWith(existingSpan)); + return new HttpServerSpanWithScope(null, currentContextWith(existingSpan)); } else { //We are inside nested servlet/filter, don't create new span return null; } } - final Span.Builder builder = tracer.spanBuilder(spanNameForMethod(origin)) - .setSpanKind(SERVER) - .setParent(extract(request, getGetter())) + HttpServerSpan span = HttpServerSpan + .create(tracer, spanNameForMethod(origin), extract(request, getGetter())); //TODO Where span.origin.type is defined? - .setAttribute("span.origin.type", originType); - - Span span = builder.startSpan(); - //TODO fix parameter order - onConnection(request, span); + span.setAttribute("span.origin.type", originType); + span.setPeerIp(peerHostIP(request)); + span.setPeerPort(peerPort(request)); onRequest(span, request); - return new SpanWithScope(span, currentContextWith(span)); + return new HttpServerSpanWithScope(span, currentContextWith(span)); } - protected Span findExistingSpan(REQUEST request) { + protected HttpServerSpan findExistingSpan(REQUEST request) { return null; } @@ -113,14 +90,10 @@ private boolean sameTrace(Span oneSpan, Span otherSpan) { return oneSpan.getContext().getTraceId().equals(otherSpan.getContext().getTraceId()); } - protected abstract Integer peerPort(REQUEST request); - - protected abstract String peerHostIP(REQUEST request); - //TODO use semantic attributes - public void onRequest(final Span span, final REQUEST request) { + public void onRequest(final HttpServerSpan span, final REQUEST request) { persistSpanToRequest(span, request); - span.setAttribute(Tags.HTTP_METHOD, method(request)); + span.setMethod(method(request)); // Copy of HttpClientDecorator url handling try { @@ -153,7 +126,7 @@ public void onRequest(final Span span, final REQUEST request) { urlBuilder.append("#").append(fragment); } - span.setAttribute(Tags.HTTP_URL, urlBuilder.toString()); + span.setUrl(urlBuilder.toString()); if (Config.get().isHttpServerTagQueryString()) { span.setAttribute(MoreTags.HTTP_QUERY, url.getQuery()); @@ -166,10 +139,6 @@ public void onRequest(final Span span, final REQUEST request) { // TODO set resource name from URL. } - protected abstract void persistSpanToRequest(Span span, REQUEST request); - - protected abstract URI url(REQUEST request) throws URISyntaxException; - private String getSpanName(REQUEST request) { if (request == null) { return DEFAULT_SPAN_NAME; @@ -177,9 +146,6 @@ private String getSpanName(REQUEST request) { final String method = method(request); return method != null ? "HTTP " + method : DEFAULT_SPAN_NAME; } - - protected abstract String method(REQUEST request); - /** * This method is used to generate an acceptable span (operation) name based on a given method * reference. Anonymous classes are named based on their parent. @@ -205,34 +171,22 @@ public String spanNameForClass(final Class clazz) { } return className; } - - protected void onError(final Span span, final Throwable throwable) { - addThrowable(span, unwrapThrowable(throwable)); - } - - //TODO semantic attributes - public static void addThrowable(final Span span, final Throwable throwable) { - span.setAttribute(MoreTags.ERROR_MSG, throwable.getMessage()); - span.setAttribute(MoreTags.ERROR_TYPE, throwable.getClass().getName()); - - final StringWriter errorString = new StringWriter(); - throwable.printStackTrace(new PrintWriter(errorString)); - span.setAttribute(MoreTags.ERROR_STACK, errorString.toString()); + protected void onError(final HttpServerSpan span, final Throwable throwable) { + span.setError(unwrapThrowable(throwable)); } public Span getCurrentSpan() { return tracer.getCurrentSpan(); } - protected abstract HttpTextFormat.Getter getGetter(); //TODO should end methods remove SPAN attribute from request as well? - public void end(SpanWithScope spanWithScope, RESPONSE response){ + public void end(HttpServerSpanWithScope spanWithScope, RESPONSE response) { if (spanWithScope == null) { return; } - final Span span = spanWithScope.getSpan(); + final HttpServerSpan span = spanWithScope.getSpan(); if (span != null) { end(span, response); } @@ -240,18 +194,16 @@ public void end(SpanWithScope spanWithScope, RESPONSE response){ spanWithScope.closeScope(); } - public void end(Span span, RESPONSE response){ - int responseStatus = status(response); - setStatus(span, responseStatus); - span.end(); + public void end(HttpServerSpan span, RESPONSE response) { + span.end(status(response)); } - - public void endExceptionally(SpanWithScope spanWithScope, Throwable throwable, RESPONSE response){ + public void endExceptionally(HttpServerSpanWithScope spanWithScope, Throwable throwable, + RESPONSE response) { if (spanWithScope == null) { return; } - final Span span = spanWithScope.getSpan(); + final HttpServerSpan span = spanWithScope.getSpan(); if (span != null) { endExceptionally(span, throwable, response); } @@ -259,7 +211,7 @@ public void endExceptionally(SpanWithScope spanWithScope, Throwable throwable, R spanWithScope.closeScope(); } - public void endExceptionally(Span span, Throwable throwable, RESPONSE response){ + public void endExceptionally(HttpServerSpan span, Throwable throwable, RESPONSE response) { int responseStatus = status(response); if (responseStatus == 200) { //TODO I think this is wrong. @@ -267,13 +219,8 @@ public void endExceptionally(Span span, Throwable throwable, RESPONSE response){ //We may change span status, but not http_status attribute responseStatus = 500; } - setStatus(span, responseStatus); - onError(span, unwrapThrowable(throwable)); - span.end(); + span.end(responseStatus, unwrapThrowable(throwable)); } - - protected abstract int status(RESPONSE response); - protected Throwable unwrapThrowable(Throwable throwable) { return throwable instanceof ExecutionException ? throwable.getCause() : throwable; } @@ -284,12 +231,14 @@ public static SpanContext extract(final C carrier, final HttpTextFormat.Gett final Span span = getSpan(context); return span.getContext(); } + protected abstract String getVersion(); + protected abstract String getInstrumentationName(); + protected abstract void persistSpanToRequest(HttpServerSpan span, REQUEST request); - private void setStatus(Span span, int status) { - SemanticAttributes.HTTP_STATUS_CODE.set(span, status); - //TODO status_message - if (Config.get().getHttpServerErrorStatuses().contains(status)) { - span.setStatus(Status.UNKNOWN); - } - } + protected abstract int status(RESPONSE response); + protected abstract Integer peerPort(REQUEST request); + protected abstract String peerHostIP(REQUEST request); + protected abstract URI url(REQUEST request) throws URISyntaxException; + protected abstract HttpTextFormat.Getter getGetter(); + protected abstract String method(REQUEST request); } diff --git a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java index c7ead88baad6..7fe0bc98af8b 100644 --- a/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java +++ b/instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java @@ -22,12 +22,14 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import com.google.auto.service.AutoService; -import io.opentelemetry.auto.instrumentation.api.SpanWithScope; import io.opentelemetry.auto.tooling.Instrumenter; -import io.opentelemetry.trace.Span; +import io.opentelemetry.auto.typed.server.http.HttpServerSpan; +import io.opentelemetry.auto.typed.server.http.HttpServerSpanWithScope; import java.lang.reflect.Method; import java.util.Map; import net.bytebuddy.asm.Advice; +import net.bytebuddy.asm.Advice.Argument; +import net.bytebuddy.asm.Advice.Origin; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -57,9 +59,9 @@ public ElementMatcher typeMatcher() { @Override public String[] helperClassNames() { return new String[] { - packageName + ".GrizzlyDecorator", - packageName + ".GrizzlyRequestExtractAdapter", - getClass().getName() + "$SpanClosingListener" + packageName + ".GrizzlyDecorator", + packageName + ".GrizzlyRequestExtractAdapter", + getClass().getName() + "$SpanClosingListener" }; } @@ -76,9 +78,9 @@ public Map, String> transfor public static class HandleAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) - public static SpanWithScope methodEnter( - @Advice.Origin final Method method, - @Advice.Argument(0) final Request request) { + public static HttpServerSpanWithScope methodEnter( + @Origin final Method method, + @Argument(0) final Request request) { request.addAfterServiceListener(SpanClosingListener.LISTENER); return TRACER.startSpan(request, method, null); @@ -87,7 +89,7 @@ public static SpanWithScope methodEnter( @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void methodExit( @Advice.Argument(1) final Response response, - @Advice.Enter final SpanWithScope spanWithScope, + @Advice.Enter final HttpServerSpanWithScope spanWithScope, @Advice.Thrown final Throwable throwable) { if (throwable != null) { @@ -102,9 +104,9 @@ public static class SpanClosingListener implements AfterServiceListener { @Override public void onAfterService(final Request request) { final Object spanAttr = request.getAttribute(SPAN_ATTRIBUTE); - if (spanAttr instanceof Span) { + if (spanAttr instanceof HttpServerSpan) { request.removeAttribute(SPAN_ATTRIBUTE); - TRACER.end((Span) spanAttr, request.getResponse()); + TRACER.end((HttpServerSpan) spanAttr, request.getResponse()); } } } 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 4168610eb477..193d65930dcc 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 @@ -2,7 +2,8 @@ import static io.opentelemetry.auto.instrumentation.grizzly.GrizzlyRequestExtractAdapter.GETTER; -import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerTracer; +import io.opentelemetry.auto.typed.server.http.HttpServerSpan; +import io.opentelemetry.auto.typed.server.http.HttpServerTracer; import io.opentelemetry.context.propagation.HttpTextFormat.Getter; import io.opentelemetry.trace.Span; import java.net.URI; @@ -32,7 +33,7 @@ protected String peerHostIP(Request request) { } @Override - protected void persistSpanToRequest(Span span, Request request) { + protected void persistSpanToRequest(HttpServerSpan span, Request request) { request.setAttribute(SPAN_ATTRIBUTE, span); } @@ -64,14 +65,14 @@ protected int status(Response response) { } @Override - protected Span findExistingSpan(Request request) { + protected HttpServerSpan findExistingSpan(Request request) { Object span = request.getAttribute(SPAN_ATTRIBUTE); - return span instanceof Span ? (Span) span : null; + return span instanceof Span ? (HttpServerSpan) span : null; } - public void onRequest(Span span, Request request) { - request.setAttribute("traceId", span.getContext().getTraceId().toLowerBase16()); - request.setAttribute("spanId", span.getContext().getSpanId().toLowerBase16()); + public void onRequest(HttpServerSpan span, Request request) { + request.setAttribute("traceId", span.getTraceId().toLowerBase16()); + request.setAttribute("spanId", span.getSpanId().toLowerBase16()); super.onRequest(span, request); } } diff --git a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java index 74269edcd13a..0e197a097c09 100644 --- a/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java +++ b/instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java @@ -16,24 +16,27 @@ package io.opentelemetry.auto.instrumentation.servlet.v2_3; import io.opentelemetry.auto.bootstrap.InstrumentationContext; -import io.opentelemetry.auto.instrumentation.api.SpanWithScope; +import io.opentelemetry.auto.typed.server.http.HttpServerSpanWithScope; import java.lang.reflect.Method; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import net.bytebuddy.asm.Advice; -import net.bytebuddy.implementation.bytecode.assign.Assigner; +import net.bytebuddy.asm.Advice.Argument; +import net.bytebuddy.asm.Advice.Origin; +import net.bytebuddy.asm.Advice.This; +import net.bytebuddy.implementation.bytecode.assign.Assigner.Typing; public class Servlet2Advice { public static final Servlet2HttpServerTracer TRACER = new Servlet2HttpServerTracer(); @Advice.OnMethodEnter(suppress = Throwable.class) - public static SpanWithScope onEnter( - @Advice.This final Object servlet, - @Advice.Origin final Method method, - @Advice.Argument(0) final ServletRequest request, - @Advice.Argument(value = 1, typing = Assigner.Typing.DYNAMIC) final ServletResponse response) { + public static HttpServerSpanWithScope onEnter( + @This final Object servlet, + @Origin final Method method, + @Argument(0) final ServletRequest request, + @Argument(value = 1, typing = Typing.DYNAMIC) final ServletResponse response) { if (!(request instanceof HttpServletRequest)) { return null; @@ -53,7 +56,7 @@ public static SpanWithScope onEnter( public static void stopSpan( @Advice.Argument(0) final ServletRequest request, @Advice.Argument(1) final ServletResponse response, - @Advice.Enter final SpanWithScope spanWithScope, + @Advice.Enter final HttpServerSpanWithScope spanWithScope, @Advice.Thrown final Throwable throwable) { if (request instanceof HttpServletRequest && response instanceof HttpServletResponse) { TRACER.setPrincipal((HttpServletRequest) request); diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java index 562a399c2f55..e98979b4046c 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java @@ -16,8 +16,8 @@ package io.opentelemetry.auto.instrumentation.servlet.v3_0; import io.opentelemetry.auto.bootstrap.InstrumentationContext; -import io.opentelemetry.auto.instrumentation.api.SpanWithScope; -import io.opentelemetry.trace.Span; +import io.opentelemetry.auto.typed.server.http.HttpServerSpan; +import io.opentelemetry.auto.typed.server.http.HttpServerSpanWithScope; import java.lang.reflect.Method; import java.util.concurrent.atomic.AtomicBoolean; import javax.servlet.ServletRequest; @@ -25,16 +25,19 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import net.bytebuddy.asm.Advice; +import net.bytebuddy.asm.Advice.Argument; +import net.bytebuddy.asm.Advice.Origin; +import net.bytebuddy.asm.Advice.This; public class Servlet3Advice { public static final Servlet3HttpServerTracer TRACER = new Servlet3HttpServerTracer(); @Advice.OnMethodEnter(suppress = Throwable.class) - public static SpanWithScope onEnter( - @Advice.This final Object servlet, - @Advice.Origin final Method method, - @Advice.Argument(0) final ServletRequest request, - @Advice.Argument(1) final ServletResponse response) { + public static HttpServerSpanWithScope onEnter( + @This final Object servlet, + @Origin final Method method, + @Argument(0) final ServletRequest request, + @Argument(1) final ServletResponse response) { if (!(request instanceof HttpServletRequest)) { return null; } @@ -52,7 +55,7 @@ public static SpanWithScope onEnter( public static void stopSpan( @Advice.Argument(0) final ServletRequest request, @Advice.Argument(1) final ServletResponse response, - @Advice.Enter final SpanWithScope spanWithScope, + @Advice.Enter final HttpServerSpanWithScope spanWithScope, @Advice.Thrown final Throwable throwable) { if (spanWithScope == null) { return; @@ -69,7 +72,7 @@ public static void stopSpan( //Usually Tracer takes care of this checks and of closing scopes. //But in case of async response processing we have to handle scope in this thread, //not in some arbitrary thread that may later take care of actual response. - Span span = spanWithScope.getSpan(); + HttpServerSpan span = spanWithScope.getSpan(); if(span == null){ spanWithScope.closeScope(); return; diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java index 525e061442a7..b34bb704ceeb 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java @@ -1,7 +1,7 @@ package io.opentelemetry.auto.instrumentation.servlet.v3_0; import io.opentelemetry.auto.instrumentation.servlet.ServletHttpServerTracer; -import io.opentelemetry.trace.Span; +import io.opentelemetry.auto.typed.server.http.HttpServerSpan; import io.opentelemetry.trace.Status; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -21,9 +21,9 @@ protected int status(HttpServletResponse response) { return response.getStatus(); } - public void onTimeout(Span span, long timeout) { + public void onTimeout(HttpServerSpan span, long timeout) { span.setStatus(Status.DEADLINE_EXCEEDED); - span.setAttribute("timeout", timeout); + span.setAttribute("timeout", timeout); //TODO not defined span.end(); } } diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java index 255be5b3d556..cdb11b47f427 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/TagSettingAsyncListener.java @@ -15,7 +15,7 @@ */ package io.opentelemetry.auto.instrumentation.servlet.v3_0; -import io.opentelemetry.trace.Span; +import io.opentelemetry.auto.typed.server.http.HttpServerSpan; import java.util.concurrent.atomic.AtomicBoolean; import javax.servlet.AsyncEvent; import javax.servlet.AsyncListener; @@ -23,10 +23,10 @@ public class TagSettingAsyncListener implements AsyncListener { private final AtomicBoolean responseHandled; - private final Span span; + private final HttpServerSpan span; private final Servlet3HttpServerTracer servletHttpServerTracer; - public TagSettingAsyncListener(final AtomicBoolean responseHandled, final Span span, + public TagSettingAsyncListener(final AtomicBoolean responseHandled, final HttpServerSpan span, Servlet3HttpServerTracer servletHttpServerTracer) { this.responseHandled = responseHandled; this.span = span; diff --git a/instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java index 240826f2ee83..a1d11a85a79d 100644 --- a/instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java @@ -1,10 +1,10 @@ package io.opentelemetry.auto.instrumentation.servlet; -import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerTracer; import io.opentelemetry.auto.instrumentation.api.MoreTags; +import io.opentelemetry.auto.typed.server.http.HttpServerSpan; +import io.opentelemetry.auto.typed.server.http.HttpServerTracer; import io.opentelemetry.context.propagation.HttpTextFormat; import io.opentelemetry.context.propagation.HttpTextFormat.Getter; -import io.opentelemetry.trace.Span; import java.lang.reflect.Method; import java.net.URI; import java.net.URISyntaxException; @@ -39,13 +39,13 @@ private String getSpanName(Method method) { } @Override - protected Span findExistingSpan(HttpServletRequest request) { + protected HttpServerSpan findExistingSpan(HttpServletRequest request) { Object span = request.getAttribute(SPAN_ATTRIBUTE); - return span instanceof Span ? (Span) span : null; + return span instanceof HttpServerSpan ? (HttpServerSpan) span : null; } @Override - protected void persistSpanToRequest(Span span, HttpServletRequest request) { + protected void persistSpanToRequest(HttpServerSpan span, HttpServletRequest request) { request.setAttribute(SPAN_ATTRIBUTE, span); } @@ -65,10 +65,11 @@ protected String method(HttpServletRequest request) { return request.getMethod(); } - public void onRequest(Span span, HttpServletRequest request) { + @Override + public void onRequest(HttpServerSpan span, HttpServletRequest request) { //TODO why? - request.setAttribute("traceId", span.getContext().getTraceId().toLowerBase16()); - request.setAttribute("spanId", span.getContext().getSpanId().toLowerBase16()); + request.setAttribute("traceId", span.getTraceId().toLowerBase16()); + request.setAttribute("spanId", span.getSpanId().toLowerBase16()); //TODO why? they are not in semantic convention, right? span.setAttribute("servlet.path", request.getServletPath()); @@ -100,7 +101,7 @@ protected Throwable unwrapThrowable(Throwable throwable) { } public void setPrincipal(HttpServletRequest request) { - final Span existingSpan = findExistingSpan(request); + final HttpServerSpan existingSpan = findExistingSpan(request); if (existingSpan != null) { final Principal principal = request.getUserPrincipal(); if (principal != null) {