From cac1c305222559e28654f20017055b470fb25000 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Mon, 2 Dec 2024 09:21:03 +0100 Subject: [PATCH 01/14] Service naming: split by jee deployment --- .../jbossmodules/ModuleInstrumentation.java | 14 +++- .../jbossmodules/ModuleNameHelper.java | 25 ++++++ .../liberty20/BundleNameHelper.java | 25 ++++++ .../LibertyServerInstrumentation.java | 14 +++- ...readContextClassloaderInstrumentation.java | 44 +++++++++++ .../liberty20/Liberty20Test.groovy | 14 ++++ .../instrumentation/liberty-23/build.gradle | 1 + .../LibertyServerInstrumentation.java | 14 +++- .../liberty23/Liberty23Test.groovy | 14 ++++ .../servlet2/Servlet2Advice.java | 4 + .../servlet3/Servlet3Advice.java | 4 + .../servlet3/Servlet3Decorator.java | 5 ++ .../JakartaServletInstrumentation.java | 28 ++++--- .../instrumentation/tomcat-5.5/build.gradle | 2 + .../groovy/TomcatServletTest.groovy | 9 +++ .../tomcat-classloading-9/build.gradle | 17 ++++ .../tomcat9/ContextNameHelper.java | 14 ++++ .../WebappClassLoaderInstrumentation.java | 42 ++++++++++ .../datadog/trace/api/ConfigDefaults.java | 1 + .../config/TraceInstrumentationConfig.java | 3 + .../java/datadog/trace/core/CoreTracer.java | 16 ++-- .../core/taginterceptor/TagInterceptor.java | 9 ++- .../taginterceptor/TagInterceptorTest.groovy | 48 ++++++------ .../main/java/datadog/trace/api/Config.java | 11 +++ .../api/naming/ClassloaderServiceNames.java | 78 +++++++++++++++++++ settings.gradle | 1 + 26 files changed, 413 insertions(+), 44 deletions(-) create mode 100644 dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java create mode 100644 dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java create mode 100644 dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java create mode 100644 dd-java-agent/instrumentation/tomcat-classloading-9/build.gradle create mode 100644 dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/ContextNameHelper.java create mode 100644 dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java create mode 100644 internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java diff --git a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java index 825e212129e..fb664477e2f 100644 --- a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java +++ b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java @@ -1,6 +1,8 @@ package datadog.trace.instrumentation.jbossmodules; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; +import static datadog.trace.instrumentation.jbossmodules.ModuleNameHelper.STRIPPER; +import static net.bytebuddy.matcher.ElementMatchers.isConstructor; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import static net.bytebuddy.matcher.ElementMatchers.takesArguments; @@ -8,6 +10,7 @@ import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; +import datadog.trace.api.naming.ClassloaderServiceNames; import datadog.trace.bootstrap.AgentClassLoading; import java.io.IOException; import java.io.InputStream; @@ -33,7 +36,8 @@ public String[] helperClassNames() { return new String[] { "org.jboss.modules.ModuleLinkageHelper", "org.jboss.modules.ModuleLinkageHelper$1", - "org.jboss.modules.ModuleLinkageHelper$2" + "org.jboss.modules.ModuleLinkageHelper$2", + packageName + ".ModuleNameHelper", }; } @@ -60,6 +64,7 @@ public void methodAdvice(MethodTransformer transformer) { .and(takesArgument(0, String.class)) .and(takesArgument(1, boolean.class)))), ModuleInstrumentation.class.getName() + "$WidenLoadClassAdvice"); + transformer.applyAdvice(isConstructor(), getClass().getName() + "$CaptureModuleNameAdvice"); } /** @@ -154,4 +159,11 @@ public static void onExit( } } } + + public static class CaptureModuleNameAdvice { + @Advice.OnMethodExit(suppress = Throwable.class) + public static void afterConstruct(@Advice.This final Module module) { + ClassloaderServiceNames.addIfMissing(module.getClassLoader(), STRIPPER); + } + } } diff --git a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java new file mode 100644 index 00000000000..840630fc4ed --- /dev/null +++ b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java @@ -0,0 +1,25 @@ +package datadog.trace.instrumentation.jbossmodules; + +import java.util.function.Function; +import java.util.function.Supplier; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import org.jboss.modules.ModuleClassLoader; + +public class ModuleNameHelper { + private ModuleNameHelper() {} + + private static final Pattern SUBDEPLOYMENT_MATCH = + Pattern.compile("deployment(?>.+\\.ear)?\\.(.+)\\.[j|w]ar"); + public static final Function> STRIPPER = + classLoader -> { + final Matcher matcher = + SUBDEPLOYMENT_MATCH.matcher( + ((ModuleClassLoader) classLoader).getModule().getIdentifier().getName()); + if (matcher.matches()) { + final String result = matcher.group(1); + return () -> result; + } + return () -> null; + }; +} diff --git a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java new file mode 100644 index 00000000000..1b4edc497f0 --- /dev/null +++ b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java @@ -0,0 +1,25 @@ +package datadog.trace.instrumentation.liberty20; + +import com.ibm.ws.classloading.internal.ThreadContextClassLoader; +import java.util.function.Function; +import java.util.function.Supplier; + +public class BundleNameHelper { + private BundleNameHelper() {} + + public static final Function> EXTRACTOR = + classLoader -> { + final String id = ((ThreadContextClassLoader) classLoader).getKey(); + // id is something like :name#somethingelse + final int head = id.indexOf(':'); + if (head < 0) { + return () -> null; + } + final int tail = id.lastIndexOf('#'); + if (tail < 0) { + return () -> null; + } + final String name = id.substring(head + 1, tail); + return () -> name; + }; +} diff --git a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java index eac3a358f2a..188d5b425da 100644 --- a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java @@ -12,11 +12,14 @@ import com.google.auto.service.AutoService; import com.ibm.ws.webcontainer.srt.SRTServletRequest; import com.ibm.ws.webcontainer.srt.SRTServletResponse; +import com.ibm.ws.webcontainer.webapp.WebApp; +import com.ibm.wsspi.webcontainer.webapp.IWebAppDispatcherContext; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; import datadog.trace.api.CorrelationIdentifier; import datadog.trace.api.GlobalTracer; import datadog.trace.api.gateway.Flow; +import datadog.trace.api.naming.ClassloaderServiceNames; import datadog.trace.bootstrap.ActiveSubsystems; import datadog.trace.bootstrap.ContextStore; import datadog.trace.bootstrap.InstrumentationContext; @@ -104,7 +107,16 @@ public static class HandleRequestAdvice { request.setAttribute(DD_EXTRACTED_CONTEXT_ATTRIBUTE, extractedContext); final AgentSpan span = DECORATE.startSpan(request, extractedContext); scope = activateSpan(span, true); - + final IWebAppDispatcherContext dispatcherContext = request.getWebAppDispatcherContext(); + if (dispatcherContext != null) { + final WebApp webapp = dispatcherContext.getWebApp(); + if (webapp != null) { + final ClassLoader cl = webapp.getClassLoader(); + if (cl != null) { + ClassloaderServiceNames.maybeSetToSpan(span::setServiceName, span::getServiceName, cl); + } + } + } DECORATE.afterStart(span); DECORATE.onRequest(span, request, request, extractedContext); request.setAttribute(DD_SPAN_ATTRIBUTE, span); diff --git a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java new file mode 100644 index 00000000000..53a87035c2e --- /dev/null +++ b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java @@ -0,0 +1,44 @@ +package datadog.trace.instrumentation.liberty20; + +import static net.bytebuddy.matcher.ElementMatchers.isConstructor; + +import com.google.auto.service.AutoService; +import com.ibm.ws.classloading.internal.ThreadContextClassLoader; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.agent.tooling.InstrumenterModule; +import datadog.trace.api.naming.ClassloaderServiceNames; +import net.bytebuddy.asm.Advice; + +@AutoService(InstrumenterModule.class) +public class ThreadContextClassloaderInstrumentation extends InstrumenterModule.Tracing + implements Instrumenter.ForSingleType { + + public ThreadContextClassloaderInstrumentation() { + super("liberty", "liberty-classloading"); + } + + @Override + public String instrumentedType() { + return "com.ibm.ws.classloading.internal.ThreadContextClassLoader"; + } + + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".BundleNameHelper", + }; + } + + @Override + public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvice( + isConstructor(), getClass().getName() + "$ThreadContextClassloaderAdvice"); + } + + public static class ThreadContextClassloaderAdvice { + @Advice.OnMethodExit(suppress = Throwable.class) + public static void afterConstruct(@Advice.This ThreadContextClassLoader self) { + ClassloaderServiceNames.addIfMissing(self, BundleNameHelper.EXTRACTOR); + } + } +} diff --git a/dd-java-agent/instrumentation/liberty-20/src/test/groovy/datadog/trace/instrumentation/liberty20/Liberty20Test.groovy b/dd-java-agent/instrumentation/liberty-20/src/test/groovy/datadog/trace/instrumentation/liberty20/Liberty20Test.groovy index 7e64aafba50..56c3ca15dc0 100644 --- a/dd-java-agent/instrumentation/liberty-20/src/test/groovy/datadog/trace/instrumentation/liberty20/Liberty20Test.groovy +++ b/dd-java-agent/instrumentation/liberty-20/src/test/groovy/datadog/trace/instrumentation/liberty20/Liberty20Test.groovy @@ -213,6 +213,20 @@ class Liberty20AsyncForkedTest extends Liberty20Test implements TestingGenericHt } } +@IgnoreIf({ + // failing because org.apache.xalan.transformer.TransformerImpl is + // instrumented while on the the global ignores list + System.getProperty('java.vm.name') == 'IBM J9 VM' && + System.getProperty('java.specification.version') == '1.8' }) +class LibertyServletClassloaderNamingForkedTest extends Liberty20V0ForkedTest { + @Override + protected void configurePreAgent() { + super.configurePreAgent() + // will not set the service name according to the servlet context value + injectSysConfig("trace.experimental.jee.split-by-deployment", "true") + } +} + @IgnoreIf({ // failing because org.apache.xalan.transformer.TransformerImpl is // instrumented while on the the global ignores list diff --git a/dd-java-agent/instrumentation/liberty-23/build.gradle b/dd-java-agent/instrumentation/liberty-23/build.gradle index 75e5b97f3d0..8d88ad12b93 100644 --- a/dd-java-agent/instrumentation/liberty-23/build.gradle +++ b/dd-java-agent/instrumentation/liberty-23/build.gradle @@ -34,6 +34,7 @@ dependencies { testImplementation testFixtures(project(':dd-java-agent:appsec')) testRuntimeOnly project(':dd-java-agent:instrumentation:osgi-4.3') testRuntimeOnly files({ tasks.filterLogbackClassic.filteredLogbackDir }) + testRuntimeOnly project(':dd-java-agent:instrumentation:liberty-20') testRuntimeOnly project(':dd-java-agent:instrumentation:servlet:request-5') testRuntimeOnly files({ tasks.shadowJar.archiveFile }) diff --git a/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java b/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java index 3209bbed508..0d7b5a7d0fd 100644 --- a/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java @@ -12,11 +12,14 @@ import com.google.auto.service.AutoService; import com.ibm.ws.webcontainer.srt.SRTServletRequest; import com.ibm.ws.webcontainer.srt.SRTServletResponse; +import com.ibm.ws.webcontainer.webapp.WebApp; +import com.ibm.wsspi.webcontainer.webapp.IWebAppDispatcherContext; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; import datadog.trace.api.CorrelationIdentifier; import datadog.trace.api.GlobalTracer; import datadog.trace.api.gateway.Flow; +import datadog.trace.api.naming.ClassloaderServiceNames; import datadog.trace.bootstrap.ActiveSubsystems; import datadog.trace.bootstrap.ContextStore; import datadog.trace.bootstrap.InstrumentationContext; @@ -106,7 +109,16 @@ public static class HandleRequestAdvice { request.setAttribute(DD_EXTRACTED_CONTEXT_ATTRIBUTE, extractedContext); final AgentSpan span = DECORATE.startSpan(request, extractedContext); scope = activateSpan(span, true); - + final IWebAppDispatcherContext dispatcherContext = request.getWebAppDispatcherContext(); + if (dispatcherContext != null) { + final WebApp webapp = dispatcherContext.getWebApp(); + if (webapp != null) { + final ClassLoader cl = webapp.getClassLoader(); + if (cl != null) { + ClassloaderServiceNames.maybeSetToSpan(span::setServiceName, span::getServiceName, cl); + } + } + } DECORATE.afterStart(span); DECORATE.onRequest(span, request, request, extractedContext); request.setAttribute(DD_SPAN_ATTRIBUTE, span); diff --git a/dd-java-agent/instrumentation/liberty-23/src/test/groovy/datadog/trace/instrumentation/liberty23/Liberty23Test.groovy b/dd-java-agent/instrumentation/liberty-23/src/test/groovy/datadog/trace/instrumentation/liberty23/Liberty23Test.groovy index 5ad1162f4ff..805e2dcfe97 100644 --- a/dd-java-agent/instrumentation/liberty-23/src/test/groovy/datadog/trace/instrumentation/liberty23/Liberty23Test.groovy +++ b/dd-java-agent/instrumentation/liberty-23/src/test/groovy/datadog/trace/instrumentation/liberty23/Liberty23Test.groovy @@ -170,3 +170,17 @@ class Liberty23V0ForkedTest extends Liberty23Test implements TestingGenericHttpN System.getProperty('java.specification.version') == '1.8' }) class Liberty23V1ForkedTest extends Liberty23Test implements TestingGenericHttpNamingConventions.ServerV1 { } + +@IgnoreIf({ + // failing because org.apache.xalan.transformer.TransformerImpl is + // instrumented while on the the global ignores list + System.getProperty('java.vm.name') == 'IBM J9 VM' && + System.getProperty('java.specification.version') == '1.8' }) +class LibertyServletClassloaderNamingForkedTest extends Liberty23V0ForkedTest { + @Override + protected void configurePreAgent() { + super.configurePreAgent() + // will not set the service name according to the servlet context value + injectSysConfig("trace.experimental.jee.split-by-deployment", "true") + } +} diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java index 8f27ba741b0..db6e8ac9c40 100644 --- a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java @@ -9,6 +9,7 @@ import datadog.trace.api.DDTags; import datadog.trace.api.GlobalTracer; import datadog.trace.api.gateway.Flow; +import datadog.trace.api.naming.ClassloaderServiceNames; import datadog.trace.bootstrap.InstrumentationContext; import datadog.trace.bootstrap.instrumentation.api.AgentScope; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; @@ -39,6 +40,9 @@ public static boolean onEnter( Object spanAttr = request.getAttribute(DD_SPAN_ATTRIBUTE); final boolean hasServletTrace = spanAttr instanceof AgentSpan; if (hasServletTrace) { + final AgentSpan span = (AgentSpan) spanAttr; + ClassloaderServiceNames.maybeSetToSpan( + span::setServiceName, span::getServiceName, Thread.currentThread()); // Tracing might already be applied by the FilterChain or a parent request (forward/include). return false; } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java index 8171a5d5d4f..ee58b9b156b 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java @@ -12,6 +12,7 @@ import datadog.trace.api.DDTags; import datadog.trace.api.GlobalTracer; import datadog.trace.api.gateway.Flow; +import datadog.trace.api.naming.ClassloaderServiceNames; import datadog.trace.bootstrap.instrumentation.api.AgentScope; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.instrumentation.servlet.ServletBlockingHelper; @@ -60,6 +61,9 @@ public static boolean onEnter( Object spanAttrValue = request.getAttribute(DD_SPAN_ATTRIBUTE); final boolean hasServletTrace = spanAttrValue instanceof AgentSpan; if (hasServletTrace) { + final AgentSpan span = (AgentSpan) spanAttrValue; + ClassloaderServiceNames.maybeSetToSpan( + span::setServiceName, span::getServiceName, Thread.currentThread()); // Tracing might already be applied by other instrumentation, // the FilterChain or a parent request (forward/include). return false; diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java index 8d5744e3590..900a4761da1 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.servlet3; +import datadog.trace.api.naming.ClassloaderServiceNames; import datadog.trace.bootstrap.instrumentation.api.AgentPropagation; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.URIDataAdapter; @@ -82,6 +83,10 @@ public AgentSpan onRequest( final HttpServletRequest request, AgentSpan.Context.Extracted context) { assert span != null; + final String eeService = ClassloaderServiceNames.maybeGetForThread(Thread.currentThread()); + if (eeService != null) { + span.setServiceName(eeService); + } if (request != null) { String contextPath = request.getContextPath(); String servletPath = request.getServletPath(); diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java index b4d4084344f..08bfddc1199 100644 --- a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java @@ -12,6 +12,7 @@ import datadog.trace.agent.tooling.InstrumenterModule; import datadog.trace.api.Config; import datadog.trace.api.DDTags; +import datadog.trace.api.naming.ClassloaderServiceNames; import datadog.trace.bootstrap.CallDepthThreadLocalMap; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import jakarta.servlet.ServletRequest; @@ -51,29 +52,36 @@ public void methodAdvice(MethodTransformer transformer) { public static class ExtractPrincipalAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) - public static boolean before(@Advice.Argument(0) final ServletRequest request) { + public static boolean before( + @Advice.Argument(0) final ServletRequest request, + @Advice.Local("span") AgentSpan agentSpan) { if (!(request instanceof HttpServletRequest)) { return false; } + Object span = + request.getAttribute( + "datadog.span"); // hardcode to avoid injecting HttpServiceDecorator just for this + if (span instanceof AgentSpan) { + agentSpan = (AgentSpan) span; + ClassloaderServiceNames.maybeSetToSpan( + agentSpan::setServiceName, agentSpan::getServiceName, Thread.currentThread()); + } return CallDepthThreadLocalMap.incrementCallDepth(HttpServletRequest.class) == 0; } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void after( - @Advice.Enter boolean advice, @Advice.Argument(0) final ServletRequest request) { + @Advice.Enter boolean advice, + @Advice.Argument(0) final ServletRequest request, + @Advice.Local("span") AgentSpan span) { if (advice) { CallDepthThreadLocalMap.reset(HttpServletRequest.class); final HttpServletRequest httpServletRequest = (HttpServletRequest) request; // at this point the cast should be safe - if (Config.get().isServletPrincipalEnabled() + if (span != null + && Config.get().isServletPrincipalEnabled() && httpServletRequest.getUserPrincipal() != null) { - Object span = - request.getAttribute( - "datadog.span"); // hardcode to avoid injecting HttpServiceDecorator just for this - if (span instanceof AgentSpan) { - ((AgentSpan) span) - .setTag(DDTags.USER_NAME, httpServletRequest.getUserPrincipal().getName()); - } + span.setTag(DDTags.USER_NAME, httpServletRequest.getUserPrincipal().getName()); } } } diff --git a/dd-java-agent/instrumentation/tomcat-5.5/build.gradle b/dd-java-agent/instrumentation/tomcat-5.5/build.gradle index fbee4c3d985..8ee94a412e3 100644 --- a/dd-java-agent/instrumentation/tomcat-5.5/build.gradle +++ b/dd-java-agent/instrumentation/tomcat-5.5/build.gradle @@ -98,6 +98,7 @@ dependencies { latest10TestImplementation(project(':dd-java-agent:instrumentation:tomcat-appsec-6')) latest10TestImplementation(project(':dd-java-agent:instrumentation:tomcat-appsec-7')) + latest10TestImplementation(project(':dd-java-agent:instrumentation:tomcat-classloading-9')) latestDepTestImplementation group: 'org.apache.tomcat.embed', name: 'tomcat-embed-core', version: '11.+' latestDepTestImplementation group: 'org.apache.tomcat', name: 'jakartaee-migration', version: '1.+' @@ -105,6 +106,7 @@ dependencies { latestDepTestRuntimeOnly(project(':dd-java-agent:instrumentation:tomcat-appsec-6')) latestDepTestRuntimeOnly(project(':dd-java-agent:instrumentation:tomcat-appsec-7')) + latestDepTestRuntimeOnly(project(':dd-java-agent:instrumentation:tomcat-classloading-9')) } // Exclude all the dependencies from test for latestDepTest since the names are completely different. diff --git a/dd-java-agent/instrumentation/tomcat-5.5/src/latestDepTest/groovy/TomcatServletTest.groovy b/dd-java-agent/instrumentation/tomcat-5.5/src/latestDepTest/groovy/TomcatServletTest.groovy index 147ebacbd61..3569246fd0a 100644 --- a/dd-java-agent/instrumentation/tomcat-5.5/src/latestDepTest/groovy/TomcatServletTest.groovy +++ b/dd-java-agent/instrumentation/tomcat-5.5/src/latestDepTest/groovy/TomcatServletTest.groovy @@ -204,4 +204,13 @@ class TomcatServletTest extends AbstractServletTest { } } +class TomcatServletClassloaderNamingForkedTest extends TomcatServletTest { + @Override + protected void configurePreAgent() { + super.configurePreAgent() + // will not set the service name according to the servlet context value + injectSysConfig("trace.experimental.jee.split-by-deployment", "true") + } +} + diff --git a/dd-java-agent/instrumentation/tomcat-classloading-9/build.gradle b/dd-java-agent/instrumentation/tomcat-classloading-9/build.gradle new file mode 100644 index 00000000000..24c01c62e16 --- /dev/null +++ b/dd-java-agent/instrumentation/tomcat-classloading-9/build.gradle @@ -0,0 +1,17 @@ +evaluationDependsOn ':dd-java-agent:instrumentation:tomcat-5.5' + +muzzle { + pass { + group = 'org.apache.tomcat' + module = 'tomcat-catalina' + versions = '[9.0.0.M1,)' + } +} + +apply from: "$rootDir/gradle/java.gradle" + +dependencies { + compileOnly group: 'org.apache.tomcat', name: 'tomcat-catalina', version: '9.0.0.M1' +} + +// testing happens in tomcat-5.5 module diff --git a/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/ContextNameHelper.java b/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/ContextNameHelper.java new file mode 100644 index 00000000000..9daa69c16a3 --- /dev/null +++ b/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/ContextNameHelper.java @@ -0,0 +1,14 @@ +package datadog.trace.instrumentation.tomcat9; + +import java.util.function.Function; +import java.util.function.Supplier; +import org.apache.catalina.loader.WebappClassLoaderBase; + +public class ContextNameHelper { + private ContextNameHelper() {} + + public static final Function> ADDER = + // tomcat does not initialize the context name until the context is started and the + // classloader is created too early to cache that value + classLoader -> ((WebappClassLoaderBase) classLoader)::getContextName; +} diff --git a/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java b/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java new file mode 100644 index 00000000000..694e9c56c6c --- /dev/null +++ b/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java @@ -0,0 +1,42 @@ +package datadog.trace.instrumentation.tomcat9; + +import static net.bytebuddy.matcher.ElementMatchers.isConstructor; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.agent.tooling.InstrumenterModule; +import datadog.trace.api.naming.ClassloaderServiceNames; +import net.bytebuddy.asm.Advice; +import org.apache.catalina.loader.WebappClassLoaderBase; + +@AutoService(InstrumenterModule.class) +public class WebappClassLoaderInstrumentation extends InstrumenterModule.Tracing + implements Instrumenter.ForSingleType { + public WebappClassLoaderInstrumentation() { + super("tomcat", "tomcat-classloading"); + } + + @Override + public String instrumentedType() { + return "org.apache.catalina.loader.WebappClassLoaderBase"; + } + + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".ContextNameHelper", + }; + } + + @Override + public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvice(isConstructor(), getClass().getName() + "$CaptureWebappNameAdvice"); + } + + public static class CaptureWebappNameAdvice { + @Advice.OnMethodExit(suppress = Throwable.class) + public static void afterConstruct(@Advice.This final WebappClassLoaderBase classLoader) { + ClassloaderServiceNames.addIfMissing(classLoader, ContextNameHelper.ADDER); + } + } +} diff --git a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java index 9c12e7dda8c..c9ae677dfea 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java @@ -58,6 +58,7 @@ public final class ConfigDefaults { static final boolean DEFAULT_RUNTIME_CONTEXT_FIELD_INJECTION = true; static final boolean DEFAULT_SERIALVERSIONUID_FIELD_INJECTION = true; + static final boolean DEFAULT_EXPERIMENTATAL_JEE_SPLIT_BY_DEPLOYMENT = false; static final boolean DEFAULT_PRIORITY_SAMPLING_ENABLED = true; static final String DEFAULT_PRIORITY_SAMPLING_FORCE = null; static final boolean DEFAULT_TRACE_RESOLVER_ENABLED = true; diff --git a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java index 7fca0156733..a875c5da7ea 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java @@ -70,6 +70,9 @@ public final class TraceInstrumentationConfig { public static final String JDBC_CONNECTION_CLASS_NAME = "trace.jdbc.connection.class.name"; + public static final String EXPERIMENTATAL_JEE_SPLIT_BY_DEPLOYMENT = + "trace.experimental.jee.split-by-deployment"; + public static final String HTTP_URL_CONNECTION_CLASS_NAME = "trace.http.url.connection.class.name"; diff --git a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java index 4be835039a2..43ad02677c2 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java @@ -40,6 +40,7 @@ import datadog.trace.api.interceptor.TraceInterceptor; import datadog.trace.api.internal.TraceSegment; import datadog.trace.api.metrics.SpanMetricRegistry; +import datadog.trace.api.naming.ClassloaderServiceNames; import datadog.trace.api.naming.SpanNaming; import datadog.trace.api.remoteconfig.ServiceNameCollector; import datadog.trace.api.sampling.PrioritySampling; @@ -1599,11 +1600,16 @@ private DDSpanContext buildSpanContext() { } if (serviceName == null) { serviceName = traceConfig.getPreferredServiceName(); - if (serviceName == null) { - // it could be on the initial snapshot but may be overridden to null and service name - // cannot be null - serviceName = CoreTracer.this.serviceName; - } + } + if (serviceName == null && parentServiceName == null) { + // in this case we have a local root without service name. We can try to see if we can find + // one from the thread context classloader + serviceName = ClassloaderServiceNames.maybeGetForThread(Thread.currentThread()); + } + if (serviceName == null) { + // it could be on the initial snapshot but may be overridden to null and service name + // cannot be null + serviceName = CoreTracer.this.serviceName; } final CharSequence operationName = diff --git a/dd-trace-core/src/main/java/datadog/trace/core/taginterceptor/TagInterceptor.java b/dd-trace-core/src/main/java/datadog/trace/core/taginterceptor/TagInterceptor.java index 8229c5b42a9..56ae6da4c75 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/taginterceptor/TagInterceptor.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/taginterceptor/TagInterceptor.java @@ -51,20 +51,23 @@ public class TagInterceptor { private final boolean shouldSet404ResourceName; private final boolean shouldSetUrlResourceAsName; + private final boolean jeeSplitByDeployment; public TagInterceptor(RuleFlags ruleFlags) { this( Config.get().isServiceNameSetByUser(), CapturedEnvironment.get().getProperties().get(GeneralConfig.SERVICE_NAME), Config.get().getSplitByTags(), - ruleFlags); + ruleFlags, + Config.get().isJeeSplitByDeployment()); } public TagInterceptor( boolean isServiceNameSetByUser, String inferredServiceName, Set splitServiceTags, - RuleFlags ruleFlags) { + RuleFlags ruleFlags, + boolean jeeSplitByDeployment) { this.isServiceNameSetByUser = isServiceNameSetByUser; this.inferredServiceName = inferredServiceName; this.splitServiceTags = splitServiceTags; @@ -76,6 +79,7 @@ public TagInterceptor( && ruleFlags.isEnabled(STATUS_404) && ruleFlags.isEnabled(STATUS_404_DECORATOR); shouldSetUrlResourceAsName = ruleFlags.isEnabled(URL_AS_RESOURCE_NAME); + this.jeeSplitByDeployment = jeeSplitByDeployment; } public boolean interceptTag(DDSpanContext span, String tag, Object value) { @@ -276,6 +280,7 @@ private boolean interceptServletContext(DDSpanContext span, Object value) { // so will always return false here. if (!splitByServletContext && (isServiceNameSetByUser + || jeeSplitByDeployment || !ruleFlags.isEnabled(RuleFlags.Feature.SERVLET_CONTEXT) || !span.getServiceName().isEmpty() && !span.getServiceName().equals(inferredServiceName) diff --git a/dd-trace-core/src/test/groovy/datadog/trace/core/taginterceptor/TagInterceptorTest.groovy b/dd-trace-core/src/test/groovy/datadog/trace/core/taginterceptor/TagInterceptorTest.groovy index b91cb2210b6..6c2840d28f6 100644 --- a/dd-trace-core/src/test/groovy/datadog/trace/core/taginterceptor/TagInterceptorTest.groovy +++ b/dd-trace-core/src/test/groovy/datadog/trace/core/taginterceptor/TagInterceptorTest.groovy @@ -1,8 +1,5 @@ package datadog.trace.core.taginterceptor -import datadog.trace.core.DDSpanContext -import datadog.trace.api.remoteconfig.ServiceNameCollector - import static datadog.trace.api.ConfigDefaults.DEFAULT_SERVICE_NAME import static datadog.trace.api.ConfigDefaults.DEFAULT_SERVLET_ROOT_CONTEXT_SERVICE_NAME import static datadog.trace.api.DDTags.ANALYTICS_SAMPLE_RATE @@ -12,6 +9,7 @@ import datadog.trace.api.DDSpanTypes import datadog.trace.api.DDTags import datadog.trace.api.config.GeneralConfig import datadog.trace.api.env.CapturedEnvironment +import datadog.trace.api.remoteconfig.ServiceNameCollector import datadog.trace.api.sampling.PrioritySampling import datadog.trace.bootstrap.instrumentation.api.AgentSpan import datadog.trace.bootstrap.instrumentation.api.InstrumentationTags @@ -20,6 +18,7 @@ import datadog.trace.common.sampling.AllSampler import datadog.trace.common.writer.ListWriter import datadog.trace.common.writer.LoggingWriter import datadog.trace.core.CoreSpan +import datadog.trace.core.DDSpanContext import datadog.trace.core.test.DDCoreSpecification class TagInterceptorTest extends DDCoreSpecification { @@ -182,29 +181,30 @@ class TagInterceptorTest extends DDCoreSpecification { .build() } - def "split-by-tags for servlet.context"() { + def "split-by-tags for servlet.context and experimental jee split by deployment is #jeeActive"() { setup: - def tracer = createSplittingTracer(InstrumentationTags.SERVLET_CONTEXT) - - when: - def span = tracer.buildSpan("some span") - .withTag(InstrumentationTags.SERVLET_CONTEXT, "some-context") - .start() - span.finish() - - then: - span.serviceName == "some-context" - + def tracer = tracerBuilder() + .serviceName("my-service") + .writer(new LoggingWriter()) + .sampler(new AllSampler()) + .tagInterceptor(new TagInterceptor(false, "my-service", + Collections.emptySet(), new RuleFlags(), jeeActive)) + .build() when: - span = tracer.buildSpan("some span").start() + def span = tracer.buildSpan("some span").start() span.setTag(InstrumentationTags.SERVLET_CONTEXT, "some-context") span.finish() then: - span.serviceName == "some-context" + span.serviceName == expected cleanup: tracer.close() + + where: + expected | jeeActive + "some-context" | false + "my-service" | true } def "peer.service then split-by-tags via builder"() { @@ -694,7 +694,7 @@ class TagInterceptorTest extends DDCoreSpecification { tracer.close() } - void "when interceptServiceName extraServiceProvider is called"(){ + void "when interceptServiceName extraServiceProvider is called"() { setup: final extraServiceProvider = Mock(ServiceNameCollector) ServiceNameCollector.INSTANCE = extraServiceProvider @@ -709,7 +709,7 @@ class TagInterceptorTest extends DDCoreSpecification { 1 * extraServiceProvider.addService("some-service") } - void "when interceptServletContext extraServiceProvider is called"(){ + void "when interceptServletContext extraServiceProvider is called"() { setup: final extraServiceProvider = Mock(ServiceNameCollector) ServiceNameCollector.INSTANCE = extraServiceProvider @@ -724,13 +724,13 @@ class TagInterceptorTest extends DDCoreSpecification { 1 * extraServiceProvider.addService(expected) where: - value | expected - "/" | "root-servlet" - "/test" | "test" - "test" | "test" + value | expected + "/" | "root-servlet" + "/test" | "test" + "test" | "test" } - void "When intercepts appsec propagation tag addAppsecPropagationTag is called"(){ + void "When intercepts appsec propagation tag addAppsecPropagationTag is called"() { setup: final ruleFlags = Mock(RuleFlags) ruleFlags.isEnabled(_) >> true diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index 8b5a2cc40d8..7cf8b5ffe83 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -172,6 +172,7 @@ public static String getHostName() { private final boolean dbClientSplitByInstanceTypeSuffix; private final boolean dbClientSplitByHost; private final Set splitByTags; + private final boolean jeeSplitByDeployment; private final int scopeDepthLimit; private final boolean scopeStrictMode; private final int scopeIterationKeepAlive; @@ -848,6 +849,10 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins splitByTags = tryMakeImmutableSet(configProvider.getList(SPLIT_BY_TAGS)); + jeeSplitByDeployment = + configProvider.getBoolean( + EXPERIMENTATAL_JEE_SPLIT_BY_DEPLOYMENT, DEFAULT_EXPERIMENTATAL_JEE_SPLIT_BY_DEPLOYMENT); + springDataRepositoryInterfaceResourceName = configProvider.getBoolean(SPRING_DATA_REPOSITORY_INTERFACE_RESOURCE_NAME, true); @@ -2071,6 +2076,10 @@ public Set getSplitByTags() { return splitByTags; } + public boolean isJeeSplitByDeployment() { + return jeeSplitByDeployment; + } + public int getScopeDepthLimit() { return scopeDepthLimit; } @@ -4178,6 +4187,8 @@ public String toString() { + DBMPropagationMode + ", splitByTags=" + splitByTags + + ", jeeSplitByDeployment=" + + jeeSplitByDeployment + ", scopeDepthLimit=" + scopeDepthLimit + ", scopeStrictMode=" diff --git a/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java b/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java new file mode 100644 index 00000000000..589c3b55478 --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java @@ -0,0 +1,78 @@ +package datadog.trace.api.naming; + +import datadog.trace.api.Config; +import datadog.trace.api.config.GeneralConfig; +import datadog.trace.api.env.CapturedEnvironment; +import datadog.trace.api.remoteconfig.ServiceNameCollector; +import java.util.WeakHashMap; +import java.util.function.Consumer; +import java.util.function.Function; +import java.util.function.Supplier; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + +public class ClassloaderServiceNames { + private static class Lazy { + private static final ClassloaderServiceNames INSTANCE = new ClassloaderServiceNames(); + } + + private final WeakHashMap> weakCache = new WeakHashMap<>(); + private final String inferredServiceName = + CapturedEnvironment.get().getProperties().get(GeneralConfig.SERVICE_NAME); + private final boolean enabled = + Config.get().isJeeSplitByDeployment() && !Config.get().isServiceNameSetByUser(); + + private ClassloaderServiceNames() {} + + public static void addIfMissing( + @Nonnull ClassLoader classLoader, + @Nonnull Function> adder) { + if (Lazy.INSTANCE.enabled) { + Lazy.INSTANCE.weakCache.computeIfAbsent(classLoader, adder); + } + } + + @Nullable + public static String maybeGet(@Nonnull ClassLoader classLoader) { + if (Lazy.INSTANCE.enabled) { + final Supplier supplier = Lazy.INSTANCE.weakCache.get(classLoader); + if (supplier != null) { + return supplier.get(); + } + } + return null; + } + + @Nullable + public static String maybeGetForThread(@Nonnull final Thread thread) { + return maybeGet(thread.getContextClassLoader()); + } + + public static boolean maybeSetToSpan( + @Nonnull final Consumer setter, + @Nonnull final Supplier getter, + @Nonnull final Thread thread) { + return maybeSetToSpan(setter, getter, thread.getContextClassLoader()); + } + + public static boolean maybeSetToSpan( + @Nonnull final Consumer setter, + @Nonnull final Supplier getter, + @Nonnull final ClassLoader classLoader) { + if (!Lazy.INSTANCE.enabled) { + return false; + } + final String currentServiceName = getter.get(); + if (currentServiceName != null + && !currentServiceName.equals(Lazy.INSTANCE.inferredServiceName)) { + return false; + } + final String service = maybeGet(classLoader); + if (service != null) { + setter.accept(service); + ServiceNameCollector.get().addService(service); + return true; + } + return false; + } +} diff --git a/settings.gradle b/settings.gradle index 41c55a18d48..2d78784b023 100644 --- a/settings.gradle +++ b/settings.gradle @@ -481,6 +481,7 @@ include ':dd-java-agent:instrumentation:tomcat-5.5-common' include ':dd-java-agent:instrumentation:tomcat-appsec-5.5' include ':dd-java-agent:instrumentation:tomcat-appsec-6' include ':dd-java-agent:instrumentation:tomcat-appsec-7' +include ':dd-java-agent:instrumentation:tomcat-classloading-9' include ':dd-java-agent:instrumentation:trace-annotation' include ':dd-java-agent:instrumentation:twilio' include ':dd-java-agent:instrumentation:unbescape' From 47a72fc4c06c6e0179cc2ab6d3df9872cb6e594c Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Fri, 6 Dec 2024 16:56:37 +0100 Subject: [PATCH 02/14] Do not use invokedynamic --- .../liberty20/LibertyServerInstrumentation.java | 2 +- .../liberty23/LibertyServerInstrumentation.java | 2 +- .../instrumentation/servlet2/Servlet2Advice.java | 3 +-- .../instrumentation/servlet3/Servlet3Advice.java | 3 +-- .../servlet5/JakartaServletInstrumentation.java | 3 +-- .../api/naming/ClassloaderServiceNames.java | 16 ++++++---------- 6 files changed, 11 insertions(+), 18 deletions(-) diff --git a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java index 188d5b425da..c75cf7ad5ac 100644 --- a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java @@ -113,7 +113,7 @@ public static class HandleRequestAdvice { if (webapp != null) { final ClassLoader cl = webapp.getClassLoader(); if (cl != null) { - ClassloaderServiceNames.maybeSetToSpan(span::setServiceName, span::getServiceName, cl); + ClassloaderServiceNames.maybeSetToSpan(span, cl); } } } diff --git a/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java b/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java index 0d7b5a7d0fd..bae8f137446 100644 --- a/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java @@ -115,7 +115,7 @@ public static class HandleRequestAdvice { if (webapp != null) { final ClassLoader cl = webapp.getClassLoader(); if (cl != null) { - ClassloaderServiceNames.maybeSetToSpan(span::setServiceName, span::getServiceName, cl); + ClassloaderServiceNames.maybeSetToSpan(span, cl); } } } diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java index db6e8ac9c40..aa01ee62496 100644 --- a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java @@ -41,8 +41,7 @@ public static boolean onEnter( final boolean hasServletTrace = spanAttr instanceof AgentSpan; if (hasServletTrace) { final AgentSpan span = (AgentSpan) spanAttr; - ClassloaderServiceNames.maybeSetToSpan( - span::setServiceName, span::getServiceName, Thread.currentThread()); + ClassloaderServiceNames.maybeSetToSpan(span, Thread.currentThread()); // Tracing might already be applied by the FilterChain or a parent request (forward/include). return false; } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java index ee58b9b156b..154ac4da1a5 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java @@ -62,8 +62,7 @@ public static boolean onEnter( final boolean hasServletTrace = spanAttrValue instanceof AgentSpan; if (hasServletTrace) { final AgentSpan span = (AgentSpan) spanAttrValue; - ClassloaderServiceNames.maybeSetToSpan( - span::setServiceName, span::getServiceName, Thread.currentThread()); + ClassloaderServiceNames.maybeSetToSpan(span, Thread.currentThread()); // Tracing might already be applied by other instrumentation, // the FilterChain or a parent request (forward/include). return false; diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java index 08bfddc1199..459da225f49 100644 --- a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java @@ -63,8 +63,7 @@ public static boolean before( "datadog.span"); // hardcode to avoid injecting HttpServiceDecorator just for this if (span instanceof AgentSpan) { agentSpan = (AgentSpan) span; - ClassloaderServiceNames.maybeSetToSpan( - agentSpan::setServiceName, agentSpan::getServiceName, Thread.currentThread()); + ClassloaderServiceNames.maybeSetToSpan(agentSpan, Thread.currentThread()); } return CallDepthThreadLocalMap.incrementCallDepth(HttpServletRequest.class) == 0; } diff --git a/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java b/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java index 589c3b55478..d28c2dde3e2 100644 --- a/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java +++ b/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java @@ -4,8 +4,8 @@ import datadog.trace.api.config.GeneralConfig; import datadog.trace.api.env.CapturedEnvironment; import datadog.trace.api.remoteconfig.ServiceNameCollector; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import java.util.WeakHashMap; -import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; import javax.annotation.Nonnull; @@ -49,27 +49,23 @@ public static String maybeGetForThread(@Nonnull final Thread thread) { } public static boolean maybeSetToSpan( - @Nonnull final Consumer setter, - @Nonnull final Supplier getter, - @Nonnull final Thread thread) { - return maybeSetToSpan(setter, getter, thread.getContextClassLoader()); + @Nonnull final AgentSpan span, @Nonnull final Thread thread) { + return maybeSetToSpan(span, thread.getContextClassLoader()); } public static boolean maybeSetToSpan( - @Nonnull final Consumer setter, - @Nonnull final Supplier getter, - @Nonnull final ClassLoader classLoader) { + @Nonnull final AgentSpan span, @Nonnull final ClassLoader classLoader) { if (!Lazy.INSTANCE.enabled) { return false; } - final String currentServiceName = getter.get(); + final String currentServiceName = span.getServiceName(); if (currentServiceName != null && !currentServiceName.equals(Lazy.INSTANCE.inferredServiceName)) { return false; } final String service = maybeGet(classLoader); if (service != null) { - setter.accept(service); + span.setServiceName(service); ServiceNameCollector.get().addService(service); return true; } From fe24e628e36e874a64318a67b39728e50e807c6e Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Fri, 6 Dec 2024 20:15:31 +0100 Subject: [PATCH 03/14] fix tests --- .../trace/core/taginterceptor/TagInterceptorTest.groovy | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dd-trace-core/src/test/groovy/datadog/trace/core/taginterceptor/TagInterceptorTest.groovy b/dd-trace-core/src/test/groovy/datadog/trace/core/taginterceptor/TagInterceptorTest.groovy index 6c2840d28f6..9156ce067bb 100644 --- a/dd-trace-core/src/test/groovy/datadog/trace/core/taginterceptor/TagInterceptorTest.groovy +++ b/dd-trace-core/src/test/groovy/datadog/trace/core/taginterceptor/TagInterceptorTest.groovy @@ -177,7 +177,7 @@ class TagInterceptorTest extends DDCoreSpecification { .sampler(new AllSampler()) // equivalent to split-by-tags: tag .tagInterceptor(new TagInterceptor(true, "my-service", - Collections.singleton(tag), new RuleFlags())) + Collections.singleton(tag), new RuleFlags(), false)) .build() } @@ -700,7 +700,7 @@ class TagInterceptorTest extends DDCoreSpecification { ServiceNameCollector.INSTANCE = extraServiceProvider final ruleFlags = Mock(RuleFlags) ruleFlags.isEnabled(_) >> true - final interceptor = new TagInterceptor(true, "my-service", Collections.singleton(DDTags.SERVICE_NAME), ruleFlags) + final interceptor = new TagInterceptor(true, "my-service", Collections.singleton(DDTags.SERVICE_NAME), ruleFlags, false) when: interceptor.interceptServiceName(null, Mock(DDSpanContext), "some-service") @@ -715,7 +715,7 @@ class TagInterceptorTest extends DDCoreSpecification { ServiceNameCollector.INSTANCE = extraServiceProvider final ruleFlags = Mock(RuleFlags) ruleFlags.isEnabled(_) >> true - final interceptor = new TagInterceptor(true, "my-service", Collections.singleton("servlet.context"), ruleFlags) + final interceptor = new TagInterceptor(true, "my-service", Collections.singleton("servlet.context"), ruleFlags, false) when: interceptor.interceptServletContext(Mock(DDSpanContext), value) From e9c2536f54e76bb916a7a5da114582e83166e778 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Mon, 9 Dec 2024 08:51:50 +0100 Subject: [PATCH 04/14] collapse enter and local --- .../JakartaServletInstrumentation.java | 37 +++++++++---------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java index 459da225f49..4d8e8e171c6 100644 --- a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java @@ -52,36 +52,35 @@ public void methodAdvice(MethodTransformer transformer) { public static class ExtractPrincipalAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) - public static boolean before( - @Advice.Argument(0) final ServletRequest request, - @Advice.Local("span") AgentSpan agentSpan) { + public static AgentSpan before(@Advice.Argument(0) final ServletRequest request) { if (!(request instanceof HttpServletRequest)) { - return false; + return null; } Object span = request.getAttribute( "datadog.span"); // hardcode to avoid injecting HttpServiceDecorator just for this - if (span instanceof AgentSpan) { - agentSpan = (AgentSpan) span; + if (span instanceof AgentSpan + && CallDepthThreadLocalMap.incrementCallDepth(HttpServletRequest.class) == 0) { + final AgentSpan agentSpan = (AgentSpan) span; ClassloaderServiceNames.maybeSetToSpan(agentSpan, Thread.currentThread()); + return agentSpan; } - return CallDepthThreadLocalMap.incrementCallDepth(HttpServletRequest.class) == 0; + return null; } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void after( - @Advice.Enter boolean advice, - @Advice.Argument(0) final ServletRequest request, - @Advice.Local("span") AgentSpan span) { - if (advice) { - CallDepthThreadLocalMap.reset(HttpServletRequest.class); - final HttpServletRequest httpServletRequest = - (HttpServletRequest) request; // at this point the cast should be safe - if (span != null - && Config.get().isServletPrincipalEnabled() - && httpServletRequest.getUserPrincipal() != null) { - span.setTag(DDTags.USER_NAME, httpServletRequest.getUserPrincipal().getName()); - } + @Advice.Enter final AgentSpan span, @Advice.Argument(0) final ServletRequest request) { + if (span == null) { + return; + } + + CallDepthThreadLocalMap.reset(HttpServletRequest.class); + final HttpServletRequest httpServletRequest = + (HttpServletRequest) request; // at this point the cast should be safe + if (Config.get().isServletPrincipalEnabled() + && httpServletRequest.getUserPrincipal() != null) { + span.setTag(DDTags.USER_NAME, httpServletRequest.getUserPrincipal().getName()); } } } From a0b2e0ec0c7abf6c070c4b80a03174dc5caa7422 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Mon, 9 Dec 2024 09:13:00 +0100 Subject: [PATCH 05/14] Adjust service name for non legacy tracing --- .../java/datadog/trace/api/naming/v0/MessagingNamingV0.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal-api/src/main/java/datadog/trace/api/naming/v0/MessagingNamingV0.java b/internal-api/src/main/java/datadog/trace/api/naming/v0/MessagingNamingV0.java index fe4b3bef3c3..d8b2991ed9b 100644 --- a/internal-api/src/main/java/datadog/trace/api/naming/v0/MessagingNamingV0.java +++ b/internal-api/src/main/java/datadog/trace/api/naming/v0/MessagingNamingV0.java @@ -1,6 +1,7 @@ package datadog.trace.api.naming.v0; import datadog.trace.api.Config; +import datadog.trace.api.naming.ClassloaderServiceNames; import datadog.trace.api.naming.NamingSchema; import datadog.trace.api.remoteconfig.ServiceNameCollector; import javax.annotation.Nonnull; @@ -46,6 +47,11 @@ public String inboundService(@Nonnull final String messagingSystem, boolean useL ServiceNameCollector.get().addService(messagingSystem); return messagingSystem; } else { + final String contextual = ClassloaderServiceNames.maybeGetForThread(Thread.currentThread()); + if (contextual != null) { + ServiceNameCollector.get().addService(contextual); + return contextual; + } return Config.get().getServiceName(); } } else { From 12df126208029c079becdb1580a7ff9dfe1511ad Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Mon, 9 Dec 2024 15:49:18 +0100 Subject: [PATCH 06/14] Add wildfly ejb test --- dd-smoke-tests/wildfly/build.gradle | 11 ++-- dd-smoke-tests/wildfly/spring-ear/.gitignore | 1 + .../wildfly/spring-ear/war/build.gradle | 2 + .../war/src/main/java/com/example/Common.java | 8 +++ .../java/com/example/ejb/ScheduledEjb.java | 23 ++++++++ .../com/example/hello/HelloController.java | 21 +++++++ .../war/src/main/webapp/WEB-INF/beans.xml | 8 +++ .../war/src/main/webapp/WEB-INF/web.xml | 1 + .../datadog/smoketest/WildflySmokeTest.groovy | 55 ++++++++++++++----- 9 files changed, 112 insertions(+), 18 deletions(-) create mode 100644 dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/Common.java create mode 100644 dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/ejb/ScheduledEjb.java create mode 100644 dd-smoke-tests/wildfly/spring-ear/war/src/main/webapp/WEB-INF/beans.xml diff --git a/dd-smoke-tests/wildfly/build.gradle b/dd-smoke-tests/wildfly/build.gradle index 970fac8aae2..9fe2f1ae7a7 100644 --- a/dd-smoke-tests/wildfly/build.gradle +++ b/dd-smoke-tests/wildfly/build.gradle @@ -1,6 +1,7 @@ ext { serverName = 'wildfly' - serverModule = 'servlet' + //serverModule = 'servlet' + serverModule = 'wildfly' serverVersion = '15.0.0.Final' serverExtension = 'zip' } @@ -9,7 +10,9 @@ repositories { ivy { url 'https://download.jboss.org/' patternLayout { - artifact '/[organisation]/[revision]/[module]/[organisation]-[module]-[revision].[ext]' + // artifact '/[organisation]/[revision]/[module]/[organisation]-[module]-[revision].[ext]' + // we download the full EE profile and not the servlet minimal one + artifact '/[organisation]/[revision]/[organisation]-[revision].[ext]' metadataSources { artifact() } @@ -80,12 +83,12 @@ spotless { } } -def wildflyDir="${buildDir}/${serverName}-${serverModule}-${serverVersion}" +def wildflyDir="${buildDir}/${serverName}-${serverVersion}" tasks.register("unzip", Copy) { dependsOn tasks.earBuild mustRunAfter tasks.compileTestGroovy - def zipFileNamePrefix = "servlet" + def zipFileNamePrefix = "wildfly" def zipPath = project.configurations.serverFile.find { it.name.startsWith(zipFileNamePrefix) } diff --git a/dd-smoke-tests/wildfly/spring-ear/.gitignore b/dd-smoke-tests/wildfly/spring-ear/.gitignore index 1ec2c548b11..72d30a335be 100644 --- a/dd-smoke-tests/wildfly/spring-ear/.gitignore +++ b/dd-smoke-tests/wildfly/spring-ear/.gitignore @@ -1,5 +1,6 @@ # Ignore all project specific gradle directories/files .gradle +.idea gradle build gradlew diff --git a/dd-smoke-tests/wildfly/spring-ear/war/build.gradle b/dd-smoke-tests/wildfly/spring-ear/war/build.gradle index eb58e0085e6..2ebbfebc77b 100644 --- a/dd-smoke-tests/wildfly/spring-ear/war/build.gradle +++ b/dd-smoke-tests/wildfly/spring-ear/war/build.gradle @@ -7,4 +7,6 @@ repositories { dependencies { compileOnly 'org.springframework:spring-webmvc:5.3.0' + compileOnly group: 'javax', name: 'javaee-api', version: '8.0.1' + implementation group: 'com.datadoghq', name: 'dd-trace-api', version: '1.43.0' } diff --git a/dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/Common.java b/dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/Common.java new file mode 100644 index 00000000000..42cd6cc1ff7 --- /dev/null +++ b/dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/Common.java @@ -0,0 +1,8 @@ +package com.example; + +import java.util.concurrent.atomic.AtomicBoolean; + +public class Common { + // for the sake of this example it avoids boilerplate ton inject an ejb into a spring context + public static final AtomicBoolean ENABLED = new AtomicBoolean(false); +} diff --git a/dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/ejb/ScheduledEjb.java b/dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/ejb/ScheduledEjb.java new file mode 100644 index 00000000000..0550a4e3b1e --- /dev/null +++ b/dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/ejb/ScheduledEjb.java @@ -0,0 +1,23 @@ +package com.example.ejb; + +import static com.example.Common.ENABLED; + +import datadog.trace.api.Trace; +import javax.ejb.Schedule; +import javax.ejb.Stateless; + +@Stateless +public class ScheduledEjb { + + @Schedule(second = "*/1", minute = "*", hour = "*") + public void runIt() { + if (ENABLED.getAndSet(false)) { + generateSomeTrace(); + } + } + + @Trace + private void generateSomeTrace() { + // empty + } +} diff --git a/dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/hello/HelloController.java b/dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/hello/HelloController.java index 891150cc10d..2592edc3ec8 100644 --- a/dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/hello/HelloController.java +++ b/dd-smoke-tests/wildfly/spring-ear/war/src/main/java/com/example/hello/HelloController.java @@ -1,5 +1,9 @@ package com.example.hello; +import static com.example.Common.ENABLED; + +import java.util.concurrent.CompletableFuture; +import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; @@ -10,4 +14,21 @@ public class HelloController { public String hello() { return "hello world"; } + + @RequestMapping("/enableScheduling") + public CompletableFuture> enableScheduling() { + ENABLED.set(true); + return CompletableFuture.supplyAsync( + () -> { + while (!ENABLED.get()) { + try { + Thread.sleep(200); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + break; + } + } + return ResponseEntity.ok().build(); + }); + } } diff --git a/dd-smoke-tests/wildfly/spring-ear/war/src/main/webapp/WEB-INF/beans.xml b/dd-smoke-tests/wildfly/spring-ear/war/src/main/webapp/WEB-INF/beans.xml new file mode 100644 index 00000000000..f0ba505ab81 --- /dev/null +++ b/dd-smoke-tests/wildfly/spring-ear/war/src/main/webapp/WEB-INF/beans.xml @@ -0,0 +1,8 @@ + + + + diff --git a/dd-smoke-tests/wildfly/spring-ear/war/src/main/webapp/WEB-INF/web.xml b/dd-smoke-tests/wildfly/spring-ear/war/src/main/webapp/WEB-INF/web.xml index 36f2c845f1a..9d1fd42e198 100644 --- a/dd-smoke-tests/wildfly/spring-ear/war/src/main/webapp/WEB-INF/web.xml +++ b/dd-smoke-tests/wildfly/spring-ear/war/src/main/webapp/WEB-INF/web.xml @@ -8,6 +8,7 @@ dispatcher org.springframework.web.servlet.DispatcherServlet + true 1 diff --git a/dd-smoke-tests/wildfly/src/test/groovy/datadog/smoketest/WildflySmokeTest.groovy b/dd-smoke-tests/wildfly/src/test/groovy/datadog/smoketest/WildflySmokeTest.groovy index 5da6a415260..f6a3158c2f8 100644 --- a/dd-smoke-tests/wildfly/src/test/groovy/datadog/smoketest/WildflySmokeTest.groovy +++ b/dd-smoke-tests/wildfly/src/test/groovy/datadog/smoketest/WildflySmokeTest.groovy @@ -1,11 +1,13 @@ package datadog.smoketest +import datadog.trace.agent.test.utils.OkHttpUtils import datadog.trace.agent.test.utils.PortUtils -import datadog.trace.test.util.Flaky import okhttp3.Request import spock.lang.Shared +import spock.util.concurrent.PollingConditions + +import java.util.concurrent.atomic.AtomicInteger -@Flaky class WildflySmokeTest extends AbstractServerSmokeTest { @Shared @@ -24,12 +26,42 @@ class WildflySmokeTest extends AbstractServerSmokeTest { *defaultJavaProperties, "-Djboss.http.port=${httpPort}", "-Djboss.https.port=${httpsPort}", - "-Djboss.management.http.port=${managementPort}" + "-Djboss.management.http.port=${managementPort}", + "-Ddd.trace.experimental.jee.split-by-deployment=true", + "-Ddd.writer.type=MultiWriter:TraceStructureWriter:${output.getAbsolutePath()}:includeService,DDAgentWriter", ] - processBuilder.environment().put("JAVA_OPTS", javaOpts.collect({ it.replace(' ', '\\ ')}).join(' ')) + processBuilder.environment().put("JAVA_OPTS", javaOpts.collect({ it.replace(' ', '\\ ') }).join(' ')) return processBuilder } + @Override + File createTemporaryFile() { + def ret = File.createTempFile("trace-structure-docs", "out") + ret + } + + @Override + def inferServiceName() { + // do not set DD_SERVICE + false + } + + @Override + protected boolean isAcceptable(int processIndex, Map traceCounts) { + def hasServletRequestTraces = traceCounts.find { it.getKey() == "[war:servlet.request[war:spring.handler]]" }?.getValue()?.get() == 201 + def hasScheduledEjbTrace = traceCounts.find { it.getKey() == "[war:trace.annotation]" }?.getValue()?.get() == 1 + assert hasScheduledEjbTrace && hasServletRequestTraces: "Encountered traces: " + traceCounts + return true + } + + + def setupSpec() { + //wait for the deployment + new PollingConditions(timeout: 300, delay: 2).eventually { + assert OkHttpUtils.client().newCall(new Request.Builder().url("http://localhost:$httpPort/war/hello").build()).execute().code() == 200 + } + } + def cleanupSpec() { ProcessBuilder processBuilder = new ProcessBuilder( "${wildflyDirectory}/bin/jboss-cli.sh", @@ -41,9 +73,9 @@ class WildflySmokeTest extends AbstractServerSmokeTest { process.waitFor() } - def "default home page #n th time"() { + def "spring controller #n th time"() { setup: - String url = "http://localhost:$httpPort/" + String url = "http://localhost:$httpPort/war/hello" def request = new Request.Builder().url(url).get().build() when: @@ -52,26 +84,21 @@ class WildflySmokeTest extends AbstractServerSmokeTest { then: def responseBodyStr = response.body().string() responseBodyStr != null - responseBodyStr.contains("Your WildFly instance is running.") - response.body().contentType().toString().contains("text/html") + responseBodyStr.contentEquals("hello world") response.code() == 200 - where: n << (1..200) } - def "spring context loaded successfully"() { + def "scheduled ejb has right service name"() { setup: - String url = "http://localhost:$httpPort/war/hello" + String url = "http://localhost:$httpPort/war/enableScheduling" def request = new Request.Builder().url(url).get().build() when: def response = client.newCall(request).execute() then: - def responseBodyStr = response.body().string() - responseBodyStr != null - responseBodyStr.contentEquals("hello world") response.code() == 200 } } From 5765d302528fdb1a8153e843d457caa504e5215d Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Tue, 10 Dec 2024 11:12:24 +0100 Subject: [PATCH 07/14] limit max java verson to 11 for wildfly 15 --- dd-smoke-tests/wildfly/build.gradle | 1 + 1 file changed, 1 insertion(+) diff --git a/dd-smoke-tests/wildfly/build.gradle b/dd-smoke-tests/wildfly/build.gradle index 9fe2f1ae7a7..d10e2ed4e73 100644 --- a/dd-smoke-tests/wildfly/build.gradle +++ b/dd-smoke-tests/wildfly/build.gradle @@ -4,6 +4,7 @@ ext { serverModule = 'wildfly' serverVersion = '15.0.0.Final' serverExtension = 'zip' + maxJavaVersionForTests = JavaVersion.VERSION_11 } repositories { From 4b43be2a72f3cb6bd2a9b6c71f550dbcc38b88b7 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Thu, 12 Dec 2024 09:47:23 +0100 Subject: [PATCH 08/14] renaming --- .../instrumentation/jbossmodules/ModuleInstrumentation.java | 4 ++-- .../trace/instrumentation/jbossmodules/ModuleNameHelper.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java index fb664477e2f..c991dcea65b 100644 --- a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java +++ b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java @@ -1,7 +1,7 @@ package datadog.trace.instrumentation.jbossmodules; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; -import static datadog.trace.instrumentation.jbossmodules.ModuleNameHelper.STRIPPER; +import static datadog.trace.instrumentation.jbossmodules.ModuleNameHelper.NAME_EXTRACTOR; import static net.bytebuddy.matcher.ElementMatchers.isConstructor; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; @@ -163,7 +163,7 @@ public static void onExit( public static class CaptureModuleNameAdvice { @Advice.OnMethodExit(suppress = Throwable.class) public static void afterConstruct(@Advice.This final Module module) { - ClassloaderServiceNames.addIfMissing(module.getClassLoader(), STRIPPER); + ClassloaderServiceNames.addIfMissing(module.getClassLoader(), NAME_EXTRACTOR); } } } diff --git a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java index 840630fc4ed..f2bb6751c18 100644 --- a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java +++ b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java @@ -11,7 +11,7 @@ private ModuleNameHelper() {} private static final Pattern SUBDEPLOYMENT_MATCH = Pattern.compile("deployment(?>.+\\.ear)?\\.(.+)\\.[j|w]ar"); - public static final Function> STRIPPER = + public static final Function> NAME_EXTRACTOR = classLoader -> { final Matcher matcher = SUBDEPLOYMENT_MATCH.matcher( From c2cb1c408ae2448b6386bb646e49dec7afa61df6 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Thu, 12 Dec 2024 11:57:58 +0100 Subject: [PATCH 09/14] refactor according to the review --- .../jbossmodules/ModuleInstrumentation.java | 7 +++-- .../jbossmodules/ModuleNameHelper.java | 24 +++++++------- .../liberty20/BundleNameHelper.java | 31 +++++++++---------- ...readContextClassloaderInstrumentation.java | 6 +++- .../JakartaServletInstrumentation.java | 5 ++- .../WebappClassLoaderInstrumentation.java | 21 ++++++++++--- .../api/naming/ClassloaderServiceNames.java | 15 +++------ 7 files changed, 58 insertions(+), 51 deletions(-) diff --git a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java index c991dcea65b..420db803bf7 100644 --- a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java +++ b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java @@ -1,7 +1,7 @@ package datadog.trace.instrumentation.jbossmodules; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; -import static datadog.trace.instrumentation.jbossmodules.ModuleNameHelper.NAME_EXTRACTOR; +import static datadog.trace.instrumentation.jbossmodules.ModuleNameHelper.extractDeploymentName; import static net.bytebuddy.matcher.ElementMatchers.isConstructor; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; @@ -163,7 +163,10 @@ public static void onExit( public static class CaptureModuleNameAdvice { @Advice.OnMethodExit(suppress = Throwable.class) public static void afterConstruct(@Advice.This final Module module) { - ClassloaderServiceNames.addIfMissing(module.getClassLoader(), NAME_EXTRACTOR); + final String name = extractDeploymentName(module.getClassLoader()); + if (name != null && !name.isEmpty()) { + ClassloaderServiceNames.addServiceName(module.getClassLoader(), name); + } } } } diff --git a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java index f2bb6751c18..70e1a878e7a 100644 --- a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java +++ b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java @@ -1,9 +1,8 @@ package datadog.trace.instrumentation.jbossmodules; -import java.util.function.Function; -import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; +import javax.annotation.Nonnull; import org.jboss.modules.ModuleClassLoader; public class ModuleNameHelper { @@ -11,15 +10,14 @@ private ModuleNameHelper() {} private static final Pattern SUBDEPLOYMENT_MATCH = Pattern.compile("deployment(?>.+\\.ear)?\\.(.+)\\.[j|w]ar"); - public static final Function> NAME_EXTRACTOR = - classLoader -> { - final Matcher matcher = - SUBDEPLOYMENT_MATCH.matcher( - ((ModuleClassLoader) classLoader).getModule().getIdentifier().getName()); - if (matcher.matches()) { - final String result = matcher.group(1); - return () -> result; - } - return () -> null; - }; + + public static String extractDeploymentName(@Nonnull final ClassLoader classLoader) { + final Matcher matcher = + SUBDEPLOYMENT_MATCH.matcher( + ((ModuleClassLoader) classLoader).getModule().getIdentifier().getName()); + if (matcher.matches()) { + return matcher.group(1); + } + return null; + }; } diff --git a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java index 1b4edc497f0..cb09030a6cc 100644 --- a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java +++ b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java @@ -1,25 +1,22 @@ package datadog.trace.instrumentation.liberty20; import com.ibm.ws.classloading.internal.ThreadContextClassLoader; -import java.util.function.Function; -import java.util.function.Supplier; public class BundleNameHelper { private BundleNameHelper() {} - public static final Function> EXTRACTOR = - classLoader -> { - final String id = ((ThreadContextClassLoader) classLoader).getKey(); - // id is something like :name#somethingelse - final int head = id.indexOf(':'); - if (head < 0) { - return () -> null; - } - final int tail = id.lastIndexOf('#'); - if (tail < 0) { - return () -> null; - } - final String name = id.substring(head + 1, tail); - return () -> name; - }; + public static String extractDeploymentName(final ThreadContextClassLoader classLoader) { + final String id = classLoader.getKey(); + // id is something like :name#somethingelse + final int head = id.indexOf(':'); + if (head < 0) { + return null; + } + final int tail = id.lastIndexOf('#', head); + if (tail < 0) { + return null; + } + final String name = id.substring(head + 1, tail); + return null; + } } diff --git a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java index 53a87035c2e..e91582458dd 100644 --- a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.liberty20; +import static datadog.trace.instrumentation.liberty20.BundleNameHelper.extractDeploymentName; import static net.bytebuddy.matcher.ElementMatchers.isConstructor; import com.google.auto.service.AutoService; @@ -38,7 +39,10 @@ public void methodAdvice(MethodTransformer transformer) { public static class ThreadContextClassloaderAdvice { @Advice.OnMethodExit(suppress = Throwable.class) public static void afterConstruct(@Advice.This ThreadContextClassLoader self) { - ClassloaderServiceNames.addIfMissing(self, BundleNameHelper.EXTRACTOR); + final String name = extractDeploymentName(self); + if (name != null && !name.isEmpty()) { + ClassloaderServiceNames.addServiceName(self, name); + } } } } diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java index 4d8e8e171c6..5503bf5031e 100644 --- a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java @@ -2,6 +2,7 @@ import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.hasSuperType; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; +import static datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator.DD_SPAN_ATTRIBUTE; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; @@ -56,9 +57,7 @@ public static AgentSpan before(@Advice.Argument(0) final ServletRequest request) if (!(request instanceof HttpServletRequest)) { return null; } - Object span = - request.getAttribute( - "datadog.span"); // hardcode to avoid injecting HttpServiceDecorator just for this + Object span = request.getAttribute(DD_SPAN_ATTRIBUTE); if (span instanceof AgentSpan && CallDepthThreadLocalMap.incrementCallDepth(HttpServletRequest.class) == 0) { final AgentSpan agentSpan = (AgentSpan) span; diff --git a/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java b/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java index 694e9c56c6c..be5b4f6d102 100644 --- a/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java @@ -1,12 +1,15 @@ package datadog.trace.instrumentation.tomcat9; -import static net.bytebuddy.matcher.ElementMatchers.isConstructor; +import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; import datadog.trace.api.naming.ClassloaderServiceNames; import net.bytebuddy.asm.Advice; +import org.apache.catalina.Context; +import org.apache.catalina.WebResourceRoot; import org.apache.catalina.loader.WebappClassLoaderBase; @AutoService(InstrumenterModule.class) @@ -30,13 +33,23 @@ public String[] helperClassNames() { @Override public void methodAdvice(MethodTransformer transformer) { - transformer.applyAdvice(isConstructor(), getClass().getName() + "$CaptureWebappNameAdvice"); + transformer.applyAdvice( + isMethod().and(named("setResources")), getClass().getName() + "$CaptureWebappNameAdvice"); } public static class CaptureWebappNameAdvice { @Advice.OnMethodExit(suppress = Throwable.class) - public static void afterConstruct(@Advice.This final WebappClassLoaderBase classLoader) { - ClassloaderServiceNames.addIfMissing(classLoader, ContextNameHelper.ADDER); + public static void onContextAvailable( + @Advice.This final WebappClassLoaderBase classLoader, + @Advice.Argument(0) final WebResourceRoot webResourceRoot) { + // at this moment we have the context set in this classloader, hence its name + final Context context = webResourceRoot.getContext(); + if (context != null) { + final String contextName = context.getBaseName(); + if (contextName != null && !contextName.isEmpty()) { + ClassloaderServiceNames.addServiceName(classLoader, contextName); + } + } } } } diff --git a/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java b/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java index d28c2dde3e2..c6373c5fbe4 100644 --- a/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java +++ b/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java @@ -6,8 +6,6 @@ import datadog.trace.api.remoteconfig.ServiceNameCollector; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import java.util.WeakHashMap; -import java.util.function.Function; -import java.util.function.Supplier; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -16,7 +14,7 @@ private static class Lazy { private static final ClassloaderServiceNames INSTANCE = new ClassloaderServiceNames(); } - private final WeakHashMap> weakCache = new WeakHashMap<>(); + private final WeakHashMap weakCache = new WeakHashMap<>(); private final String inferredServiceName = CapturedEnvironment.get().getProperties().get(GeneralConfig.SERVICE_NAME); private final boolean enabled = @@ -24,21 +22,16 @@ private static class Lazy { private ClassloaderServiceNames() {} - public static void addIfMissing( - @Nonnull ClassLoader classLoader, - @Nonnull Function> adder) { + public static void addServiceName(@Nonnull ClassLoader classLoader, @Nonnull String serviceName) { if (Lazy.INSTANCE.enabled) { - Lazy.INSTANCE.weakCache.computeIfAbsent(classLoader, adder); + Lazy.INSTANCE.weakCache.put(classLoader, serviceName); } } @Nullable public static String maybeGet(@Nonnull ClassLoader classLoader) { if (Lazy.INSTANCE.enabled) { - final Supplier supplier = Lazy.INSTANCE.weakCache.get(classLoader); - if (supplier != null) { - return supplier.get(); - } + return Lazy.INSTANCE.weakCache.get(classLoader); } return null; } From a40b4dcc221ecd07f2b1ae6da71d119b71eb5542 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Thu, 12 Dec 2024 12:04:15 +0100 Subject: [PATCH 10/14] remove helper --- .../instrumentation/tomcat9/ContextNameHelper.java | 14 -------------- .../tomcat9/WebappClassLoaderInstrumentation.java | 7 ------- 2 files changed, 21 deletions(-) delete mode 100644 dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/ContextNameHelper.java diff --git a/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/ContextNameHelper.java b/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/ContextNameHelper.java deleted file mode 100644 index 9daa69c16a3..00000000000 --- a/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/ContextNameHelper.java +++ /dev/null @@ -1,14 +0,0 @@ -package datadog.trace.instrumentation.tomcat9; - -import java.util.function.Function; -import java.util.function.Supplier; -import org.apache.catalina.loader.WebappClassLoaderBase; - -public class ContextNameHelper { - private ContextNameHelper() {} - - public static final Function> ADDER = - // tomcat does not initialize the context name until the context is started and the - // classloader is created too early to cache that value - classLoader -> ((WebappClassLoaderBase) classLoader)::getContextName; -} diff --git a/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java b/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java index be5b4f6d102..17199ce08fc 100644 --- a/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat-classloading-9/src/main/java/datadog/trace/instrumentation/tomcat9/WebappClassLoaderInstrumentation.java @@ -24,13 +24,6 @@ public String instrumentedType() { return "org.apache.catalina.loader.WebappClassLoaderBase"; } - @Override - public String[] helperClassNames() { - return new String[] { - packageName + ".ContextNameHelper", - }; - } - @Override public void methodAdvice(MethodTransformer transformer) { transformer.applyAdvice( From 1bdfabfe1f7024159c05f9756854ccee0a0655b2 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Thu, 12 Dec 2024 12:14:14 +0100 Subject: [PATCH 11/14] avoid casting --- .../instrumentation/jbossmodules/ModuleNameHelper.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java index 70e1a878e7a..d99f2ded925 100644 --- a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java +++ b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleNameHelper.java @@ -11,13 +11,12 @@ private ModuleNameHelper() {} private static final Pattern SUBDEPLOYMENT_MATCH = Pattern.compile("deployment(?>.+\\.ear)?\\.(.+)\\.[j|w]ar"); - public static String extractDeploymentName(@Nonnull final ClassLoader classLoader) { + public static String extractDeploymentName(@Nonnull final ModuleClassLoader classLoader) { final Matcher matcher = - SUBDEPLOYMENT_MATCH.matcher( - ((ModuleClassLoader) classLoader).getModule().getIdentifier().getName()); + SUBDEPLOYMENT_MATCH.matcher(classLoader.getModule().getIdentifier().getName()); if (matcher.matches()) { return matcher.group(1); } return null; - }; + } } From 20d93e329c4406a3bbe1f668ff03327ce5146e80 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Thu, 12 Dec 2024 12:48:08 +0100 Subject: [PATCH 12/14] review --- .../jbossmodules/ModuleInstrumentation.java | 3 +- .../liberty20/BundleNameHelper.java | 3 +- .../LibertyServerInstrumentation.java | 17 ++++---- ...readContextClassloaderInstrumentation.java | 3 +- .../LibertyServerInstrumentation.java | 17 ++++---- .../servlet2/Servlet2Advice.java | 2 +- .../servlet3/Servlet3Advice.java | 2 +- .../servlet3/Servlet3Decorator.java | 5 +-- .../JakartaServletInstrumentation.java | 2 +- .../java/datadog/trace/core/CoreTracer.java | 2 +- .../api/naming/ClassloaderServiceNames.java | 39 ++++++++++++------- .../api/naming/v0/MessagingNamingV0.java | 2 +- 12 files changed, 53 insertions(+), 44 deletions(-) diff --git a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java index 420db803bf7..235675a90f6 100644 --- a/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java +++ b/dd-java-agent/instrumentation/jboss-modules/src/main/java/datadog/trace/instrumentation/jbossmodules/ModuleInstrumentation.java @@ -1,7 +1,6 @@ package datadog.trace.instrumentation.jbossmodules; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; -import static datadog.trace.instrumentation.jbossmodules.ModuleNameHelper.extractDeploymentName; import static net.bytebuddy.matcher.ElementMatchers.isConstructor; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; @@ -163,7 +162,7 @@ public static void onExit( public static class CaptureModuleNameAdvice { @Advice.OnMethodExit(suppress = Throwable.class) public static void afterConstruct(@Advice.This final Module module) { - final String name = extractDeploymentName(module.getClassLoader()); + final String name = ModuleNameHelper.extractDeploymentName(module.getClassLoader()); if (name != null && !name.isEmpty()) { ClassloaderServiceNames.addServiceName(module.getClassLoader(), name); } diff --git a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java index cb09030a6cc..3e489e8f090 100644 --- a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java +++ b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java @@ -16,7 +16,6 @@ public static String extractDeploymentName(final ThreadContextClassLoader classL if (tail < 0) { return null; } - final String name = id.substring(head + 1, tail); - return null; + return id.substring(head + 1, tail); } } diff --git a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java index c75cf7ad5ac..02e4d1e797b 100644 --- a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/LibertyServerInstrumentation.java @@ -16,6 +16,7 @@ import com.ibm.wsspi.webcontainer.webapp.IWebAppDispatcherContext; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; +import datadog.trace.api.Config; import datadog.trace.api.CorrelationIdentifier; import datadog.trace.api.GlobalTracer; import datadog.trace.api.gateway.Flow; @@ -107,13 +108,15 @@ public static class HandleRequestAdvice { request.setAttribute(DD_EXTRACTED_CONTEXT_ATTRIBUTE, extractedContext); final AgentSpan span = DECORATE.startSpan(request, extractedContext); scope = activateSpan(span, true); - final IWebAppDispatcherContext dispatcherContext = request.getWebAppDispatcherContext(); - if (dispatcherContext != null) { - final WebApp webapp = dispatcherContext.getWebApp(); - if (webapp != null) { - final ClassLoader cl = webapp.getClassLoader(); - if (cl != null) { - ClassloaderServiceNames.maybeSetToSpan(span, cl); + if (Config.get().isJeeSplitByDeployment()) { + final IWebAppDispatcherContext dispatcherContext = request.getWebAppDispatcherContext(); + if (dispatcherContext != null) { + final WebApp webapp = dispatcherContext.getWebApp(); + if (webapp != null) { + final ClassLoader cl = webapp.getClassLoader(); + if (cl != null) { + ClassloaderServiceNames.maybeSetToSpan(span, cl); + } } } } diff --git a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java index e91582458dd..3275e2abd45 100644 --- a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/ThreadContextClassloaderInstrumentation.java @@ -1,6 +1,5 @@ package datadog.trace.instrumentation.liberty20; -import static datadog.trace.instrumentation.liberty20.BundleNameHelper.extractDeploymentName; import static net.bytebuddy.matcher.ElementMatchers.isConstructor; import com.google.auto.service.AutoService; @@ -39,7 +38,7 @@ public void methodAdvice(MethodTransformer transformer) { public static class ThreadContextClassloaderAdvice { @Advice.OnMethodExit(suppress = Throwable.class) public static void afterConstruct(@Advice.This ThreadContextClassLoader self) { - final String name = extractDeploymentName(self); + final String name = BundleNameHelper.extractDeploymentName(self); if (name != null && !name.isEmpty()) { ClassloaderServiceNames.addServiceName(self, name); } diff --git a/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java b/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java index bae8f137446..8d0440eef3b 100644 --- a/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty-23/src/main/java/datadog/trace/instrumentation/liberty23/LibertyServerInstrumentation.java @@ -16,6 +16,7 @@ import com.ibm.wsspi.webcontainer.webapp.IWebAppDispatcherContext; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; +import datadog.trace.api.Config; import datadog.trace.api.CorrelationIdentifier; import datadog.trace.api.GlobalTracer; import datadog.trace.api.gateway.Flow; @@ -109,13 +110,15 @@ public static class HandleRequestAdvice { request.setAttribute(DD_EXTRACTED_CONTEXT_ATTRIBUTE, extractedContext); final AgentSpan span = DECORATE.startSpan(request, extractedContext); scope = activateSpan(span, true); - final IWebAppDispatcherContext dispatcherContext = request.getWebAppDispatcherContext(); - if (dispatcherContext != null) { - final WebApp webapp = dispatcherContext.getWebApp(); - if (webapp != null) { - final ClassLoader cl = webapp.getClassLoader(); - if (cl != null) { - ClassloaderServiceNames.maybeSetToSpan(span, cl); + if (Config.get().isJeeSplitByDeployment()) { + final IWebAppDispatcherContext dispatcherContext = request.getWebAppDispatcherContext(); + if (dispatcherContext != null) { + final WebApp webapp = dispatcherContext.getWebApp(); + if (webapp != null) { + final ClassLoader cl = webapp.getClassLoader(); + if (cl != null) { + ClassloaderServiceNames.maybeSetToSpan(span, cl); + } } } } diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java index aa01ee62496..5473d5d7a94 100644 --- a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java @@ -41,7 +41,7 @@ public static boolean onEnter( final boolean hasServletTrace = spanAttr instanceof AgentSpan; if (hasServletTrace) { final AgentSpan span = (AgentSpan) spanAttr; - ClassloaderServiceNames.maybeSetToSpan(span, Thread.currentThread()); + ClassloaderServiceNames.maybeSetToSpan(span); // Tracing might already be applied by the FilterChain or a parent request (forward/include). return false; } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java index 154ac4da1a5..bc8ca7bb645 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java @@ -62,7 +62,7 @@ public static boolean onEnter( final boolean hasServletTrace = spanAttrValue instanceof AgentSpan; if (hasServletTrace) { final AgentSpan span = (AgentSpan) spanAttrValue; - ClassloaderServiceNames.maybeSetToSpan(span, Thread.currentThread()); + ClassloaderServiceNames.maybeSetToSpan(span); // Tracing might already be applied by other instrumentation, // the FilterChain or a parent request (forward/include). return false; diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java index 900a4761da1..9b1f179200e 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java @@ -83,10 +83,7 @@ public AgentSpan onRequest( final HttpServletRequest request, AgentSpan.Context.Extracted context) { assert span != null; - final String eeService = ClassloaderServiceNames.maybeGetForThread(Thread.currentThread()); - if (eeService != null) { - span.setServiceName(eeService); - } + ClassloaderServiceNames.maybeSetToSpan(span); if (request != null) { String contextPath = request.getContextPath(); String servletPath = request.getServletPath(); diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java index 5503bf5031e..01dc7225427 100644 --- a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/JakartaServletInstrumentation.java @@ -61,7 +61,7 @@ public static AgentSpan before(@Advice.Argument(0) final ServletRequest request) if (span instanceof AgentSpan && CallDepthThreadLocalMap.incrementCallDepth(HttpServletRequest.class) == 0) { final AgentSpan agentSpan = (AgentSpan) span; - ClassloaderServiceNames.maybeSetToSpan(agentSpan, Thread.currentThread()); + ClassloaderServiceNames.maybeSetToSpan(agentSpan); return agentSpan; } return null; diff --git a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java index 43ad02677c2..7d93d13c2a6 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java @@ -1604,7 +1604,7 @@ private DDSpanContext buildSpanContext() { if (serviceName == null && parentServiceName == null) { // in this case we have a local root without service name. We can try to see if we can find // one from the thread context classloader - serviceName = ClassloaderServiceNames.maybeGetForThread(Thread.currentThread()); + serviceName = ClassloaderServiceNames.maybeGetForCurrentThread(); } if (serviceName == null) { // it could be on the initial snapshot but may be overridden to null and service name diff --git a/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java b/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java index c6373c5fbe4..c9d73f7df1d 100644 --- a/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java +++ b/internal-api/src/main/java/datadog/trace/api/naming/ClassloaderServiceNames.java @@ -10,6 +10,9 @@ import javax.annotation.Nullable; public class ClassloaderServiceNames { + private static final boolean ENABLED = + Config.get().isJeeSplitByDeployment() && !Config.get().isServiceNameSetByUser(); + private static class Lazy { private static final ClassloaderServiceNames INSTANCE = new ClassloaderServiceNames(); } @@ -17,51 +20,57 @@ private static class Lazy { private final WeakHashMap weakCache = new WeakHashMap<>(); private final String inferredServiceName = CapturedEnvironment.get().getProperties().get(GeneralConfig.SERVICE_NAME); - private final boolean enabled = - Config.get().isJeeSplitByDeployment() && !Config.get().isServiceNameSetByUser(); private ClassloaderServiceNames() {} public static void addServiceName(@Nonnull ClassLoader classLoader, @Nonnull String serviceName) { - if (Lazy.INSTANCE.enabled) { + if (ENABLED) { Lazy.INSTANCE.weakCache.put(classLoader, serviceName); } } @Nullable public static String maybeGet(@Nonnull ClassLoader classLoader) { - if (Lazy.INSTANCE.enabled) { + if (ENABLED) { return Lazy.INSTANCE.weakCache.get(classLoader); } return null; } + /** + * Fetches the service name linked to the current thread's context classloader. + * + * @return a nullable service name. + */ @Nullable - public static String maybeGetForThread(@Nonnull final Thread thread) { - return maybeGet(thread.getContextClassLoader()); + public static String maybeGetForCurrentThread() { + return maybeGet(Thread.currentThread().getContextClassLoader()); } - public static boolean maybeSetToSpan( - @Nonnull final AgentSpan span, @Nonnull final Thread thread) { - return maybeSetToSpan(span, thread.getContextClassLoader()); + /** + * Sets the service name to the provided spans according to the service name linked to the current + * thread's classloader. + * + * @param span a nonnull span + */ + public static void maybeSetToSpan(@Nonnull final AgentSpan span) { + maybeSetToSpan(span, Thread.currentThread().getContextClassLoader()); } - public static boolean maybeSetToSpan( + public static void maybeSetToSpan( @Nonnull final AgentSpan span, @Nonnull final ClassLoader classLoader) { - if (!Lazy.INSTANCE.enabled) { - return false; + if (!ENABLED) { + return; } final String currentServiceName = span.getServiceName(); if (currentServiceName != null && !currentServiceName.equals(Lazy.INSTANCE.inferredServiceName)) { - return false; + return; } final String service = maybeGet(classLoader); if (service != null) { span.setServiceName(service); ServiceNameCollector.get().addService(service); - return true; } - return false; } } diff --git a/internal-api/src/main/java/datadog/trace/api/naming/v0/MessagingNamingV0.java b/internal-api/src/main/java/datadog/trace/api/naming/v0/MessagingNamingV0.java index d8b2991ed9b..89be91e3d7e 100644 --- a/internal-api/src/main/java/datadog/trace/api/naming/v0/MessagingNamingV0.java +++ b/internal-api/src/main/java/datadog/trace/api/naming/v0/MessagingNamingV0.java @@ -47,7 +47,7 @@ public String inboundService(@Nonnull final String messagingSystem, boolean useL ServiceNameCollector.get().addService(messagingSystem); return messagingSystem; } else { - final String contextual = ClassloaderServiceNames.maybeGetForThread(Thread.currentThread()); + final String contextual = ClassloaderServiceNames.maybeGetForCurrentThread(); if (contextual != null) { ServiceNameCollector.get().addService(contextual); return contextual; From 0559bdb730788ac6bd76be5b02338a65ec99d993 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Thu, 12 Dec 2024 13:29:29 +0100 Subject: [PATCH 13/14] revert lastindexof --- .../trace/instrumentation/liberty20/BundleNameHelper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java index 3e489e8f090..f00e285cc54 100644 --- a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java +++ b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java @@ -12,7 +12,7 @@ public static String extractDeploymentName(final ThreadContextClassLoader classL if (head < 0) { return null; } - final int tail = id.lastIndexOf('#', head); + final int tail = id.lastIndexOf('#'); if (tail < 0) { return null; } From 933c7fa836db9e0280ed53769342680dce7cb061 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Thu, 12 Dec 2024 13:54:48 +0100 Subject: [PATCH 14/14] enforce checks --- .../trace/instrumentation/liberty20/BundleNameHelper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java index f00e285cc54..6acbed6647b 100644 --- a/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java +++ b/dd-java-agent/instrumentation/liberty-20/src/main/java/datadog/trace/instrumentation/liberty20/BundleNameHelper.java @@ -13,7 +13,7 @@ public static String extractDeploymentName(final ThreadContextClassLoader classL return null; } final int tail = id.lastIndexOf('#'); - if (tail < 0) { + if (tail <= head) { return null; } return id.substring(head + 1, tail);