From b01556832c90ce3a1622a3009c12f5d2690b924a Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Fri, 16 Apr 2021 17:36:03 -0700 Subject: [PATCH 01/20] Remove some testNotFound exclusions --- .../src/test/groovy/DropwizardTest.groovy | 22 +++++++++---- .../groovy/FinatraServerLatestTest.groovy | 14 ++++---- .../src/test/groovy/FinatraServerTest.groovy | 14 ++++---- .../src/test/groovy/test/GrailsTest.groovy | 7 ++-- .../main/groovy/JaxRsHttpServerTest.groovy | 2 +- .../test/groovy/server/PlayServerTest.groovy | 2 +- .../test/groovy/server/PlayServerTest.groovy | 2 +- .../server/RatpackHttpServerTest.groovy | 2 +- .../test/boot/SpringBootBasedTest.groovy | 23 +++++++------ .../test/filter/ServletFilterTest.groovy | 32 ++++++------------- .../test/groovy/Struts2ActionSpanTest.groovy | 22 +++++++------ .../test/base/HttpServerTest.groovy | 17 +++++----- 12 files changed, 83 insertions(+), 76 deletions(-) diff --git a/instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy b/instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy index f2f928062920..19ae65c11c10 100644 --- a/instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy +++ b/instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy @@ -7,6 +7,7 @@ import static io.opentelemetry.api.trace.SpanKind.INTERNAL import static io.opentelemetry.api.trace.SpanKind.SERVER import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT @@ -57,18 +58,25 @@ class DropwizardTest extends HttpServerTest implements Ag } @Override - boolean hasHandlerSpan() { - true + boolean hasHandlerSpan(ServerEndpoint endpoint) { + endpoint != NOT_FOUND } @Override - boolean testNotFound() { - false + boolean testPathParam() { + true } @Override - boolean testPathParam() { - true + String expectedServerSpanName(ServerEndpoint endpoint) { + switch (endpoint) { + case PATH_PARAM: + return "/path/{id}/param" + case NOT_FOUND: + return "/*" + default: + return endpoint.resolvePath(address).path + } } @Override @@ -88,7 +96,7 @@ class DropwizardTest extends HttpServerTest implements Ag @Override void serverSpan(TraceAssert trace, int index, String traceID = null, String parentID = null, String method = "GET", Long responseContentLength = null, ServerEndpoint endpoint = SUCCESS) { trace.span(index) { - name "${endpoint == PATH_PARAM ? "/path/{id}/param" : endpoint.resolvePath(address).path}" + name expectedServerSpanName(endpoint) kind SERVER errored endpoint.errored if (parentID != null) { diff --git a/instrumentation/finatra-2.9/javaagent/src/latestDepTest/groovy/FinatraServerLatestTest.groovy b/instrumentation/finatra-2.9/javaagent/src/latestDepTest/groovy/FinatraServerLatestTest.groovy index b225035c4973..7c0c53d7ec29 100644 --- a/instrumentation/finatra-2.9/javaagent/src/latestDepTest/groovy/FinatraServerLatestTest.groovy +++ b/instrumentation/finatra-2.9/javaagent/src/latestDepTest/groovy/FinatraServerLatestTest.groovy @@ -4,6 +4,7 @@ */ import static io.opentelemetry.api.trace.SpanKind.INTERNAL +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS import com.twitter.app.lifecycle.Event @@ -67,19 +68,18 @@ class FinatraServerLatestTest extends HttpServerTest implements Agen } @Override - boolean hasHandlerSpan() { - return true + void stopServer(HttpServer httpServer) { + Await.ready(httpServer.close(), TIMEOUT) } @Override - boolean testNotFound() { - // Resource name is set to "GET /notFound" - false + boolean hasHandlerSpan(ServerEndpoint endpoint) { + endpoint != NOT_FOUND } @Override - void stopServer(HttpServer httpServer) { - Await.ready(httpServer.close(), TIMEOUT) + String expectedServerSpanName(ServerEndpoint endpoint) { + return endpoint == NOT_FOUND ? "HTTP GET" : super.expectedServerSpanName(endpoint) } @Override diff --git a/instrumentation/finatra-2.9/javaagent/src/test/groovy/FinatraServerTest.groovy b/instrumentation/finatra-2.9/javaagent/src/test/groovy/FinatraServerTest.groovy index 04ef2c205a32..51eabcd5198a 100644 --- a/instrumentation/finatra-2.9/javaagent/src/test/groovy/FinatraServerTest.groovy +++ b/instrumentation/finatra-2.9/javaagent/src/test/groovy/FinatraServerTest.groovy @@ -4,6 +4,7 @@ */ import static io.opentelemetry.api.trace.SpanKind.INTERNAL +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS import com.twitter.finatra.http.HttpServer @@ -48,19 +49,18 @@ class FinatraServerTest extends HttpServerTest implements AgentTestT } @Override - boolean hasHandlerSpan() { - return true + void stopServer(HttpServer httpServer) { + Await.ready(httpServer.close(), TIMEOUT) } @Override - boolean testNotFound() { - // Resource name is set to "GET /notFound" - false + boolean hasHandlerSpan(ServerEndpoint endpoint) { + endpoint != NOT_FOUND } @Override - void stopServer(HttpServer httpServer) { - Await.ready(httpServer.close(), TIMEOUT) + String expectedServerSpanName(ServerEndpoint endpoint) { + return endpoint == NOT_FOUND ? "HTTP GET" : super.expectedServerSpanName(endpoint) } @Override diff --git a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy index 9f91e9d289b7..087d3abc4182 100644 --- a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy +++ b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy @@ -36,7 +36,8 @@ class GrailsTest extends HttpServerTest implemen try { ServerProperties.getDeclaredMethod("getServlet") contextPathKey = "server.servlet.contextPath" - } catch (NoSuchMethodException ignore) {} + } catch (NoSuchMethodException ignore) { + } Map properties = new HashMap<>() properties.put("server.port", port) properties.put(contextPathKey, contextPath) @@ -80,12 +81,12 @@ class GrailsTest extends HttpServerTest implemen } @Override - boolean hasHandlerSpan() { + boolean hasHandlerSpan(ServerEndpoint endpoint) { true } @Override - boolean hasExceptionOnServerSpan() { + boolean hasExceptionOnServerSpan(ServerEndpoint endpoint) { true } diff --git a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-testing/src/main/groovy/JaxRsHttpServerTest.groovy b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-testing/src/main/groovy/JaxRsHttpServerTest.groovy index b6bb32b0232e..9332cb2c027f 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-testing/src/main/groovy/JaxRsHttpServerTest.groovy +++ b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-testing/src/main/groovy/JaxRsHttpServerTest.groovy @@ -109,7 +109,7 @@ abstract class JaxRsHttpServerTest extends HttpServerTest implements Agent } @Override - boolean hasHandlerSpan() { + boolean hasHandlerSpan(ServerEndpoint endpoint) { true } diff --git a/instrumentation/play/play-2.4/javaagent/src/test/groovy/server/PlayServerTest.groovy b/instrumentation/play/play-2.4/javaagent/src/test/groovy/server/PlayServerTest.groovy index 62a405335e0a..f154a9632634 100644 --- a/instrumentation/play/play-2.4/javaagent/src/test/groovy/server/PlayServerTest.groovy +++ b/instrumentation/play/play-2.4/javaagent/src/test/groovy/server/PlayServerTest.groovy @@ -61,7 +61,7 @@ class PlayServerTest extends HttpServerTest implements AgentTestTrait { } @Override - boolean hasHandlerSpan() { + boolean hasHandlerSpan(ServerEndpoint endpoint) { true } diff --git a/instrumentation/play/play-2.6/javaagent/src/test/groovy/server/PlayServerTest.groovy b/instrumentation/play/play-2.6/javaagent/src/test/groovy/server/PlayServerTest.groovy index 03034cabf15c..22014ea4d7cb 100644 --- a/instrumentation/play/play-2.6/javaagent/src/test/groovy/server/PlayServerTest.groovy +++ b/instrumentation/play/play-2.6/javaagent/src/test/groovy/server/PlayServerTest.groovy @@ -63,7 +63,7 @@ class PlayServerTest extends HttpServerTest implements AgentTestTrait { } @Override - boolean hasHandlerSpan() { + boolean hasHandlerSpan(ServerEndpoint endpoint) { true } diff --git a/instrumentation/ratpack-1.4/javaagent/src/test/groovy/server/RatpackHttpServerTest.groovy b/instrumentation/ratpack-1.4/javaagent/src/test/groovy/server/RatpackHttpServerTest.groovy index 2e0ffd2d2b43..b247449fd854 100644 --- a/instrumentation/ratpack-1.4/javaagent/src/test/groovy/server/RatpackHttpServerTest.groovy +++ b/instrumentation/ratpack-1.4/javaagent/src/test/groovy/server/RatpackHttpServerTest.groovy @@ -98,7 +98,7 @@ class RatpackHttpServerTest extends HttpServerTest implements Agent } @Override - boolean hasHandlerSpan() { + boolean hasHandlerSpan(ServerEndpoint endpoint) { true } diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy index ad9f8433eccc..428b6c2f19d8 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy @@ -49,12 +49,12 @@ class SpringBootBasedTest extends HttpServerTest } @Override - boolean hasHandlerSpan() { + boolean hasHandlerSpan(ServerEndpoint endpoint) { true } @Override - boolean hasExceptionOnServerSpan() { + boolean hasExceptionOnServerSpan(ServerEndpoint endpoint) { true } @@ -73,24 +73,29 @@ class SpringBootBasedTest extends HttpServerTest true } + @Override boolean hasErrorPageSpans(ServerEndpoint endpoint) { endpoint == NOT_FOUND } + @Override int getErrorPageSpansCount(ServerEndpoint endpoint) { 2 } @Override String expectedServerSpanName(ServerEndpoint endpoint) { - if (endpoint == PATH_PARAM) { - return getContextPath() + "/path/{id}/param" - } else if (endpoint == AUTH_ERROR || endpoint == NOT_FOUND) { - return getContextPath() + "/error" - } else if (endpoint == LOGIN) { - return "HTTP POST" + switch (endpoint) { + case PATH_PARAM: + return getContextPath() + "/path/{id}/param" + case AUTH_ERROR: + case NOT_FOUND: + return getContextPath() + "/error" + case LOGIN: + return "HTTP POST" + default: + return super.expectedServerSpanName(endpoint) } - return super.expectedServerSpanName(endpoint) } def "test spans with auth error"() { diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy index 60a3bc610741..39241ecebe7b 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy @@ -8,7 +8,7 @@ package test.filter import static io.opentelemetry.api.trace.SpanKind.INTERNAL import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION -import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -36,13 +36,13 @@ class ServletFilterTest extends HttpServerTest i } @Override - boolean hasHandlerSpan() { - false + boolean hasHandlerSpan(ServerEndpoint endpoint) { + endpoint == NOT_FOUND } @Override boolean hasErrorPageSpans(ServerEndpoint endpoint) { - endpoint == ERROR || endpoint == EXCEPTION + endpoint == ERROR || endpoint == EXCEPTION || endpoint == NOT_FOUND } @Override @@ -52,7 +52,7 @@ class ServletFilterTest extends HttpServerTest i @Override boolean hasResponseSpan(ServerEndpoint endpoint) { - endpoint == REDIRECT || endpoint == ERROR + endpoint == REDIRECT || endpoint == ERROR || endpoint == NOT_FOUND } @Override @@ -62,6 +62,7 @@ class ServletFilterTest extends HttpServerTest i redirectSpan(trace, index, parent) break case ERROR: + case NOT_FOUND: sendErrorSpan(trace, index, parent) break } @@ -73,33 +74,20 @@ class ServletFilterTest extends HttpServerTest i } @Override - boolean testNotFound() { - // FIXME: the instrumentation adds an extra controller span which is not consistent. - // Fix tests or remove extra span. - false - } - - @Override - void handlerSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + void handlerSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint) { trace.span(index) { - name "TestController.${endpoint.name().toLowerCase()}" + name "ResourceHttpRequestHandler.handleRequest" kind INTERNAL - errored endpoint == EXCEPTION childOf((SpanData) parent) - if (endpoint == EXCEPTION) { - errorEvent(Exception, EXCEPTION.body) - } } } @Override String expectedServerSpanName(ServerEndpoint endpoint) { - if (endpoint == PATH_PARAM) { - return "/path/{id}/param" - } else if (endpoint == ERROR || endpoint == EXCEPTION) { + if (endpoint == ERROR || endpoint == EXCEPTION || endpoint == NOT_FOUND) { return "/error" } - return endpoint.resolvePath(address).path + return "HTTP GET" } @Override diff --git a/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy b/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy index 1ac2cb959627..f77330ee1bb8 100644 --- a/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy +++ b/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy @@ -5,6 +5,7 @@ import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static io.opentelemetry.instrumentation.test.utils.TraceUtils.basicServerSpan @@ -27,11 +28,6 @@ import org.eclipse.jetty.util.resource.FileResource class Struts2ActionSpanTest extends HttpServerTest implements AgentTestTrait { - @Override - boolean testNotFound() { - return false - } - @Override boolean testPathParam() { return true @@ -43,13 +39,13 @@ class Struts2ActionSpanTest extends HttpServerTest implements AgentTestT } @Override - boolean hasHandlerSpan() { - return true + boolean hasHandlerSpan(ServerEndpoint endpoint) { + return endpoint != NOT_FOUND } @Override boolean hasResponseSpan(ServerEndpoint endpoint) { - endpoint == REDIRECT || endpoint == ERROR || endpoint == EXCEPTION + endpoint == REDIRECT || endpoint == ERROR || endpoint == EXCEPTION || endpoint == NOT_FOUND } @Override @@ -60,13 +56,21 @@ class Struts2ActionSpanTest extends HttpServerTest implements AgentTestT break case ERROR: case EXCEPTION: + case NOT_FOUND: sendErrorSpan(trace, index, handlerSpan) break } } String expectedServerSpanName(ServerEndpoint endpoint) { - return endpoint == PATH_PARAM ? getContextPath() + "/path/{id}/param" : endpoint.resolvePath(address).path + switch (endpoint) { + case PATH_PARAM: + return getContextPath() + "/path/{id}/param" + case NOT_FOUND: + return "HTTP GET" + default: + return endpoint.resolvePath(address).path + } } @Override diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy index 7c39e38ca997..a80e2a2ae8e8 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy @@ -17,8 +17,8 @@ import static io.opentelemetry.instrumentation.test.utils.TraceUtils.basicSpan import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace import static org.junit.Assume.assumeTrue -import io.opentelemetry.api.common.AttributeKey import io.opentelemetry.api.GlobalOpenTelemetry +import io.opentelemetry.api.common.AttributeKey import io.opentelemetry.api.trace.Span import io.opentelemetry.api.trace.SpanKind import io.opentelemetry.context.Context @@ -46,12 +46,12 @@ abstract class HttpServerTest extends InstrumentationSpecification imple return "" } - boolean hasHandlerSpan() { + boolean hasHandlerSpan(ServerEndpoint endpoint) { false } - boolean hasExceptionOnServerSpan() { - !hasHandlerSpan() + boolean hasExceptionOnServerSpan(ServerEndpoint endpoint) { + !hasHandlerSpan(endpoint) } boolean hasRenderSpan(ServerEndpoint endpoint) { @@ -390,6 +390,7 @@ abstract class HttpServerTest extends InstrumentationSpecification imple This way we verify that child span created by the server actually corresponds to the client request. */ + def "high concurrency test"() { setup: assumeTrue(testConcurrency()) @@ -448,10 +449,10 @@ abstract class HttpServerTest extends InstrumentationSpecification imple void assertTheTraces(int size, String traceID = null, String parentID = null, String method = "GET", ServerEndpoint endpoint = SUCCESS, String errorMessage = null, Response response = null) { def spanCount = 1 // server span - if (hasHandlerSpan()) { + if (hasResponseSpan(endpoint)) { spanCount++ } - if (hasResponseSpan(endpoint)) { + if (hasHandlerSpan(endpoint)) { spanCount++ } if (endpoint != NOT_FOUND) { @@ -474,12 +475,12 @@ abstract class HttpServerTest extends InstrumentationSpecification imple trace(it, spanCount) { def spanIndex = 0 serverSpan(it, spanIndex++, traceID, parentID, method, response?.body()?.contentLength(), endpoint) - if (hasHandlerSpan()) { + if (hasHandlerSpan(endpoint)) { handlerSpan(it, spanIndex++, span(0), method, endpoint) } if (endpoint != NOT_FOUND) { def controllerSpanIndex = 0 - if (hasHandlerSpan()) { + if (hasHandlerSpan(endpoint)) { controllerSpanIndex++ } if (hasForwardSpan()) { From b65a7ab56ccc2d08bd383fbfdf3129b20c9ea0b2 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sat, 17 Apr 2021 11:58:13 -0700 Subject: [PATCH 02/20] Remove stuff that got mixed in here from another branch --- .../src/test/groovy/test/filter/ServletFilterTest.groovy | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy index 39241ecebe7b..5405f896be5f 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy @@ -9,6 +9,7 @@ import static io.opentelemetry.api.trace.SpanKind.INTERNAL import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -84,10 +85,12 @@ class ServletFilterTest extends HttpServerTest i @Override String expectedServerSpanName(ServerEndpoint endpoint) { - if (endpoint == ERROR || endpoint == EXCEPTION || endpoint == NOT_FOUND) { + if (endpoint == PATH_PARAM) { + return "/path/{id}/param" + } else if (endpoint == ERROR || endpoint == EXCEPTION || endpoint == NOT_FOUND) { return "/error" } - return "HTTP GET" + return endpoint.resolvePath(address).path } @Override From ad06d1c57bd684cb1699198bfa42d5fef20d5cbe Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 18 Apr 2021 18:00:26 -0700 Subject: [PATCH 03/20] Point of notFound is to be not found --- .../src/test/groovy/test/ErrorController.groovy | 5 ----- .../src/test/groovy/test/GrailsTest.groovy | 16 ++++------------ 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/ErrorController.groovy b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/ErrorController.groovy index 6ca586eb40f9..2f8137cd4c7b 100644 --- a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/ErrorController.groovy +++ b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/ErrorController.groovy @@ -16,9 +16,4 @@ class ErrorController implements Controller { def index() { render ERROR.body } - - @Action - def notFound() { - response.sendError(404, "Not Found") - } } diff --git a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy index 087d3abc4182..abfc228661af 100644 --- a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy +++ b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy @@ -102,7 +102,7 @@ class GrailsTest extends HttpServerTest implemen @Override int getErrorPageSpansCount(ServerEndpoint endpoint) { - endpoint == NOT_FOUND ? 3 : 2 + endpoint == NOT_FOUND ? 1 : 2 } @Override @@ -113,17 +113,9 @@ class GrailsTest extends HttpServerTest implemen @Override void errorPageSpans(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint) { forwardSpan(trace, index, trace.span(0)) - def errorSpanName = endpoint == NOT_FOUND ? "ErrorController.notFound" : "ErrorController.index" - trace.span(index + 1) { - name errorSpanName - kind INTERNAL - errored false - attributes { - } - } - if (endpoint == NOT_FOUND) { - trace.span(index + 2) { - name ~/\.sendError$/ + if (endpoint != NOT_FOUND) { + trace.span(index + 1) { + name "ErrorController.index" kind INTERNAL errored false attributes { From 0e396617a7962f39133f83f9e28404f9cac0a717 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 18 Apr 2021 20:05:52 -0700 Subject: [PATCH 04/20] Better server span name for Grails --- .../api/servlet/ServletSpanNaming.java | 90 ++++++++++++++----- .../instrumentation/grails/GrailsTracer.java | 24 +++-- ...ingsInfoHandlerAdapterInstrumentation.java | 8 +- .../src/test/groovy/test/GrailsTest.groovy | 4 +- .../jetty/v11_0/Jetty11HttpServerTracer.java | 2 +- .../jetty/v8_0/Jetty8HttpServerTracer.java | 2 +- .../liberty/LibertyHttpServerTracer.java | 2 +- .../v2_2/Servlet2HttpServerTracer.java | 23 ++--- .../v3_0/Servlet3HttpServerTracer.java | 32 +++---- .../v5_0/JakartaServletHttpServerTracer.java | 30 ++++--- .../servlet/ServletHttpServerTracer.java | 10 ++- .../HandlerAdapterInstrumentation.java | 2 +- .../HandlerMappingResourceNameFilter.java | 2 +- .../springwebmvc/SpringWebMvcTracer.java | 10 ++- .../struts2/Struts2Tracer.java | 7 +- ...RequestHandlerExecutorInstrumentation.java | 18 ++-- 16 files changed, 167 insertions(+), 99 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletSpanNaming.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletSpanNaming.java index f5a88477f199..b57da6fa787c 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletSpanNaming.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletSpanNaming.java @@ -11,7 +11,7 @@ /** * Helper container for tracking whether servlet integration should update server span name or not. */ -public class ServletSpanNaming { +public abstract class ServletSpanNaming { private static final ContextKey CONTEXT_KEY = ContextKey.named("opentelemetry-servlet-span-naming-key"); @@ -20,39 +20,81 @@ public static Context init(Context context) { if (context.get(CONTEXT_KEY) != null) { return context; } - return context.with(CONTEXT_KEY, new ServletSpanNaming()); + return context.with(CONTEXT_KEY, new DefaultServletSpanNaming()); } - private volatile boolean servletUpdatedServerSpanName = false; + public static ServletSpanNaming from(Context context) { + ServletSpanNaming servletSpanNaming = context.get(CONTEXT_KEY); + return servletSpanNaming == null ? NoopServletSpanNaming.INSTANCE : servletSpanNaming; + } private ServletSpanNaming() {} /** - * Returns true, if servlet integration should update server span name. After server span name has - * been updated with setServletUpdatedServerSpanName this method will return - * false. - * - * @param context server context - * @return true, if the server span name should be updated by servlet integration, or - * false otherwise. + * This should be called before servlet instrumentation updates the server span name. If it + * returns true, the servlet instrumentation should update the server span name and then should + * call {@link #setServletUpdatedServerSpanName}. */ - public static boolean shouldUpdateServerSpanName(Context context) { - ServletSpanNaming servletSpanNaming = context.get(CONTEXT_KEY); - if (servletSpanNaming != null) { - return !servletSpanNaming.servletUpdatedServerSpanName; - } - return false; - } + public abstract boolean shouldServletUpdateServerSpanName(); + + /** This should be called after servlet instrumentation updates the server span name. */ + public abstract void setServletUpdatedServerSpanName(); /** - * Indicate that the servlet integration has updated the name for the server span. - * - * @param context server context + * This should be called before controller instrumentation updates the server span name. If it + * returns true, the controller instrumentation should update the server span name and then should + * call {@link #setControllerUpdatedServerSpanName}. */ - public static void setServletUpdatedServerSpanName(Context context) { - ServletSpanNaming servletSpanNaming = context.get(CONTEXT_KEY); - if (servletSpanNaming != null) { - servletSpanNaming.servletUpdatedServerSpanName = true; + public abstract boolean shouldControllerUpdateServerSpanName(); + + /** This should be called after controller instrumentation updates the server span name. */ + public abstract void setControllerUpdatedServerSpanName(); + + private static class DefaultServletSpanNaming extends ServletSpanNaming { + + private volatile boolean servletUpdatedServerSpanName; + private volatile boolean controllerUpdatedServerSpanName; + + @Override + public boolean shouldServletUpdateServerSpanName() { + return !servletUpdatedServerSpanName; + } + + @Override + public void setServletUpdatedServerSpanName() { + servletUpdatedServerSpanName = true; } + + @Override + public boolean shouldControllerUpdateServerSpanName() { + return !controllerUpdatedServerSpanName; + } + + @Override + public void setControllerUpdatedServerSpanName() { + controllerUpdatedServerSpanName = true; + servletUpdatedServerSpanName = true; // just in case not set already + } + } + + private static class NoopServletSpanNaming extends ServletSpanNaming { + + private static final ServletSpanNaming INSTANCE = new NoopServletSpanNaming(); + + @Override + public boolean shouldServletUpdateServerSpanName() { + return true; + } + + @Override + public void setServletUpdatedServerSpanName() {} + + @Override + public boolean shouldControllerUpdateServerSpanName() { + return true; + } + + @Override + public void setControllerUpdatedServerSpanName() {} } } diff --git a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java index 093f511bc777..9b461072383b 100644 --- a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java +++ b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java @@ -8,7 +8,9 @@ import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; +import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; import io.opentelemetry.instrumentation.api.tracer.BaseTracer; +import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import org.grails.web.mapping.mvc.GrailsControllerUrlMappingInfo; public class GrailsTracer extends BaseTracer { @@ -23,14 +25,20 @@ public Context startSpan(Object controller, String action) { return startSpan(spanNameForClass(controller.getClass()) + "." + action); } - public void nameServerSpan( - Context context, Span serverSpan, GrailsControllerUrlMappingInfo info) { - String action = - info.getActionName() != null - ? info.getActionName() - : info.getControllerClass().getDefaultAction(); - serverSpan.updateName( - ServletContextPath.prepend(context, "/" + info.getControllerName() + "/" + action)); + public void updateServerSpanName(Context context, GrailsControllerUrlMappingInfo info) { + Span serverSpan = ServerSpan.fromContextOrNull(context); + if (serverSpan != null) { + ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); + if (servletSpanNaming.shouldControllerUpdateServerSpanName()) { + String action = + info.getActionName() != null + ? info.getActionName() + : info.getControllerClass().getDefaultAction(); + serverSpan.updateName( + ServletContextPath.prepend(context, "/" + info.getControllerName() + "/" + action)); + servletSpanNaming.setControllerUpdatedServerSpanName(); + } + } } @Override diff --git a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/UrlMappingsInfoHandlerAdapterInstrumentation.java b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/UrlMappingsInfoHandlerAdapterInstrumentation.java index 11f64072bb6a..41f717ee99a9 100644 --- a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/UrlMappingsInfoHandlerAdapterInstrumentation.java +++ b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/UrlMappingsInfoHandlerAdapterInstrumentation.java @@ -13,9 +13,7 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import static net.bytebuddy.matcher.ElementMatchers.takesArguments; -import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; import java.util.Map; @@ -48,11 +46,7 @@ public static void nameSpan(@Advice.Argument(2) Object handler) { if (handler instanceof GrailsControllerUrlMappingInfo) { Context parentContext = Java8BytecodeBridge.currentContext(); - Span serverSpan = ServerSpan.fromContextOrNull(parentContext); - if (serverSpan != null) { - tracer() - .nameServerSpan(parentContext, serverSpan, (GrailsControllerUrlMappingInfo) handler); - } + tracer().updateServerSpanName(parentContext, (GrailsControllerUrlMappingInfo) handler); } } } diff --git a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy index abfc228661af..38f70b964708 100644 --- a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy +++ b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy @@ -57,8 +57,8 @@ class GrailsTest extends HttpServerTest implemen return getContextPath() + "/test/path" } else if (endpoint == QUERY_PARAM) { return getContextPath() + "/test/query" - } else if (endpoint == ERROR || endpoint == EXCEPTION) { - return getContextPath() + "/error/index" + } else if (endpoint == ERROR) { + return getContextPath() + "/test/error" } else if (endpoint == NOT_FOUND) { return getContextPath() + "/**" } diff --git a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java index 70f24f596d33..91c887523799 100644 --- a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java +++ b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java @@ -18,7 +18,7 @@ public static Jetty11HttpServerTracer tracer() { } public Context startServerSpan(HttpServletRequest request) { - return startSpan(request, "HTTP " + request.getMethod()); + return startSpan(request, "HTTP " + request.getMethod(), false); } @Override diff --git a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java index b589501a44ca..de20c1b8e700 100644 --- a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java +++ b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java @@ -18,7 +18,7 @@ public static Jetty8HttpServerTracer tracer() { } public Context startServerSpan(HttpServletRequest request) { - return startSpan(request, "HTTP " + request.getMethod()); + return startSpan(request, "HTTP " + request.getMethod(), false); } @Override diff --git a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java index be8f4cffbccd..d46a95689b1d 100644 --- a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java +++ b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java @@ -17,7 +17,7 @@ public static LibertyHttpServerTracer tracer() { } public Context startSpan(HttpServletRequest request) { - return startSpan(request, "HTTP " + request.getMethod()); + return startSpan(request, "HTTP " + request.getMethod(), false); } @Override diff --git a/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java b/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java index 0900c3635c4d..cfb6e0ddc273 100644 --- a/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java +++ b/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java @@ -24,21 +24,24 @@ public static Servlet2HttpServerTracer tracer() { } public Context startSpan(HttpServletRequest request) { - Context context = startSpan(request, getSpanName(request)); - // server span name shouldn't be update when server span was created by servlet 2.2 - // instrumentation - ServletSpanNaming.setServletUpdatedServerSpanName(context); - return context; + return startSpan(request, getSpanName(request), true); } + @Override public Context updateContext(Context context, HttpServletRequest request) { + updateServerSpanName(context, request); + return super.updateContext(context, request); + } + + private void updateServerSpanName(Context context, HttpServletRequest request) { Span span = ServerSpan.fromContextOrNull(context); - if (span != null && ServletSpanNaming.shouldUpdateServerSpanName(context)) { - span.updateName(getSpanName(request)); - ServletSpanNaming.setServletUpdatedServerSpanName(context); + if (span != null) { + ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); + if (servletSpanNaming.shouldServletUpdateServerSpanName()) { + span.updateName(getSpanName(request)); + servletSpanNaming.setServletUpdatedServerSpanName(); + } } - - return super.updateContext(context, request); } @Override diff --git a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java index 20dd96b20cbb..1bf16c8a83fa 100644 --- a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java +++ b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java @@ -31,13 +31,8 @@ public static Servlet3HttpServerTracer tracer() { } public Context startSpan(Object servletOrFilter, HttpServletRequest request) { - Context context = startSpan(request, getSpanName(servletOrFilter, request)); - // server span name shouldn't be update when server span was created from a call to Servlet - // if server span was created from a call to Filter then name may be updated from updateContext. - if (servletOrFilter instanceof Servlet) { - ServletSpanNaming.setServletUpdatedServerSpanName(context); - } - return context; + return startSpan( + request, getSpanName(servletOrFilter, request), servletOrFilter instanceof Servlet); } private String getSpanName(Object servletOrFilter, HttpServletRequest request) { @@ -116,16 +111,23 @@ private static MappingResolver getMappingResolver( public Context updateContext( Context context, Object servletOrFilter, HttpServletRequest request) { - Span span = ServerSpan.fromContextOrNull(context); - if (span != null && ServletSpanNaming.shouldUpdateServerSpanName(context)) { - String spanName = getSpanNameFromPath(servletOrFilter, request); - if (spanName != null) { - span.updateName(spanName); - ServletSpanNaming.setServletUpdatedServerSpanName(context); + updateServerSpanName(context, servletOrFilter, request); + return updateContext(context, request); + } + + private static void updateServerSpanName( + Context context, Object servletOrFilter, HttpServletRequest request) { + Span serverSpan = ServerSpan.fromContextOrNull(context); + if (serverSpan != null) { + ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); + if (servletSpanNaming.shouldServletUpdateServerSpanName()) { + String spanName = getSpanNameFromPath(servletOrFilter, request); + if (spanName != null) { + serverSpan.updateName(spanName); + servletSpanNaming.setServletUpdatedServerSpanName(); + } } } - - return updateContext(context, request); } @Override diff --git a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java index 06b48ff6d7be..2c6116cd44b5 100644 --- a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java @@ -34,13 +34,8 @@ public static JakartaServletHttpServerTracer tracer() { } public Context startSpan(Object servletOrFilter, HttpServletRequest request) { - Context context = startSpan(request, getSpanName(servletOrFilter, request)); - // server span name shouldn't be update when server span was created from a call to Servlet - // if server span was created from a call to Filter then name may be updated from updateContext. - if (servletOrFilter instanceof Servlet) { - ServletSpanNaming.setServletUpdatedServerSpanName(context); - } - return context; + return startSpan( + request, getSpanName(servletOrFilter, request), servletOrFilter instanceof Servlet); } private String getSpanName(Object servletOrFilter, HttpServletRequest request) { @@ -119,16 +114,23 @@ private static MappingResolver getMappingResolver( public Context updateContext( Context context, Object servletOrFilter, HttpServletRequest request) { + updateServerSpanName(context, servletOrFilter, request); + return updateContext(context, request); + } + + private void updateServerSpanName( + Context context, Object servletOrFilter, HttpServletRequest request) { Span span = ServerSpan.fromContextOrNull(context); - if (span != null && ServletSpanNaming.shouldUpdateServerSpanName(context)) { - String spanName = getSpanNameFromPath(servletOrFilter, request); - if (spanName != null) { - span.updateName(spanName); - ServletSpanNaming.setServletUpdatedServerSpanName(context); + if (span != null) { + ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); + if (servletSpanNaming.shouldServletUpdateServerSpanName()) { + String spanName = getSpanNameFromPath(servletOrFilter, request); + if (spanName != null) { + span.updateName(spanName); + servletSpanNaming.setServletUpdatedServerSpanName(); + } } } - - return updateContext(context, request); } @Override diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index 74366ea03ebb..52656091698b 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -37,7 +37,7 @@ public ServletHttpServerTracer(ServletAccessor accessor) { this.accessor = accessor; } - public Context startSpan(REQUEST request, String spanName) { + public Context startSpan(REQUEST request, String spanName, boolean servlet) { Context context = startSpan(request, request, request, spanName); SpanContext spanContext = Span.fromContext(context).getSpanContext(); @@ -45,13 +45,17 @@ public Context startSpan(REQUEST request, String spanName) { accessor.setRequestAttribute(request, "trace_id", spanContext.getTraceId()); accessor.setRequestAttribute(request, "span_id", spanContext.getSpanId()); + if (servlet) { + // server span name shouldn't be updated when server span was created from a call to Servlet + // (if created from a call to Filter then name may be updated from updateContext) + ServletSpanNaming.from(context).setServletUpdatedServerSpanName(); + } return addServletContextPath(context, request); } @Override protected Context customizeContext(Context context, REQUEST request) { - // add context for tracking whether servlet instrumentation has updated - // server span + // add context for tracking whether servlet instrumentation has updated the server span name context = ServletSpanNaming.init(context); // add context for current request's context path return addServletContextPath(context, request); diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java index c314b490b466..5e053774b19e 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java @@ -63,7 +63,7 @@ public static void nameResourceAndStartSpan( Span serverSpan = ServerSpan.fromContextOrNull(parentContext); if (serverSpan != null) { // Name the parent span based on the matching pattern - tracer().onRequest(parentContext, serverSpan, request); + tracer().updateServerSpanName(parentContext, serverSpan, request); // Now create a span for handler/controller execution. context = tracer().startHandlerSpan(parentContext, handler); if (context != null) { diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java index a4c675544ec6..43a7f98d22be 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java @@ -49,7 +49,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha // Name the parent span based on the matching pattern // Let the parent span resource name be set with the attribute set in findMapping. - tracer().onRequest(context, serverSpan, (HttpServletRequest) request); + tracer().updateServerSpanName(context, serverSpan, (HttpServletRequest) request); } } catch (Exception ignored) { // mapping.getHandler() threw exception. Ignore diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java index bd1e76855f27..9e7b34bcfc86 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java @@ -12,6 +12,7 @@ import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.config.Config; import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; +import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; import io.opentelemetry.instrumentation.api.tracer.BaseTracer; import java.lang.reflect.Method; import javax.servlet.Servlet; @@ -52,12 +53,17 @@ public Context startSpan(ModelAndView mv) { return parentContext.with(span.startSpan()); } - public void onRequest(Context context, Span span, HttpServletRequest request) { + public void updateServerSpanName(Context context, Span serverSpan, HttpServletRequest request) { if (request != null) { Object bestMatchingPattern = request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); if (bestMatchingPattern != null) { - span.updateName(ServletContextPath.prepend(context, bestMatchingPattern.toString())); + ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); + if (servletSpanNaming.shouldControllerUpdateServerSpanName()) { + serverSpan.updateName( + ServletContextPath.prepend(context, bestMatchingPattern.toString())); + servletSpanNaming.setControllerUpdatedServerSpanName(); + } } } } diff --git a/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java b/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java index cdc55d4c6a9e..e8d209079f8e 100644 --- a/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java +++ b/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java @@ -50,6 +50,11 @@ public void updateServerSpanName(Context context, ActionProxy actionProxy) { return; } + ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); + if (!servletSpanNaming.shouldControllerUpdateServerSpanName()) { + return; + } + // We take name from the config, because it contains the path pattern from the // configuration. String result = actionProxy.getConfig().getName(); @@ -69,7 +74,7 @@ public void updateServerSpanName(Context context, ActionProxy actionProxy) { serverSpan.updateName(ServletContextPath.prepend(context, result)); // prevent servlet integration from doing further updates to server span name - ServletSpanNaming.setServletUpdatedServerSpanName(context); + servletSpanNaming.setControllerUpdatedServerSpanName(); } @Override diff --git a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java index 7eb7ee0bb955..5d69f7b314b6 100644 --- a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java +++ b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java @@ -49,14 +49,16 @@ public static void onExit(@Advice.Argument(0) IRequestHandler handler) { return; } if (handler instanceof IPageClassRequestHandler) { - // using class name as page name - String pageName = ((IPageClassRequestHandler) handler).getPageClass().getName(); - // wicket filter mapping without wildcard, if wicket filter is mapped to /* - // this will be an empty string - String filterPath = RequestCycle.get().getRequest().getFilterPath(); - serverSpan.updateName(ServletContextPath.prepend(context, filterPath + "/" + pageName)); - // prevent servlet integration from doing further updates to server span name - ServletSpanNaming.setServletUpdatedServerSpanName(context); + ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); + if (servletSpanNaming.shouldControllerUpdateServerSpanName()) { + // using class name as page name + String pageName = ((IPageClassRequestHandler) handler).getPageClass().getName(); + // wicket filter mapping without wildcard, if wicket filter is mapped to /* + // this will be an empty string + String filterPath = RequestCycle.get().getRequest().getFilterPath(); + serverSpan.updateName(ServletContextPath.prepend(context, filterPath + "/" + pageName)); + servletSpanNaming.setControllerUpdatedServerSpanName(); + } } } } From 9b65f7fcd63870de7f6e4723d99fb8f886bf64f2 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 18 Apr 2021 20:47:10 -0700 Subject: [PATCH 05/20] Better wicket server span name --- .../wicket-8.0/javaagent/src/test/groovy/WicketTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/wicket-8.0/javaagent/src/test/groovy/WicketTest.groovy b/instrumentation/wicket-8.0/javaagent/src/test/groovy/WicketTest.groovy index 67e44a661924..b497ceb55c4b 100644 --- a/instrumentation/wicket-8.0/javaagent/src/test/groovy/WicketTest.groovy +++ b/instrumentation/wicket-8.0/javaagent/src/test/groovy/WicketTest.groovy @@ -82,7 +82,7 @@ class WicketTest extends AgentInstrumentationSpecification implements HttpServer assertTraces(1) { trace(0, 1) { - basicServerSpan(it, 0, getContextPath() + "/wicket-test/org.apache.wicket.markup.html.pages.InternalErrorPage", null, new Exception("test exception")) + basicServerSpan(it, 0, getContextPath() + "/wicket-test/hello.ExceptionPage", null, new Exception("test exception")) } } } From 51c9de5c5ce22e62b078442b83230529a95bc404 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 18 Apr 2021 21:24:08 -0700 Subject: [PATCH 06/20] Fix tests --- .../test/groovy/test/boot/SpringBootBasedTest.groovy | 3 --- .../test/groovy/test/filter/ServletFilterTest.groovy | 10 +++++----- .../instrumentation/test/base/HttpServerTest.groovy | 9 ++++++++- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy index 428b6c2f19d8..bce5129f97c2 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy @@ -88,9 +88,6 @@ class SpringBootBasedTest extends HttpServerTest switch (endpoint) { case PATH_PARAM: return getContextPath() + "/path/{id}/param" - case AUTH_ERROR: - case NOT_FOUND: - return getContextPath() + "/error" case LOGIN: return "HTTP POST" default: diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy index 5405f896be5f..e62094076f95 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy @@ -85,12 +85,12 @@ class ServletFilterTest extends HttpServerTest i @Override String expectedServerSpanName(ServerEndpoint endpoint) { - if (endpoint == PATH_PARAM) { - return "/path/{id}/param" - } else if (endpoint == ERROR || endpoint == EXCEPTION || endpoint == NOT_FOUND) { - return "/error" + switch (endpoint) { + case PATH_PARAM: + return getContextPath() + "/path/{id}/param" + default: + return super.expectedServerSpanName(endpoint) } - return endpoint.resolvePath(address).path } @Override diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy index a80e2a2ae8e8..bc70442b2e3c 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy @@ -39,7 +39,14 @@ import spock.lang.Unroll abstract class HttpServerTest extends InstrumentationSpecification implements HttpServerTestTrait { String expectedServerSpanName(ServerEndpoint endpoint) { - return endpoint == PATH_PARAM ? getContextPath() + "/path/:id/param" : endpoint.resolvePath(address).path + switch (endpoint) { + case PATH_PARAM: + return getContextPath() + "/path/:id/param" + case NOT_FOUND: + return getContextPath() + "/**" + default: + return endpoint.resolvePath(address).path + } } String getContextPath() { From fc94fb86819104040fb83a190e52f7834716ca4e Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 18 Apr 2021 21:38:20 -0700 Subject: [PATCH 07/20] Skip armeria notFound test --- .../v1_3/AbstractArmeriaHttpServerTest.groovy | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/instrumentation/armeria-1.3/testing/src/main/groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.groovy b/instrumentation/armeria-1.3/testing/src/main/groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.groovy index 4bf8da5ea904..75ba7ccdfe2c 100644 --- a/instrumentation/armeria-1.3/testing/src/main/groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.groovy +++ b/instrumentation/armeria-1.3/testing/src/main/groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.groovy @@ -49,6 +49,12 @@ abstract class AbstractArmeriaHttpServerTest extends HttpServerTest { ] } + @Override + boolean testNotFound() { + // currently span name is /notFound which indicates it won't be low-cardinality + false + } + @Override Server startServer(int port) { ServerBuilder sb = Server.builder() @@ -61,31 +67,31 @@ abstract class AbstractArmeriaHttpServerTest extends HttpServerTest { } } - sb.service(REDIRECT.path) {ctx, req -> + sb.service(REDIRECT.path) { ctx, req -> controller(REDIRECT) { HttpResponse.of(ResponseHeaders.of(HttpStatus.valueOf(REDIRECT.status), HttpHeaderNames.LOCATION, REDIRECT.body)) } } - sb.service(ERROR.path) {ctx, req -> + sb.service(ERROR.path) { ctx, req -> controller(ERROR) { HttpResponse.of(HttpStatus.valueOf(ERROR.status), MediaType.PLAIN_TEXT_UTF_8, ERROR.body) } } - sb.service(EXCEPTION.path) {ctx, req -> + sb.service(EXCEPTION.path) { ctx, req -> controller(EXCEPTION) { throw new Exception(EXCEPTION.body) } } - sb.service("/query") {ctx, req -> + sb.service("/query") { ctx, req -> controller(QUERY_PARAM) { HttpResponse.of(HttpStatus.valueOf(QUERY_PARAM.status), MediaType.PLAIN_TEXT_UTF_8, "some=${QueryParams.fromQueryString(ctx.query()).get("some")}") } } - sb.service("/path/:id/param") {ctx, req -> + sb.service("/path/:id/param") { ctx, req -> controller(PATH_PARAM) { HttpResponse.of(HttpStatus.valueOf(PATH_PARAM.status), MediaType.PLAIN_TEXT_UTF_8, ctx.pathParam("id")) } From b2cb7c4e8bfcac8617b0e82f7f3a1f939d88b6ed Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 18 Apr 2021 22:25:55 -0700 Subject: [PATCH 08/20] Fix tomcat tests --- .../instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy | 6 ++++++ .../instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy index 9e6ef2c86e9e..9b9ffb88b35c 100644 --- a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy @@ -30,6 +30,12 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait return "/app" } + @Override + boolean testNotFound() { + // currently span name is /notFound which indicates it won't be low-cardinality + false + } + @Override Tomcat startServer(int port) { Tomcat tomcat = new Tomcat() diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy index ef38df012275..fe450efa1d30 100644 --- a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy @@ -30,6 +30,12 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait return "/app" } + @Override + boolean testNotFound() { + // currently span name is /notFound which indicates it won't be low-cardinality + false + } + @Override Tomcat startServer(int port) { Tomcat tomcat = new Tomcat() From 94fb3f769646a8c64574ec71790cc4247e6185e7 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Tue, 20 Apr 2021 15:19:32 -0700 Subject: [PATCH 09/20] Change default verification of not found span name --- .../src/test/groovy/GlassFishServerTest.groovy | 8 -------- .../javaagent/src/test/groovy/TomcatServlet3Test.groovy | 8 -------- .../src/test/groovy/test/boot/SpringBootBasedTest.groovy | 2 ++ .../src/test/groovy/test/filter/ServletFilterTest.groovy | 2 ++ .../instrumentation/test/base/HttpServerTest.groovy | 2 +- 5 files changed, 5 insertions(+), 17 deletions(-) diff --git a/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy b/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy index db5cd6278b62..5efe578d68d5 100644 --- a/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy +++ b/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy @@ -77,12 +77,4 @@ class GlassFishServerTest extends HttpServerTest implements AgentTest break } } - - @Override - String expectedServerSpanName(ServerEndpoint endpoint) { - if (endpoint == NOT_FOUND) { - return getContextPath() + "/*" - } - return super.expectedServerSpanName(endpoint) - } } diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3Test.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3Test.groovy index 6135f8b6a69c..aaad04eb3159 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3Test.groovy +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3Test.groovy @@ -54,14 +54,6 @@ abstract class TomcatServlet3Test extends AbstractServlet3Test super.responseSpan(trace, index, parent, method, endpoint) } - @Override - String expectedServerSpanName(ServerEndpoint endpoint) { - if (endpoint == NOT_FOUND) { - return getContextPath() + "/*" - } - return super.expectedServerSpanName(endpoint) - } - @Shared def accessLogValue = new TestAccessLogValve() diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy index 5f685706c388..44006e766da9 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy @@ -89,6 +89,8 @@ class SpringBootBasedTest extends HttpServerTest switch (endpoint) { case PATH_PARAM: return getContextPath() + "/path/{id}/param" + case NOT_FOUND: + return getContextPath() + "/**" case LOGIN: return "HTTP POST" default: diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy index 85679a73f2c6..95985fee9151 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy @@ -88,6 +88,8 @@ class ServletFilterTest extends HttpServerTest i switch (endpoint) { case PATH_PARAM: return getContextPath() + "/path/{id}/param" + case NOT_FOUND: + return getContextPath() + "/**" default: return super.expectedServerSpanName(endpoint) } diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy index b3a33d31cc24..4f1953096bbd 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy @@ -45,7 +45,7 @@ abstract class HttpServerTest extends InstrumentationSpecification imple case PATH_PARAM: return getContextPath() + "/path/:id/param" case NOT_FOUND: - return getContextPath() + "/**" + return getContextPath() + "/*" default: return endpoint.resolvePath(address).path } From 75190c03cd9065cd5cdc759699731e4e0e7a9fe1 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Tue, 20 Apr 2021 15:39:01 -0700 Subject: [PATCH 10/20] Better not found tests --- .../src/test/groovy/test/UrlMappings.groovy | 19 ++++++++-------- .../GrizzlyFilterchainServerTest.groovy | 3 --- .../tomcat/v10_0/TomcatHandlerTest.groovy | 22 +++++++++++++------ .../tomcat/v7_0/TomcatHandlerTest.groovy | 22 +++++++++++++------ 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/UrlMappings.groovy b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/UrlMappings.groovy index ef49ee310188..bb65ff069ceb 100644 --- a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/UrlMappings.groovy +++ b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/UrlMappings.groovy @@ -7,15 +7,14 @@ package test class UrlMappings { - static mappings = { - "/success"(controller: 'test', action: 'success') - "/query"(controller: 'test', action: 'query') - "/redirect"(controller: 'test', action: 'redirect') - "/error-status"(controller: 'test', action: 'error') - "/exception"(controller: 'test', action: 'exception') - "/path/$id/param"(controller: 'test', action: 'path') + static mappings = { + "/success"(controller: 'test', action: 'success') + "/query"(controller: 'test', action: 'query') + "/redirect"(controller: 'test', action: 'redirect') + "/error-status"(controller: 'test', action: 'error') + "/exception"(controller: 'test', action: 'exception') + "/path/$id/param"(controller: 'test', action: 'path') - "500"(controller: 'error') - "404"(controller: 'error', action: 'notFound') - } + "500"(controller: 'error') + } } diff --git a/instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyFilterchainServerTest.groovy b/instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyFilterchainServerTest.groovy index 315ef2acf3d0..6fe984c837b3 100644 --- a/instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyFilterchainServerTest.groovy +++ b/instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyFilterchainServerTest.groovy @@ -141,9 +141,6 @@ class GrizzlyFilterchainServerTest extends HttpServerTest implements break case "/exception": throw new Exception(EXCEPTION.body) - case "/notFound": - endpoint = NOT_FOUND - break case "/query?some=query": endpoint = QUERY_PARAM break diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy index 9b9ffb88b35c..b219226e532e 100644 --- a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy @@ -6,6 +6,7 @@ package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT import io.opentelemetry.instrumentation.test.AgentTestTrait @@ -31,9 +32,13 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait } @Override - boolean testNotFound() { - // currently span name is /notFound which indicates it won't be low-cardinality - false + String expectedServerSpanName(ServerEndpoint endpoint) { + switch (endpoint) { + case NOT_FOUND: + return "HTTP GET" + default: + return endpoint.resolvePath(address).path + } } @Override @@ -48,9 +53,11 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait Tomcat.addServlet(ctx, "testServlet", new TestServlet()) // Mapping servlet to /* will result in all requests have a name of just a context. - ServerEndpoint.values().each { - ctx.addServletMappingDecoded(it.path, "testServlet") - } + ServerEndpoint.values().toList().stream() + .filter { it != NOT_FOUND } + .forEach { + ctx.addServletMappingDecoded(it.path, "testServlet") + } (tomcat.host as StandardHost).errorReportValveClass = ErrorHandlerValve.name @@ -66,7 +73,7 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait @Override boolean hasResponseSpan(ServerEndpoint endpoint) { - endpoint == REDIRECT || endpoint == ERROR + endpoint == REDIRECT || endpoint == ERROR || endpoint == NOT_FOUND } @Override @@ -76,6 +83,7 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait redirectSpan(trace, index, parent) break case ERROR: + case NOT_FOUND: sendErrorSpan(trace, index, parent) break } diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy index fe450efa1d30..d7af9bfc8a15 100644 --- a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy @@ -6,6 +6,7 @@ package io.opentelemetry.javaagent.instrumentation.tomcat.v7_0 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT import io.opentelemetry.instrumentation.test.AgentTestTrait @@ -31,9 +32,13 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait } @Override - boolean testNotFound() { - // currently span name is /notFound which indicates it won't be low-cardinality - false + String expectedServerSpanName(ServerEndpoint endpoint) { + switch (endpoint) { + case NOT_FOUND: + return "HTTP GET" + default: + return endpoint.resolvePath(address).path + } } @Override @@ -48,9 +53,11 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait Tomcat.addServlet(ctx, "testServlet", new TestServlet()) // Mapping servlet to /* will result in all requests have a name of just a context. - ServerEndpoint.values().each { - ctx.addServletMappingDecoded(it.path, "testServlet") - } + ServerEndpoint.values().toList().stream() + .filter { it != NOT_FOUND } + .forEach { + ctx.addServletMappingDecoded(it.path, "testServlet") + } (tomcat.host as StandardHost).errorReportValveClass = ErrorHandlerValve.name @@ -66,7 +73,7 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait @Override boolean hasResponseSpan(ServerEndpoint endpoint) { - endpoint == REDIRECT || endpoint == ERROR + endpoint == REDIRECT || endpoint == ERROR || endpoint == NOT_FOUND } @Override @@ -76,6 +83,7 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait redirectSpan(trace, index, parent) break case ERROR: + case NOT_FOUND: sendErrorSpan(trace, index, parent) break } From a973ed5c4343c41c0418aecd915d67d1fd1773ba Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Tue, 20 Apr 2021 15:44:48 -0700 Subject: [PATCH 11/20] Rename ServletSpanNaming to ServerSpanNaming --- ...tSpanNaming.java => ServerSpanNaming.java} | 20 +++++++++---------- .../instrumentation/grails/GrailsTracer.java | 8 ++++---- .../v2_2/Servlet2HttpServerTracer.java | 8 ++++---- .../v3_0/Servlet3HttpServerTracer.java | 8 ++++---- .../v5_0/JakartaServletHttpServerTracer.java | 8 ++++---- .../servlet/ServletHttpServerTracer.java | 6 +++--- .../springwebmvc/SpringWebMvcTracer.java | 8 ++++---- .../struts2/Struts2Tracer.java | 8 ++++---- .../tomcat/common/TomcatTracer.java | 4 ++-- .../undertow/UndertowHttpServerTracer.java | 4 ++-- ...RequestHandlerExecutorInstrumentation.java | 8 ++++---- 11 files changed, 45 insertions(+), 45 deletions(-) rename instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/{ServletSpanNaming.java => ServerSpanNaming.java} (79%) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletSpanNaming.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java similarity index 79% rename from instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletSpanNaming.java rename to instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java index b57da6fa787c..1d6aaf87b47d 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletSpanNaming.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java @@ -11,24 +11,24 @@ /** * Helper container for tracking whether servlet integration should update server span name or not. */ -public abstract class ServletSpanNaming { +public abstract class ServerSpanNaming { - private static final ContextKey CONTEXT_KEY = + private static final ContextKey CONTEXT_KEY = ContextKey.named("opentelemetry-servlet-span-naming-key"); public static Context init(Context context) { if (context.get(CONTEXT_KEY) != null) { return context; } - return context.with(CONTEXT_KEY, new DefaultServletSpanNaming()); + return context.with(CONTEXT_KEY, new DefaultServerSpanNaming()); } - public static ServletSpanNaming from(Context context) { - ServletSpanNaming servletSpanNaming = context.get(CONTEXT_KEY); - return servletSpanNaming == null ? NoopServletSpanNaming.INSTANCE : servletSpanNaming; + public static ServerSpanNaming from(Context context) { + ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY); + return serverSpanNaming == null ? NoopServerSpanNaming.INSTANCE : serverSpanNaming; } - private ServletSpanNaming() {} + private ServerSpanNaming() {} /** * This should be called before servlet instrumentation updates the server span name. If it @@ -50,7 +50,7 @@ private ServletSpanNaming() {} /** This should be called after controller instrumentation updates the server span name. */ public abstract void setControllerUpdatedServerSpanName(); - private static class DefaultServletSpanNaming extends ServletSpanNaming { + private static class DefaultServerSpanNaming extends ServerSpanNaming { private volatile boolean servletUpdatedServerSpanName; private volatile boolean controllerUpdatedServerSpanName; @@ -77,9 +77,9 @@ public void setControllerUpdatedServerSpanName() { } } - private static class NoopServletSpanNaming extends ServletSpanNaming { + private static class NoopServerSpanNaming extends ServerSpanNaming { - private static final ServletSpanNaming INSTANCE = new NoopServletSpanNaming(); + private static final ServerSpanNaming INSTANCE = new NoopServerSpanNaming(); @Override public boolean shouldServletUpdateServerSpanName() { diff --git a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java index 9b461072383b..2c2f1fdf751c 100644 --- a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java +++ b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java @@ -7,8 +7,8 @@ import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; -import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; import io.opentelemetry.instrumentation.api.tracer.BaseTracer; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import org.grails.web.mapping.mvc.GrailsControllerUrlMappingInfo; @@ -28,15 +28,15 @@ public Context startSpan(Object controller, String action) { public void updateServerSpanName(Context context, GrailsControllerUrlMappingInfo info) { Span serverSpan = ServerSpan.fromContextOrNull(context); if (serverSpan != null) { - ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); - if (servletSpanNaming.shouldControllerUpdateServerSpanName()) { + ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); + if (serverSpanNaming.shouldControllerUpdateServerSpanName()) { String action = info.getActionName() != null ? info.getActionName() : info.getControllerClass().getDefaultAction(); serverSpan.updateName( ServletContextPath.prepend(context, "/" + info.getControllerName() + "/" + action)); - servletSpanNaming.setControllerUpdatedServerSpanName(); + serverSpanNaming.setControllerUpdatedServerSpanName(); } } } diff --git a/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java b/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java index cfb6e0ddc273..c5a6139ceac1 100644 --- a/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java +++ b/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java @@ -7,7 +7,7 @@ import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; +import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.instrumentation.servlet.javax.JavaxServletHttpServerTracer; import javax.servlet.http.HttpServletRequest; @@ -36,10 +36,10 @@ public Context updateContext(Context context, HttpServletRequest request) { private void updateServerSpanName(Context context, HttpServletRequest request) { Span span = ServerSpan.fromContextOrNull(context); if (span != null) { - ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); - if (servletSpanNaming.shouldServletUpdateServerSpanName()) { + ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); + if (serverSpanNaming.shouldServletUpdateServerSpanName()) { span.updateName(getSpanName(request)); - servletSpanNaming.setServletUpdatedServerSpanName(); + serverSpanNaming.setServletUpdatedServerSpanName(); } } } diff --git a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java index 1bf16c8a83fa..43ea46819350 100644 --- a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java +++ b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java @@ -7,7 +7,7 @@ import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; +import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.instrumentation.servlet.MappingResolver; import io.opentelemetry.instrumentation.servlet.javax.JavaxServletHttpServerTracer; @@ -119,12 +119,12 @@ private static void updateServerSpanName( Context context, Object servletOrFilter, HttpServletRequest request) { Span serverSpan = ServerSpan.fromContextOrNull(context); if (serverSpan != null) { - ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); - if (servletSpanNaming.shouldServletUpdateServerSpanName()) { + ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); + if (serverSpanNaming.shouldServletUpdateServerSpanName()) { String spanName = getSpanNameFromPath(servletOrFilter, request); if (spanName != null) { serverSpan.updateName(spanName); - servletSpanNaming.setServletUpdatedServerSpanName(); + serverSpanNaming.setServletUpdatedServerSpanName(); } } } diff --git a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java index 2c6116cd44b5..d044712dc963 100644 --- a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java @@ -8,7 +8,7 @@ import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.TextMapGetter; -import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; +import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.instrumentation.servlet.MappingResolver; import io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer; @@ -122,12 +122,12 @@ private void updateServerSpanName( Context context, Object servletOrFilter, HttpServletRequest request) { Span span = ServerSpan.fromContextOrNull(context); if (span != null) { - ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); - if (servletSpanNaming.shouldServletUpdateServerSpanName()) { + ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); + if (serverSpanNaming.shouldServletUpdateServerSpanName()) { String spanName = getSpanNameFromPath(servletOrFilter, request); if (spanName != null) { span.updateName(spanName); - servletSpanNaming.setServletUpdatedServerSpanName(); + serverSpanNaming.setServletUpdatedServerSpanName(); } } } diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index 52656091698b..cd0ff14646b9 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -12,8 +12,8 @@ import io.opentelemetry.context.propagation.TextMapGetter; import io.opentelemetry.instrumentation.api.config.Config; import io.opentelemetry.instrumentation.api.servlet.AppServerBridge; +import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; -import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; import io.opentelemetry.instrumentation.api.tracer.HttpServerTracer; import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; import java.net.URI; @@ -48,7 +48,7 @@ public Context startSpan(REQUEST request, String spanName, boolean servlet) { if (servlet) { // server span name shouldn't be updated when server span was created from a call to Servlet // (if created from a call to Filter then name may be updated from updateContext) - ServletSpanNaming.from(context).setServletUpdatedServerSpanName(); + ServerSpanNaming.from(context).setServletUpdatedServerSpanName(); } return addServletContextPath(context, request); } @@ -56,7 +56,7 @@ public Context startSpan(REQUEST request, String spanName, boolean servlet) { @Override protected Context customizeContext(Context context, REQUEST request) { // add context for tracking whether servlet instrumentation has updated the server span name - context = ServletSpanNaming.init(context); + context = ServerSpanNaming.init(context); // add context for current request's context path return addServletContextPath(context, request); } diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java index 9e7b34bcfc86..b59a7c738414 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java @@ -11,8 +11,8 @@ import io.opentelemetry.api.trace.SpanBuilder; import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.config.Config; +import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; -import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; import io.opentelemetry.instrumentation.api.tracer.BaseTracer; import java.lang.reflect.Method; import javax.servlet.Servlet; @@ -58,11 +58,11 @@ public void updateServerSpanName(Context context, Span serverSpan, HttpServletRe Object bestMatchingPattern = request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); if (bestMatchingPattern != null) { - ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); - if (servletSpanNaming.shouldControllerUpdateServerSpanName()) { + ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); + if (serverSpanNaming.shouldControllerUpdateServerSpanName()) { serverSpan.updateName( ServletContextPath.prepend(context, bestMatchingPattern.toString())); - servletSpanNaming.setControllerUpdatedServerSpanName(); + serverSpanNaming.setControllerUpdatedServerSpanName(); } } } diff --git a/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java b/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java index e8d209079f8e..2bbad77f26c8 100644 --- a/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java +++ b/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java @@ -12,8 +12,8 @@ import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.SpanBuilder; import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; -import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; import io.opentelemetry.instrumentation.api.tracer.BaseTracer; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; @@ -50,8 +50,8 @@ public void updateServerSpanName(Context context, ActionProxy actionProxy) { return; } - ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); - if (!servletSpanNaming.shouldControllerUpdateServerSpanName()) { + ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); + if (!serverSpanNaming.shouldControllerUpdateServerSpanName()) { return; } @@ -74,7 +74,7 @@ public void updateServerSpanName(Context context, ActionProxy actionProxy) { serverSpan.updateName(ServletContextPath.prepend(context, result)); // prevent servlet integration from doing further updates to server span name - servletSpanNaming.setControllerUpdatedServerSpanName(); + serverSpanNaming.setControllerUpdatedServerSpanName(); } @Override diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java index cf5270dca37f..fac4e025596a 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java @@ -8,7 +8,7 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.TextMapGetter; import io.opentelemetry.instrumentation.api.servlet.AppServerBridge; -import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; +import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.tracer.HttpServerTracer; import java.net.URI; import java.util.Collections; @@ -35,7 +35,7 @@ public Context startServerSpan(Request request) { @Override protected Context customizeContext(Context context, Request request) { - context = ServletSpanNaming.init(context); + context = ServerSpanNaming.init(context); return AppServerBridge.init(context); } diff --git a/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowHttpServerTracer.java b/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowHttpServerTracer.java index a021b9aace4c..df7310fda5ac 100644 --- a/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowHttpServerTracer.java +++ b/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowHttpServerTracer.java @@ -8,7 +8,7 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.TextMapGetter; import io.opentelemetry.instrumentation.api.servlet.AppServerBridge; -import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; +import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.tracer.HttpServerTracer; import io.opentelemetry.javaagent.instrumentation.api.undertow.KeyHolder; import io.opentelemetry.javaagent.instrumentation.api.undertow.UndertowActiveHandlers; @@ -39,7 +39,7 @@ public Context startServerSpan(HttpServerExchange exchange) { @Override protected Context customizeContext(Context context, HttpServerExchange exchange) { - context = ServletSpanNaming.init(context); + context = ServerSpanNaming.init(context); // span is ended when counter reaches 0, we start from 2 which accounts for the // handler that started the span and exchange completion listener context = UndertowActiveHandlers.init(context, 2); diff --git a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java index 5d69f7b314b6..443a274cee71 100644 --- a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java +++ b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java @@ -10,8 +10,8 @@ import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; -import io.opentelemetry.instrumentation.api.servlet.ServletSpanNaming; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; @@ -49,15 +49,15 @@ public static void onExit(@Advice.Argument(0) IRequestHandler handler) { return; } if (handler instanceof IPageClassRequestHandler) { - ServletSpanNaming servletSpanNaming = ServletSpanNaming.from(context); - if (servletSpanNaming.shouldControllerUpdateServerSpanName()) { + ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); + if (serverSpanNaming.shouldControllerUpdateServerSpanName()) { // using class name as page name String pageName = ((IPageClassRequestHandler) handler).getPageClass().getName(); // wicket filter mapping without wildcard, if wicket filter is mapped to /* // this will be an empty string String filterPath = RequestCycle.get().getRequest().getFilterPath(); serverSpan.updateName(ServletContextPath.prepend(context, filterPath + "/" + pageName)); - servletSpanNaming.setControllerUpdatedServerSpanName(); + serverSpanNaming.setControllerUpdatedServerSpanName(); } } } From eef120e15c7bb9a1080cc41e1d80ad648093ff79 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Tue, 20 Apr 2021 16:21:18 -0700 Subject: [PATCH 12/20] Fix test --- .../src/test/groovy/test/GrailsTest.groovy | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy index d8c0c729725e..27b9c17ef827 100644 --- a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy +++ b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy @@ -103,7 +103,7 @@ class GrailsTest extends HttpServerTest implemen @Override int getErrorPageSpansCount(ServerEndpoint endpoint) { - endpoint == NOT_FOUND ? 1 : 2 + 2 } @Override @@ -114,12 +114,11 @@ class GrailsTest extends HttpServerTest implemen @Override void errorPageSpans(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint) { forwardSpan(trace, index, trace.span(0)) - if (endpoint != NOT_FOUND) { - trace.span(index + 1) { - name "ErrorController.index" - kind INTERNAL - attributes { - } + def errorSpanName = endpoint == NOT_FOUND ? "BasicErrorController.error" : "ErrorController.index" + trace.span(index + 1) { + name errorSpanName + kind INTERNAL + attributes { } } } From 023eac49e726e23b7b008d38f31995315ed81529 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Tue, 20 Apr 2021 23:34:07 -0700 Subject: [PATCH 13/20] ServerSpanName --- .../api/servlet/ServerSpanNaming.java | 96 ++++++------------- .../instrumentation/grails/GrailsTracer.java | 27 +++--- .../v2_2/Servlet2HttpServerTracer.java | 17 +--- .../v3_0/Servlet3HttpServerTracer.java | 22 +---- .../v5_0/JakartaServletHttpServerTracer.java | 22 +---- .../servlet/ServletHttpServerTracer.java | 7 +- .../HandlerAdapterInstrumentation.java | 3 +- .../HandlerMappingResourceNameFilter.java | 7 +- .../springwebmvc/SpringWebMvcTracer.java | 14 ++- .../struts2/Struts2Tracer.java | 56 +++++------ .../tomcat/common/TomcatTracer.java | 4 +- .../undertow/UndertowHttpServerTracer.java | 4 +- ...RequestHandlerExecutorInstrumentation.java | 22 +++-- 13 files changed, 110 insertions(+), 191 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java index 1d6aaf87b47d..f75f37b41a2d 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java @@ -5,96 +5,60 @@ package io.opentelemetry.instrumentation.api.servlet; +import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; import io.opentelemetry.context.ContextKey; +import io.opentelemetry.instrumentation.api.tracer.ServerSpan; +import java.util.function.Supplier; /** * Helper container for tracking whether servlet integration should update server span name or not. */ -public abstract class ServerSpanNaming { +public final class ServerSpanNaming { private static final ContextKey CONTEXT_KEY = ContextKey.named("opentelemetry-servlet-span-naming-key"); - public static Context init(Context context) { + public static Context init(Context context, Source initialSource) { if (context.get(CONTEXT_KEY) != null) { return context; } - return context.with(CONTEXT_KEY, new DefaultServerSpanNaming()); + return context.with(CONTEXT_KEY, new ServerSpanNaming(null)); } - public static ServerSpanNaming from(Context context) { - ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY); - return serverSpanNaming == null ? NoopServerSpanNaming.INSTANCE : serverSpanNaming; - } - - private ServerSpanNaming() {} - - /** - * This should be called before servlet instrumentation updates the server span name. If it - * returns true, the servlet instrumentation should update the server span name and then should - * call {@link #setServletUpdatedServerSpanName}. - */ - public abstract boolean shouldServletUpdateServerSpanName(); - - /** This should be called after servlet instrumentation updates the server span name. */ - public abstract void setServletUpdatedServerSpanName(); - - /** - * This should be called before controller instrumentation updates the server span name. If it - * returns true, the controller instrumentation should update the server span name and then should - * call {@link #setControllerUpdatedServerSpanName}. - */ - public abstract boolean shouldControllerUpdateServerSpanName(); - - /** This should be called after controller instrumentation updates the server span name. */ - public abstract void setControllerUpdatedServerSpanName(); - - private static class DefaultServerSpanNaming extends ServerSpanNaming { - - private volatile boolean servletUpdatedServerSpanName; - private volatile boolean controllerUpdatedServerSpanName; + private volatile Source updatedBySource; - @Override - public boolean shouldServletUpdateServerSpanName() { - return !servletUpdatedServerSpanName; - } + private ServerSpanNaming(Source initialSource) { + this.updatedBySource = initialSource; + } - @Override - public void setServletUpdatedServerSpanName() { - servletUpdatedServerSpanName = true; + public static void updateServerSpanName( + Context context, Source source, Supplier serverSpanName) { + Span serverSpan = ServerSpan.fromContextOrNull(context); + if (serverSpan == null) { + return; } - - @Override - public boolean shouldControllerUpdateServerSpanName() { - return !controllerUpdatedServerSpanName; + ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY); + if (serverSpanNaming == null) { + return; } - - @Override - public void setControllerUpdatedServerSpanName() { - controllerUpdatedServerSpanName = true; - servletUpdatedServerSpanName = true; // just in case not set already + Source updatedBySource = serverSpanNaming.updatedBySource; + if (updatedBySource != null && updatedBySource.level >= source.level) { + return; } + serverSpan.updateName(serverSpanName.get()); + serverSpanNaming.updatedBySource = source; } - private static class NoopServerSpanNaming extends ServerSpanNaming { + public enum Source { + CONTAINER(1), + SERVLET(2), + CONTROLLER(3); - private static final ServerSpanNaming INSTANCE = new NoopServerSpanNaming(); + private final int level; - @Override - public boolean shouldServletUpdateServerSpanName() { - return true; + Source(int level) { + this.level = level; } - - @Override - public void setServletUpdatedServerSpanName() {} - - @Override - public boolean shouldControllerUpdateServerSpanName() { - return true; - } - - @Override - public void setControllerUpdatedServerSpanName() {} } } diff --git a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java index 2c2f1fdf751c..073d1fa20335 100644 --- a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java +++ b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java @@ -5,12 +5,12 @@ package io.opentelemetry.javaagent.instrumentation.grails; -import io.opentelemetry.api.trace.Span; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTROLLER; + import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; import io.opentelemetry.instrumentation.api.tracer.BaseTracer; -import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import org.grails.web.mapping.mvc.GrailsControllerUrlMappingInfo; public class GrailsTracer extends BaseTracer { @@ -26,19 +26,16 @@ public Context startSpan(Object controller, String action) { } public void updateServerSpanName(Context context, GrailsControllerUrlMappingInfo info) { - Span serverSpan = ServerSpan.fromContextOrNull(context); - if (serverSpan != null) { - ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); - if (serverSpanNaming.shouldControllerUpdateServerSpanName()) { - String action = - info.getActionName() != null - ? info.getActionName() - : info.getControllerClass().getDefaultAction(); - serverSpan.updateName( - ServletContextPath.prepend(context, "/" + info.getControllerName() + "/" + action)); - serverSpanNaming.setControllerUpdatedServerSpanName(); - } - } + ServerSpanNaming.updateServerSpanName( + context, + CONTROLLER, + () -> { + String action = + info.getActionName() != null + ? info.getActionName() + : info.getControllerClass().getDefaultAction(); + return ServletContextPath.prepend(context, "/" + info.getControllerName() + "/" + action); + }); } @Override diff --git a/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java b/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java index c5a6139ceac1..dc7de3bc6415 100644 --- a/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java +++ b/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java @@ -5,10 +5,10 @@ package io.opentelemetry.instrumentation.servlet.v2_2; -import io.opentelemetry.api.trace.Span; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.SERVLET; + import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; -import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.instrumentation.servlet.javax.JavaxServletHttpServerTracer; import javax.servlet.http.HttpServletRequest; @@ -29,21 +29,10 @@ public Context startSpan(HttpServletRequest request) { @Override public Context updateContext(Context context, HttpServletRequest request) { - updateServerSpanName(context, request); + ServerSpanNaming.updateServerSpanName(context, SERVLET, () -> getSpanName(request)); return super.updateContext(context, request); } - private void updateServerSpanName(Context context, HttpServletRequest request) { - Span span = ServerSpan.fromContextOrNull(context); - if (span != null) { - ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); - if (serverSpanNaming.shouldServletUpdateServerSpanName()) { - span.updateName(getSpanName(request)); - serverSpanNaming.setServletUpdatedServerSpanName(); - } - } - } - @Override protected String getInstrumentationName() { return "io.opentelemetry.javaagent.servlet-2.2"; diff --git a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java index 43ea46819350..08a44a535c10 100644 --- a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java +++ b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java @@ -5,10 +5,10 @@ package io.opentelemetry.instrumentation.servlet.v3_0; -import io.opentelemetry.api.trace.Span; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.SERVLET; + import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; -import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.instrumentation.servlet.MappingResolver; import io.opentelemetry.instrumentation.servlet.javax.JavaxServletHttpServerTracer; import java.util.Collection; @@ -111,25 +111,11 @@ private static MappingResolver getMappingResolver( public Context updateContext( Context context, Object servletOrFilter, HttpServletRequest request) { - updateServerSpanName(context, servletOrFilter, request); + ServerSpanNaming.updateServerSpanName( + context, SERVLET, () -> getSpanNameFromPath(servletOrFilter, request)); return updateContext(context, request); } - private static void updateServerSpanName( - Context context, Object servletOrFilter, HttpServletRequest request) { - Span serverSpan = ServerSpan.fromContextOrNull(context); - if (serverSpan != null) { - ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); - if (serverSpanNaming.shouldServletUpdateServerSpanName()) { - String spanName = getSpanNameFromPath(servletOrFilter, request); - if (spanName != null) { - serverSpan.updateName(spanName); - serverSpanNaming.setServletUpdatedServerSpanName(); - } - } - } - } - @Override protected String getInstrumentationName() { return "io.opentelemetry.javaagent.servlet-3.0"; diff --git a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java index d044712dc963..d4bca7c71ffe 100644 --- a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java @@ -5,11 +5,11 @@ package io.opentelemetry.instrumentation.servlet.jakarta.v5_0; -import io.opentelemetry.api.trace.Span; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.SERVLET; + import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.TextMapGetter; import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; -import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.instrumentation.servlet.MappingResolver; import io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer; import jakarta.servlet.RequestDispatcher; @@ -114,25 +114,11 @@ private static MappingResolver getMappingResolver( public Context updateContext( Context context, Object servletOrFilter, HttpServletRequest request) { - updateServerSpanName(context, servletOrFilter, request); + ServerSpanNaming.updateServerSpanName( + context, SERVLET, () -> getSpanNameFromPath(servletOrFilter, request)); return updateContext(context, request); } - private void updateServerSpanName( - Context context, Object servletOrFilter, HttpServletRequest request) { - Span span = ServerSpan.fromContextOrNull(context); - if (span != null) { - ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); - if (serverSpanNaming.shouldServletUpdateServerSpanName()) { - String spanName = getSpanNameFromPath(servletOrFilter, request); - if (spanName != null) { - span.updateName(spanName); - serverSpanNaming.setServletUpdatedServerSpanName(); - } - } - } - } - @Override protected String getInstrumentationName() { return "io.opentelemetry.javaagent.servlet-5.0"; diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index cd0ff14646b9..40d69f8b594f 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -5,6 +5,9 @@ package io.opentelemetry.instrumentation.servlet; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.SERVLET; + import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.SpanContext; import io.opentelemetry.api.trace.StatusCode; @@ -48,7 +51,7 @@ public Context startSpan(REQUEST request, String spanName, boolean servlet) { if (servlet) { // server span name shouldn't be updated when server span was created from a call to Servlet // (if created from a call to Filter then name may be updated from updateContext) - ServerSpanNaming.from(context).setServletUpdatedServerSpanName(); + ServerSpanNaming.setUpdatedServerSpanName(SERVLET); } return addServletContextPath(context, request); } @@ -56,7 +59,7 @@ public Context startSpan(REQUEST request, String spanName, boolean servlet) { @Override protected Context customizeContext(Context context, REQUEST request) { // add context for tracking whether servlet instrumentation has updated the server span name - context = ServerSpanNaming.init(context); + context = ServerSpanNaming.init(context, CONTAINER); // add context for current request's context path return addServletContextPath(context, request); } diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java index 5e053774b19e..197c73b998d7 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java @@ -61,9 +61,10 @@ public static void nameResourceAndStartSpan( @Advice.Local("otelScope") Scope scope) { Context parentContext = Java8BytecodeBridge.currentContext(); Span serverSpan = ServerSpan.fromContextOrNull(parentContext); + // TODO (trask) is it important to check serverSpan != null here? if (serverSpan != null) { // Name the parent span based on the matching pattern - tracer().updateServerSpanName(parentContext, serverSpan, request); + tracer().updateServerSpanName(parentContext, request); // Now create a span for handler/controller execution. context = tracer().startHandlerSpan(parentContext, handler); if (context != null) { diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java index 43a7f98d22be..4376f52dd1b6 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java @@ -7,9 +7,7 @@ import static io.opentelemetry.javaagent.instrumentation.springwebmvc.SpringWebMvcTracer.tracer; -import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import java.io.IOException; import java.util.List; import javax.servlet.Filter; @@ -41,15 +39,14 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha } Context context = Context.current(); - Span serverSpan = ServerSpan.fromContextOrNull(context); - if (handlerMappings != null && serverSpan != null) { + if (handlerMappings != null) { try { if (findMapping((HttpServletRequest) request)) { // Name the parent span based on the matching pattern // Let the parent span resource name be set with the attribute set in findMapping. - tracer().updateServerSpanName(context, serverSpan, (HttpServletRequest) request); + tracer().updateServerSpanName(context, (HttpServletRequest) request); } } catch (Exception ignored) { // mapping.getHandler() threw exception. Ignore diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java index b59a7c738414..8b4c4c7f932a 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java @@ -6,8 +6,8 @@ package io.opentelemetry.javaagent.instrumentation.springwebmvc; import static io.opentelemetry.api.trace.SpanKind.INTERNAL; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTROLLER; -import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.SpanBuilder; import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.config.Config; @@ -53,17 +53,15 @@ public Context startSpan(ModelAndView mv) { return parentContext.with(span.startSpan()); } - public void updateServerSpanName(Context context, Span serverSpan, HttpServletRequest request) { + public void updateServerSpanName(Context context, HttpServletRequest request) { if (request != null) { Object bestMatchingPattern = request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); if (bestMatchingPattern != null) { - ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); - if (serverSpanNaming.shouldControllerUpdateServerSpanName()) { - serverSpan.updateName( - ServletContextPath.prepend(context, bestMatchingPattern.toString())); - serverSpanNaming.setControllerUpdatedServerSpanName(); - } + ServerSpanNaming.updateServerSpanName( + context, + CONTROLLER, + () -> ServletContextPath.prepend(context, bestMatchingPattern.toString())); } } } diff --git a/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java b/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java index 2bbad77f26c8..a57e779b0dae 100644 --- a/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java +++ b/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java @@ -6,16 +6,15 @@ package io.opentelemetry.javaagent.instrumentation.struts2; import static io.opentelemetry.api.trace.SpanKind.INTERNAL; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTROLLER; import com.opensymphony.xwork2.ActionInvocation; import com.opensymphony.xwork2.ActionProxy; -import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.SpanBuilder; import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; import io.opentelemetry.instrumentation.api.tracer.BaseTracer; -import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; public class Struts2Tracer extends BaseTracer { @@ -45,36 +44,29 @@ public Context startSpan(Context parentContext, ActionInvocation actionInvocatio // Handle cases where action parameters are encoded into URL path public void updateServerSpanName(Context context, ActionProxy actionProxy) { - Span serverSpan = ServerSpan.fromContextOrNull(context); - if (serverSpan == null) { - return; - } - - ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); - if (!serverSpanNaming.shouldControllerUpdateServerSpanName()) { - return; - } - - // We take name from the config, because it contains the path pattern from the - // configuration. - String result = actionProxy.getConfig().getName(); - - String actionNamespace = actionProxy.getNamespace(); - if (actionNamespace != null && !actionNamespace.isEmpty()) { - if (actionNamespace.endsWith("/") || result.startsWith("/")) { - result = actionNamespace + result; - } else { - result = actionNamespace + "/" + result; - } - } - - if (!result.startsWith("/")) { - result = "/" + result; - } - - serverSpan.updateName(ServletContextPath.prepend(context, result)); - // prevent servlet integration from doing further updates to server span name - serverSpanNaming.setControllerUpdatedServerSpanName(); + ServerSpanNaming.updateServerSpanName( + context, + CONTROLLER, + () -> { + // We take name from the config, because it contains the path pattern from the + // configuration. + String result = actionProxy.getConfig().getName(); + + String actionNamespace = actionProxy.getNamespace(); + if (actionNamespace != null && !actionNamespace.isEmpty()) { + if (actionNamespace.endsWith("/") || result.startsWith("/")) { + result = actionNamespace + result; + } else { + result = actionNamespace + "/" + result; + } + } + + if (!result.startsWith("/")) { + result = "/" + result; + } + + return ServletContextPath.prepend(context, result); + }); } @Override diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java index fac4e025596a..9c1b3e0f268b 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java @@ -5,6 +5,8 @@ package io.opentelemetry.javaagent.instrumentation.tomcat.common; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER; + import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.TextMapGetter; import io.opentelemetry.instrumentation.api.servlet.AppServerBridge; @@ -35,7 +37,7 @@ public Context startServerSpan(Request request) { @Override protected Context customizeContext(Context context, Request request) { - context = ServerSpanNaming.init(context); + context = ServerSpanNaming.init(context, CONTAINER); return AppServerBridge.init(context); } diff --git a/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowHttpServerTracer.java b/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowHttpServerTracer.java index df7310fda5ac..2378b5738ede 100644 --- a/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowHttpServerTracer.java +++ b/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowHttpServerTracer.java @@ -5,6 +5,8 @@ package io.opentelemetry.javaagent.instrumentation.undertow; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER; + import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.TextMapGetter; import io.opentelemetry.instrumentation.api.servlet.AppServerBridge; @@ -39,7 +41,7 @@ public Context startServerSpan(HttpServerExchange exchange) { @Override protected Context customizeContext(Context context, HttpServerExchange exchange) { - context = ServerSpanNaming.init(context); + context = ServerSpanNaming.init(context, CONTAINER); // span is ended when counter reaches 0, we start from 2 which accounts for the // handler that started the span and exchange completion listener context = UndertowActiveHandlers.init(context, 2); diff --git a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java index 443a274cee71..8d8fa8cf141a 100644 --- a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java +++ b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java @@ -5,6 +5,7 @@ package io.opentelemetry.javaagent.instrumentation.wicket; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTROLLER; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; @@ -49,16 +50,17 @@ public static void onExit(@Advice.Argument(0) IRequestHandler handler) { return; } if (handler instanceof IPageClassRequestHandler) { - ServerSpanNaming serverSpanNaming = ServerSpanNaming.from(context); - if (serverSpanNaming.shouldControllerUpdateServerSpanName()) { - // using class name as page name - String pageName = ((IPageClassRequestHandler) handler).getPageClass().getName(); - // wicket filter mapping without wildcard, if wicket filter is mapped to /* - // this will be an empty string - String filterPath = RequestCycle.get().getRequest().getFilterPath(); - serverSpan.updateName(ServletContextPath.prepend(context, filterPath + "/" + pageName)); - serverSpanNaming.setControllerUpdatedServerSpanName(); - } + ServerSpanNaming.updateServerSpanName( + context, + CONTROLLER, + () -> { + // using class name as page name + String pageName = ((IPageClassRequestHandler) handler).getPageClass().getName(); + // wicket filter mapping without wildcard, if wicket filter is mapped to /* + // this will be an empty string + String filterPath = RequestCycle.get().getRequest().getFilterPath(); + return ServletContextPath.prepend(context, filterPath + "/" + pageName); + }); } } } From a85bb621bf13bddbcb9863a2bd9043a445120080 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Tue, 20 Apr 2021 23:34:22 -0700 Subject: [PATCH 14/20] customizeContent --- .../instrumentation/api/tracer/HttpServerTracer.java | 6 ------ .../jetty/v11_0/Jetty11HttpServerTracer.java | 7 +------ .../jetty/v8_0/Jetty8HttpServerTracer.java | 7 +------ .../servlet/ServletHttpServerTracer.java | 12 ++++-------- .../instrumentation/tomcat/common/TomcatTracer.java | 6 +----- .../undertow/UndertowHttpServerTracer.java | 8 ++------ 6 files changed, 9 insertions(+), 37 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java index 0ee1f41a96de..36ab42b17d54 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java @@ -81,17 +81,11 @@ public Context startSpan( onConnectionAndRequest(spanBuilder, connection, request); Context context = withServerSpan(parentContext, spanBuilder.startSpan()); - context = customizeContext(context, request); attachServerContext(context, storage); return context; } - /** Override in subclass to customize context that is returned by {@code startSpan}. */ - protected Context customizeContext(Context context, REQUEST request) { - return context; - } - /** * Convenience method. Delegates to {@link #end(Context, Object, long)}, passing {@code timestamp} * value of {@code -1}. diff --git a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java index 91c887523799..9a5333a4ee59 100644 --- a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java +++ b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java @@ -18,12 +18,7 @@ public static Jetty11HttpServerTracer tracer() { } public Context startServerSpan(HttpServletRequest request) { - return startSpan(request, "HTTP " + request.getMethod(), false); - } - - @Override - protected Context customizeContext(Context context, HttpServletRequest request) { - context = super.customizeContext(context, request); + Context context = startSpan(request, "HTTP " + request.getMethod(), false); return AppServerBridge.init(context, false); } diff --git a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java index de20c1b8e700..29b9c6150af7 100644 --- a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java +++ b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java @@ -18,12 +18,7 @@ public static Jetty8HttpServerTracer tracer() { } public Context startServerSpan(HttpServletRequest request) { - return startSpan(request, "HTTP " + request.getMethod(), false); - } - - @Override - protected Context customizeContext(Context context, HttpServletRequest request) { - context = super.customizeContext(context, request); + Context context = startSpan(request, "HTTP " + request.getMethod(), false); return AppServerBridge.init(context, false); } diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index 40d69f8b594f..0d3b316489fd 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -48,18 +48,14 @@ public Context startSpan(REQUEST request, String spanName, boolean servlet) { accessor.setRequestAttribute(request, "trace_id", spanContext.getTraceId()); accessor.setRequestAttribute(request, "span_id", spanContext.getSpanId()); + // add context for tracking whether servlet instrumentation has updated the server span name if (servlet) { // server span name shouldn't be updated when server span was created from a call to Servlet // (if created from a call to Filter then name may be updated from updateContext) - ServerSpanNaming.setUpdatedServerSpanName(SERVLET); + context = ServerSpanNaming.init(context, SERVLET); + } else { + context = ServerSpanNaming.init(context, CONTAINER); } - return addServletContextPath(context, request); - } - - @Override - protected Context customizeContext(Context context, REQUEST request) { - // add context for tracking whether servlet instrumentation has updated the server span name - context = ServerSpanNaming.init(context, CONTAINER); // add context for current request's context path return addServletContextPath(context, request); } diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java index 9c1b3e0f268b..c33899b5ced3 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java @@ -32,11 +32,7 @@ public abstract class TomcatTracer extends HttpServerTracer Date: Tue, 20 Apr 2021 23:47:30 -0700 Subject: [PATCH 15/20] doc --- .../api/servlet/ServerSpanNaming.java | 31 ++++++++++++------- .../servlet/ServletHttpServerTracer.java | 2 +- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java index f75f37b41a2d..1b99853e5fe2 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java @@ -11,9 +11,7 @@ import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import java.util.function.Supplier; -/** - * Helper container for tracking whether servlet integration should update server span name or not. - */ +/** Helper container for tracking whether instrumentation should update server span name or not. */ public final class ServerSpanNaming { private static final ContextKey CONTEXT_KEY = @@ -23,7 +21,7 @@ public static Context init(Context context, Source initialSource) { if (context.get(CONTEXT_KEY) != null) { return context; } - return context.with(CONTEXT_KEY, new ServerSpanNaming(null)); + return context.with(CONTEXT_KEY, new ServerSpanNaming(initialSource)); } private volatile Source updatedBySource; @@ -32,6 +30,16 @@ private ServerSpanNaming(Source initialSource) { this.updatedBySource = initialSource; } + /** + * If there is a server span in the context, and {@link #init(Context, Source)} has been called to + * populate a {@code ServerSpanName} into the context, then this method will update the server + * span name using the provided {@link Supplier} if and only if the last {@link Source} to update + * the span name using this method has strictly lower priority than the provided {@link Source}. + * + *

If there is a server span in the context, and {@link #init(Context, Source)} has NOT been + * called to populate a {@code ServerSpanName} into the context, then this method will update the + * server span name using the provided {@link Supplier}. + */ public static void updateServerSpanName( Context context, Source source, Supplier serverSpanName) { Span serverSpan = ServerSpan.fromContextOrNull(context); @@ -40,14 +48,13 @@ public static void updateServerSpanName( } ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY); if (serverSpanNaming == null) { + serverSpan.updateName(serverSpanName.get()); return; } - Source updatedBySource = serverSpanNaming.updatedBySource; - if (updatedBySource != null && updatedBySource.level >= source.level) { - return; + if (source.order > serverSpanNaming.updatedBySource.order) { + serverSpan.updateName(serverSpanName.get()); + serverSpanNaming.updatedBySource = source; } - serverSpan.updateName(serverSpanName.get()); - serverSpanNaming.updatedBySource = source; } public enum Source { @@ -55,10 +62,10 @@ public enum Source { SERVLET(2), CONTROLLER(3); - private final int level; + private final int order; - Source(int level) { - this.level = level; + Source(int order) { + this.order = order; } } } diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index 0d3b316489fd..7d3fcd7b772a 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -51,9 +51,9 @@ public Context startSpan(REQUEST request, String spanName, boolean servlet) { // add context for tracking whether servlet instrumentation has updated the server span name if (servlet) { // server span name shouldn't be updated when server span was created from a call to Servlet - // (if created from a call to Filter then name may be updated from updateContext) context = ServerSpanNaming.init(context, SERVLET); } else { + // if created from a call to Filter then name may be updated from updateContext context = ServerSpanNaming.init(context, CONTAINER); } // add context for current request's context path From 478b524056444b6fe8301b942ded5035660a2ff6 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 21 Apr 2021 09:27:07 -0700 Subject: [PATCH 16/20] Extract methods --- .../instrumentation/grails/GrailsTracer.java | 18 ++++---- .../struts2/Struts2Tracer.java | 44 +++++++++---------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java index 073d1fa20335..eb7d72660b37 100644 --- a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java +++ b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java @@ -27,15 +27,15 @@ public Context startSpan(Object controller, String action) { public void updateServerSpanName(Context context, GrailsControllerUrlMappingInfo info) { ServerSpanNaming.updateServerSpanName( - context, - CONTROLLER, - () -> { - String action = - info.getActionName() != null - ? info.getActionName() - : info.getControllerClass().getDefaultAction(); - return ServletContextPath.prepend(context, "/" + info.getControllerName() + "/" + action); - }); + context, CONTROLLER, () -> getServerSpanName(context, info)); + } + + private static String getServerSpanName(Context context, GrailsControllerUrlMappingInfo info) { + String action = + info.getActionName() != null + ? info.getActionName() + : info.getControllerClass().getDefaultAction(); + return ServletContextPath.prepend(context, "/" + info.getControllerName() + "/" + action); } @Override diff --git a/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java b/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java index a57e779b0dae..cdb8db766ae1 100644 --- a/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java +++ b/instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java @@ -45,28 +45,28 @@ public Context startSpan(Context parentContext, ActionInvocation actionInvocatio // Handle cases where action parameters are encoded into URL path public void updateServerSpanName(Context context, ActionProxy actionProxy) { ServerSpanNaming.updateServerSpanName( - context, - CONTROLLER, - () -> { - // We take name from the config, because it contains the path pattern from the - // configuration. - String result = actionProxy.getConfig().getName(); - - String actionNamespace = actionProxy.getNamespace(); - if (actionNamespace != null && !actionNamespace.isEmpty()) { - if (actionNamespace.endsWith("/") || result.startsWith("/")) { - result = actionNamespace + result; - } else { - result = actionNamespace + "/" + result; - } - } - - if (!result.startsWith("/")) { - result = "/" + result; - } - - return ServletContextPath.prepend(context, result); - }); + context, CONTROLLER, () -> getServerSpanName(context, actionProxy)); + } + + private static String getServerSpanName(Context context, ActionProxy actionProxy) { + // We take name from the config, because it contains the path pattern from the + // configuration. + String result = actionProxy.getConfig().getName(); + + String actionNamespace = actionProxy.getNamespace(); + if (actionNamespace != null && !actionNamespace.isEmpty()) { + if (actionNamespace.endsWith("/") || result.startsWith("/")) { + result = actionNamespace + result; + } else { + result = actionNamespace + "/" + result; + } + } + + if (!result.startsWith("/")) { + result = "/" + result; + } + + return ServletContextPath.prepend(context, result); } @Override From 684b4bf2e37f843a6ca88e0861efe4c165c2dc4c Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 21 Apr 2021 09:38:41 -0700 Subject: [PATCH 17/20] Supplier can return null --- .../api/servlet/ServerSpanNaming.java | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java index 1b99853e5fe2..c6cae485342b 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java @@ -34,11 +34,12 @@ private ServerSpanNaming(Source initialSource) { * If there is a server span in the context, and {@link #init(Context, Source)} has been called to * populate a {@code ServerSpanName} into the context, then this method will update the server * span name using the provided {@link Supplier} if and only if the last {@link Source} to update - * the span name using this method has strictly lower priority than the provided {@link Source}. + * the span name using this method has strictly lower priority than the provided {@link Source}, + * and the value returned from the {@link Supplier} is non-null. * *

If there is a server span in the context, and {@link #init(Context, Source)} has NOT been * called to populate a {@code ServerSpanName} into the context, then this method will update the - * server span name using the provided {@link Supplier}. + * server span name using the provided {@link Supplier} if the value returned from it is non-null. */ public static void updateServerSpanName( Context context, Source source, Supplier serverSpanName) { @@ -48,12 +49,18 @@ public static void updateServerSpanName( } ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY); if (serverSpanNaming == null) { - serverSpan.updateName(serverSpanName.get()); + String name = serverSpanName.get(); + if (name != null) { + serverSpan.updateName(name); + } return; } if (source.order > serverSpanNaming.updatedBySource.order) { - serverSpan.updateName(serverSpanName.get()); - serverSpanNaming.updatedBySource = source; + String name = serverSpanName.get(); + if (name != null) { + serverSpan.updateName(name); + serverSpanNaming.updatedBySource = source; + } } } From f342beb998e1bd1952e287db714cefda61437871 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 21 Apr 2021 10:26:06 -0700 Subject: [PATCH 18/20] Cannot use lambdas inside advice methods --- ...RequestHandlerExecutorInstrumentation.java | 11 +------ .../wicket/ServerSpanNameSupplier.java | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+), 10 deletions(-) create mode 100644 instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/ServerSpanNameSupplier.java diff --git a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java index 8d8fa8cf141a..2bcb86ed5e15 100644 --- a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java +++ b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/RequestHandlerExecutorInstrumentation.java @@ -12,7 +12,6 @@ import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; -import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; @@ -24,7 +23,6 @@ import net.bytebuddy.matcher.ElementMatcher; import org.apache.wicket.core.request.handler.IPageClassRequestHandler; import org.apache.wicket.request.IRequestHandler; -import org.apache.wicket.request.cycle.RequestCycle; public class RequestHandlerExecutorInstrumentation implements TypeInstrumentation { @@ -53,14 +51,7 @@ public static void onExit(@Advice.Argument(0) IRequestHandler handler) { ServerSpanNaming.updateServerSpanName( context, CONTROLLER, - () -> { - // using class name as page name - String pageName = ((IPageClassRequestHandler) handler).getPageClass().getName(); - // wicket filter mapping without wildcard, if wicket filter is mapped to /* - // this will be an empty string - String filterPath = RequestCycle.get().getRequest().getFilterPath(); - return ServletContextPath.prepend(context, filterPath + "/" + pageName); - }); + new ServerSpanNameSupplier(context, (IPageClassRequestHandler) handler)); } } } diff --git a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/ServerSpanNameSupplier.java b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/ServerSpanNameSupplier.java new file mode 100644 index 000000000000..d8e740be5e10 --- /dev/null +++ b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/ServerSpanNameSupplier.java @@ -0,0 +1,33 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.wicket; + +import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; +import java.util.function.Supplier; +import org.apache.wicket.core.request.handler.IPageClassRequestHandler; +import org.apache.wicket.request.cycle.RequestCycle; + +public class ServerSpanNameSupplier implements Supplier { + + private final Context context; + private final IPageClassRequestHandler handler; + + public ServerSpanNameSupplier(Context context, IPageClassRequestHandler handler) { + this.context = context; + this.handler = handler; + } + + @Override + public String get() { + // using class name as page name + String pageName = ((IPageClassRequestHandler) handler).getPageClass().getName(); + // wicket filter mapping without wildcard, if wicket filter is mapped to /* + // this will be an empty string + String filterPath = RequestCycle.get().getRequest().getFilterPath(); + return ServletContextPath.prepend(context, filterPath + "/" + pageName); + } +} From 3818776d96a31626479257b4302517571e3ec28d Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 21 Apr 2021 13:06:30 -0700 Subject: [PATCH 19/20] Minor update --- .../instrumentation/api/servlet/ServerSpanNaming.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java index c6cae485342b..b92ab7b9135d 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java @@ -18,7 +18,10 @@ public final class ServerSpanNaming { ContextKey.named("opentelemetry-servlet-span-naming-key"); public static Context init(Context context, Source initialSource) { - if (context.get(CONTEXT_KEY) != null) { + ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY); + if (serverSpanNaming != null) { + // TODO (trask) does this ever happen? + serverSpanNaming.updatedBySource = initialSource; return context; } return context.with(CONTEXT_KEY, new ServerSpanNaming(initialSource)); From 8eaac6e4e8777168647d8b221f7aec725ecfdc9d Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 21 Apr 2021 13:13:35 -0700 Subject: [PATCH 20/20] Revert "customizeContent" This reverts commit a85bb621bf13bddbcb9863a2bd9043a445120080. --- .../api/servlet/ServerSpanNaming.java | 13 +++++++++++++ .../api/tracer/HttpServerTracer.java | 6 ++++++ .../jetty/v11_0/Jetty11HttpServerTracer.java | 7 ++++++- .../jetty/v8_0/Jetty8HttpServerTracer.java | 7 ++++++- .../servlet/ServletHttpServerTracer.java | 14 +++++++++----- .../tomcat/common/TomcatTracer.java | 6 +++++- .../undertow/UndertowHttpServerTracer.java | 8 ++++++-- 7 files changed, 51 insertions(+), 10 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java index b92ab7b9135d..cba442923d52 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java @@ -67,6 +67,19 @@ public static void updateServerSpanName( } } + // TODO (trask) migrate the one usage (ServletHttpServerTracer) to ServerSpanNaming.init() once we + // migrate to new Instrumenters (see + // https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/2814#discussion_r617351334 + // for the challenge with doing this now in the current Tracer structure, at least without some + // bigger changes, which we want to avoid in the Tracers as they are already deprecated) + @Deprecated + public static void updateSource(Context context, Source source) { + ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY); + if (serverSpanNaming != null && source.order > serverSpanNaming.updatedBySource.order) { + serverSpanNaming.updatedBySource = source; + } + } + public enum Source { CONTAINER(1), SERVLET(2), diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java index 36ab42b17d54..0ee1f41a96de 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java @@ -81,11 +81,17 @@ public Context startSpan( onConnectionAndRequest(spanBuilder, connection, request); Context context = withServerSpan(parentContext, spanBuilder.startSpan()); + context = customizeContext(context, request); attachServerContext(context, storage); return context; } + /** Override in subclass to customize context that is returned by {@code startSpan}. */ + protected Context customizeContext(Context context, REQUEST request) { + return context; + } + /** * Convenience method. Delegates to {@link #end(Context, Object, long)}, passing {@code timestamp} * value of {@code -1}. diff --git a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java index 9a5333a4ee59..91c887523799 100644 --- a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java +++ b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java @@ -18,7 +18,12 @@ public static Jetty11HttpServerTracer tracer() { } public Context startServerSpan(HttpServletRequest request) { - Context context = startSpan(request, "HTTP " + request.getMethod(), false); + return startSpan(request, "HTTP " + request.getMethod(), false); + } + + @Override + protected Context customizeContext(Context context, HttpServletRequest request) { + context = super.customizeContext(context, request); return AppServerBridge.init(context, false); } diff --git a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java index 29b9c6150af7..de20c1b8e700 100644 --- a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java +++ b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java @@ -18,7 +18,12 @@ public static Jetty8HttpServerTracer tracer() { } public Context startServerSpan(HttpServletRequest request) { - Context context = startSpan(request, "HTTP " + request.getMethod(), false); + return startSpan(request, "HTTP " + request.getMethod(), false); + } + + @Override + protected Context customizeContext(Context context, HttpServletRequest request) { + context = super.customizeContext(context, request); return AppServerBridge.init(context, false); } diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index 7d3fcd7b772a..d3b783268722 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -48,14 +48,18 @@ public Context startSpan(REQUEST request, String spanName, boolean servlet) { accessor.setRequestAttribute(request, "trace_id", spanContext.getTraceId()); accessor.setRequestAttribute(request, "span_id", spanContext.getSpanId()); - // add context for tracking whether servlet instrumentation has updated the server span name if (servlet) { // server span name shouldn't be updated when server span was created from a call to Servlet - context = ServerSpanNaming.init(context, SERVLET); - } else { - // if created from a call to Filter then name may be updated from updateContext - context = ServerSpanNaming.init(context, CONTAINER); + // (if created from a call to Filter then name may be updated from updateContext) + ServerSpanNaming.updateSource(context, SERVLET); } + return addServletContextPath(context, request); + } + + @Override + protected Context customizeContext(Context context, REQUEST request) { + // add context for tracking whether servlet instrumentation has updated the server span name + context = ServerSpanNaming.init(context, CONTAINER); // add context for current request's context path return addServletContextPath(context, request); } diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java index c33899b5ced3..9c1b3e0f268b 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatTracer.java @@ -32,7 +32,11 @@ public abstract class TomcatTracer extends HttpServerTracer