guanceV1120

.github/CODEOWNERS

@@ -11,16 +11,18 @@ dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentat
 dd-java-agent/agent-bootstrap/src/main/java11/datadog/trace/bootstrap/instrumentation/jfr/ @DataDog/profiling-java
 dd-smoke-tests/profiling-integration-tests/                                                @DataDog/profiling-java
 
-# @ValentinZakharov is not in the @DataDog org :( so add him here
-dd-java-agent/appsec/  @DataDog/appsec-java @ValentinZakharov
+dd-java-agent/appsec/  @DataDog/appsec-java
 
 # @DataDog/ci-app-libraries-java
 internal-api/src/main/java/datadog/trace/api/civisibility/ @DataDog/ci-app-libraries-java
 dd-trace-core/src/main/java/datadog/trace/civisibility/    @DataDog/ci-app-libraries-java
 dd-java-agent/agent-ci-visibility/                         @DataDog/ci-app-libraries-java
 dd-java-agent/instrumentation/junit-4.10/                  @DataDog/ci-app-libraries-java
 dd-java-agent/instrumentation/junit-5.3/                   @DataDog/ci-app-libraries-java
-dd-java-agent/instrumentation/testng-6.4/                  @DataDog/ci-app-libraries-java
+dd-java-agent/instrumentation/testng/                      @DataDog/ci-app-libraries-java
+dd-java-agent/instrumentation/gradle/                      @DataDog/ci-app-libraries-java
+dd-java-agent/instrumentation/maven-3.2.1/                 @DataDog/ci-app-libraries-java
+dd-smoke-tests/gradle/                                     @DataDog/ci-app-libraries-java
 
 # @DataDog/debugger-java (Live Debugger)
 dd-java-agent/agent-debugger/ @DataDog/debugger-java

.gitlab-ci.yml

@@ -217,7 +217,7 @@ tracer-base-image-release:
   rules:
     - if: '$POPULATE_CACHE'
       when: never
-    - if: '$CI_COMMIT_BRANCH == "master" && $CI_COMMIT_TAG =~ /^v1\..*/'
+    - if: '$CI_COMMIT_TAG =~ /^v1\..*/'
       when: on_success
   dependencies:
   - build

build.gradle

@@ -11,19 +11,16 @@ buildscript {
 }
 
 plugins {
-  id 'com.github.ben-manes.versions' version '0.27.0'
-
   id "com.diffplug.spotless" version "6.11.0"
-  id 'com.github.spotbugs' version '4.6.0'
-  id "de.thetaphi.forbiddenapis" version "3.2"
+  id 'com.github.spotbugs' version '5.0.14'
+  id "de.thetaphi.forbiddenapis" version "3.5.1"
 
-  id 'org.unbroken-dome.test-sets' version '4.0.0'
   id 'pl.allegro.tech.build.axion-release' version '1.14.4'
-  id 'io.github.gradle-nexus.publish-plugin' version '1.0.0'
+  id 'io.github.gradle-nexus.publish-plugin' version '1.3.0'
 
   id "com.github.johnrengelman.shadow" version "7.1.2" apply false
-  id "me.champeau.jmh" version "0.6.5" apply false
-  id 'org.gradle.playframework' version '0.12' apply false
+  id "me.champeau.jmh" version "0.7.0" apply false
+  id 'org.gradle.playframework' version '0.13' apply false
   id 'info.solidsoft.pitest' version '1.9.11'  apply false
 }
 
@@ -148,11 +145,25 @@ allprojects {
   }
 }
 
+allprojects { project ->
+  project.ext {
+    activePartition = true
+  }
+  final boolean shouldUseTaskPartitions = project.rootProject.hasProperty("taskPartitionCount") && project.rootProject.hasProperty("taskPartition")
+  if (shouldUseTaskPartitions) {
+    final int taskPartitionCount = project.rootProject.property("taskPartitionCount") as int
+    final int taskPartition = project.rootProject.property("taskPartition") as int
+    final currentTaskPartition = project.path.hashCode() % taskPartitionCount
+    project.setProperty("activePartition", currentTaskPartition == taskPartition)
+  }
+}
+
+
 def testAggregate(String taskName, includePrefixes, excludePrefixes) {
   tasks.register(taskName) { aggTest ->
     subprojects { subproject ->
-      println(subproject.path)
-      if (includePrefixes.any { subproject.path.startsWith(it) } && !excludePrefixes.any { subproject.path.startsWith(it) }) {
+      if (subproject.property("activePartition") && includePrefixes.any { subproject.path.startsWith(it) } && !excludePrefixes.any { subproject.path.startsWith(it) }) {
+        println(subproject.path)
         def testTask = subproject.tasks.findByName("test")
         if (testTask != null) {
           aggTest.dependsOn(testTask)

buildSrc/build.gradle.kts

@@ -31,15 +31,15 @@ dependencies {
   implementation(gradleApi())
   implementation(localGroovy())
 
-  implementation("net.bytebuddy", "byte-buddy-gradle-plugin", "1.12.22")
+  implementation("net.bytebuddy", "byte-buddy-gradle-plugin", "1.14.3")
 
   implementation("org.eclipse.aether", "aether-connector-basic", "1.1.0")
   implementation("org.eclipse.aether", "aether-transport-http", "1.1.0")
   implementation("org.apache.maven", "maven-aether-provider", "3.3.9")
 
   implementation("com.google.guava", "guava", "20.0")
-  implementation("org.ow2.asm", "asm", "9.0")
-  implementation("org.ow2.asm", "asm-tree", "9.0")
+  implementation("org.ow2.asm", "asm", "9.5")
+  implementation("org.ow2.asm", "asm-tree", "9.5")
 
   testImplementation("org.spockframework", "spock-core", "2.0-groovy-3.0")
   testImplementation("org.codehaus.groovy", "groovy-all", "3.0.10")

buildSrc/call-site-instrumentation-plugin/build.gradle.kts

@@ -92,5 +92,5 @@ tasks.build {
 
 tasks.test {
   useJUnitPlatform()
-  enabled = !project.hasProperty("skipBuildSrcTest")
+  enabled = project.hasProperty("runBuildSrcTests")
 }

buildSrc/call-site-instrumentation-plugin/src/main/java/datadog/trace/plugin/csi/impl/AdviceGeneratorImpl.java

@@ -341,7 +341,11 @@ private static void writeStackOperations(final AdviceSpecification advice, final
       }
       String mode = "COPY";
       if (allArgsSpec != null) {
-        mode = advice instanceof AfterSpecification ? "PREPEND_ARRAY" : "APPEND_ARRAY";
+        if (advice instanceof AfterSpecification) {
+          mode = advice.isConstructor() ? "PREPEND_ARRAY_CTOR" : "PREPEND_ARRAY";
+        } else {
+          mode = "APPEND_ARRAY";
+        }
       }
       dupMethod.addArgument(
           new FieldAccessExpr()
@@ -392,15 +396,6 @@ private static void writeAdviceMethodCall(
               .addArgument(new StringLiteralExpr(method.getMethodType().getDescriptor()))
               .addArgument(new BooleanLiteralExpr(false));
       body.addStatement(invokeStatic);
-      if (advice instanceof AfterSpecification && advice.isConstructor()) {
-        // constructors make a DUP before the <init> that we have to discard
-        final MethodCallExpr pop =
-            new MethodCallExpr()
-                .setScope(new NameExpr("handler"))
-                .setName("instruction")
-                .addArgument(opCode("POP"));
-        body.addAndGetStatement(pop);
-      }
     }
   }
 

buildSrc/call-site-instrumentation-plugin/src/main/java/datadog/trace/plugin/csi/impl/CallSiteSpecification.java

@@ -166,7 +166,10 @@ private void validateReturnSpecCompatibility(
       withParameter(
           ReturnSpecification.class,
           (i, spec) -> {
-            final Type rType = pointcut.getMethodType().getReturnType();
+            final Type rType =
+                pointcut.isConstructor()
+                    ? pointcut.getOwner()
+                    : pointcut.getMethodType().getReturnType();
             final Type advice = adviceArgumentTypes[i];
             validateCompatibility(
                 context, rType, advice, ErrorCode.ADVICE_METHOD_PARAM_RETURN_NOT_COMPATIBLE, i);
@@ -185,7 +188,7 @@ private void validateThisSpecCompatibility(
           });
     }
 
-    private void validateAdviceReturnTypeCompatibility(final ValidationContext context) {
+    protected void validateAdviceReturnTypeCompatibility(final ValidationContext context) {
       if (!advice.isVoidReturn()) {
         final Type pointcutType =
             pointcut.isConstructor()
@@ -560,16 +563,16 @@ protected void validateAdvice(@Nonnull final ValidationContext context) {
       if (advice.isVoidReturn()) {
         context.addError(ErrorCode.ADVICE_AFTER_SHOULD_NOT_RETURN_VOID);
       }
-      if (!isStaticPointcut() && !includeThis()) {
-        context.addError(ErrorCode.ADVICE_AFTER_SHOULD_HAVE_THIS);
+      if (findReturn() == null) {
+        context.addError(ErrorCode.ADVICE_AFTER_SHOULD_HAVE_RETURN);
       }
       if (!pointcut.isConstructor()) {
-        if (findReturn() == null) {
-          context.addError(ErrorCode.ADVICE_AFTER_SHOULD_HAVE_RETURN);
+        if (!isStaticPointcut() && !includeThis()) {
+          context.addError(ErrorCode.ADVICE_AFTER_SHOULD_HAVE_THIS);
         }
       } else {
-        if (findReturn() != null) {
-          context.addError(ErrorCode.ADVICE_AFTER_CONSTRUCTOR_SHOULD_NOT_HAVE_RETURN);
+        if (findAllArguments() == null) {
+          context.addError(ErrorCode.ADVICE_AFTER_CONSTRUCTOR_ALL_ARGUMENTS);
         }
       }
       super.validateAdvice(context);

buildSrc/call-site-instrumentation-plugin/src/main/java/datadog/trace/plugin/csi/impl/ext/IastExtension.java

@@ -81,7 +81,9 @@ public void apply(
   private void addTelemetry(final Configuration configuration, final CallSiteResult result)
       throws Exception {
     final Type callSiteClass = result.getSpecification().getClazz();
-    final String callSiteFile = callSiteClass.getClassName().replaceAll("\\.", File.separator);
+    final String fileSeparatorPattern = File.separator.equals("\\") ? "\\\\" : File.separator;
+    final String callSiteFile =
+        callSiteClass.getClassName().replaceAll("\\.", fileSeparatorPattern);
     final TypeResolver resolver = getTypeResolver(configuration);
 
     // new call site with telemetry embedded

buildSrc/call-site-instrumentation-plugin/src/main/java/datadog/trace/plugin/csi/util/ErrorCode.java

@@ -352,14 +352,14 @@ public String apply(final Object[] objects) {
   ADVICE_AFTER_SHOULD_HAVE_RETURN {
     @Override
     public String apply(final Object[] objects) {
-      return "After advice last parameter should be annotated with @Return for non constructors";
+      return "After advice last parameter should be annotated with @Return";
     }
   },
 
-  ADVICE_AFTER_CONSTRUCTOR_SHOULD_NOT_HAVE_RETURN {
+  ADVICE_AFTER_CONSTRUCTOR_ALL_ARGUMENTS {
     @Override
     public String apply(final Object[] objects) {
-      return "After advice should not be annotated with @Return for constructors";
+      return "After advice in constructors should use @AllArguments";
     }
   },
 

buildSrc/call-site-instrumentation-plugin/src/test/groovy/datadog/trace/plugin/csi/impl/AdviceGeneratorTest.groovy

@@ -108,9 +108,9 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
 
   @CallSite
   class AfterAdvice {
-    @CallSite.After('void java.net.URL.<init>(java.lang.String)')
-    static URL after(@CallSite.This final URL url, @CallSite.Argument final String spec) {
-      return url;
+    @CallSite.After('java.lang.String java.lang.String.concat(java.lang.String)')
+    static String after(@CallSite.This final String self, @CallSite.Argument final String param, @CallSite.Return final String result) {
+      return result
     }
   }
 
@@ -136,16 +136,57 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
     interfaces.containsAll(['CallSiteAdvice', 'Pointcut', 'InvokeAdvice', 'HasFlags', 'HasHelpers'])
     final methods = groupMethods(adviceClass)
     getStatements(methods['pointcut']) == ['return this;']
+    getStatements(methods['type']) == ['return "java/lang/String";']
+    getStatements(methods['method']) == ['return "concat";']
+    getStatements(methods['descriptor']) == ['return "(Ljava/lang/String;)Ljava/lang/String;";']
+    getStatements(methods['helperClassNames']) == ['return new String[] { "' + AfterAdvice.name + '" };']
+    getStatements(methods['flags']) == ['return COMPUTE_MAX_STACK;']
+    getStatements(methods['apply']) == [
+      'handler.dupInvoke(owner, descriptor, StackDupMode.COPY);',
+      'handler.method(opcode, owner, name, descriptor, isInterface);',
+      'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$AfterAdvice", "after", "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;", false);',
+    ]
+  }
+
+  @CallSite
+  class AfterAdviceCtor {
+    @CallSite.After('void java.net.URL.<init>(java.lang.String)')
+    static URL after(@CallSite.AllArguments final Object[] args, @CallSite.Return final URL url) {
+      return url
+    }
+  }
+
+  void 'test after advice ctor'() {
+    setup:
+    final spec = buildClassSpecification(AfterAdviceCtor)
+    final generator = buildAdviceGenerator(buildDir)
+
+    when:
+    final result = generator.generate(spec)
+
+    then:
+    assertNoErrors(result)
+    final advice = findAdvice(result, 'after')
+    assertNoErrors(advice)
+    final javaFile = new JavaParser().parse(advice.file).getResult().get()
+    assert javaFile.parsed == Node.Parsedness.PARSED
+    final packageDcl = javaFile.getPackageDeclaration().get()
+    packageDcl.name.asString() == AfterAdvice.package.name
+    final adviceClass = javaFile.getType(0)
+    adviceClass.name.asString().endsWith(AfterAdviceCtor.simpleName + 'After')
+    final interfaces = getImplementedTypes(adviceClass)
+    interfaces.containsAll(['CallSiteAdvice', 'Pointcut', 'InvokeAdvice', 'HasFlags', 'HasHelpers'])
+    final methods = groupMethods(adviceClass)
+    getStatements(methods['pointcut']) == ['return this;']
     getStatements(methods['type']) == ['return "java/net/URL";']
     getStatements(methods['method']) == ['return "<init>";']
     getStatements(methods['descriptor']) == ['return "(Ljava/lang/String;)V";']
-    getStatements(methods['helperClassNames']) == ['return new String[] { "' + AfterAdvice.name + '" };']
+    getStatements(methods['helperClassNames']) == ['return new String[] { "' + AfterAdviceCtor.name + '" };']
     getStatements(methods['flags']) == ['return COMPUTE_MAX_STACK;']
     getStatements(methods['apply']) == [
-      'handler.dupInvoke(owner, descriptor, StackDupMode.COPY);',
+      'handler.dupParameters(descriptor, StackDupMode.PREPEND_ARRAY_CTOR);',
       'handler.method(opcode, owner, name, descriptor, isInterface);',
-      'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$AfterAdvice", "after", "(Ljava/net/URL;Ljava/lang/String;)Ljava/net/URL;", false);',
-      'handler.instruction(Opcodes.POP);'
+      'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$AfterAdviceCtor", "after", "([Ljava/lang/Object;Ljava/net/URL;)Ljava/net/URL;", false);',
     ]
   }
 
@@ -374,9 +415,9 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
 
   @CallSite(minJavaVersion = 9)
   class MinJavaVersionAdvice {
-    @CallSite.After('void java.net.URL.<init>(java.lang.String)')
-    static URL after(@CallSite.This final URL url, @CallSite.Argument final String spec) {
-      return url;
+    @CallSite.After('java.lang.String java.lang.String.concat(java.lang.String)')
+    static String after(@CallSite.This final String self, @CallSite.Argument final String param, @CallSite.Return final String result) {
+      return result
     }
   }
 

buildSrc/call-site-instrumentation-plugin/src/test/groovy/datadog/trace/plugin/csi/impl/BaseCsiPluginTest.groovy

@@ -37,7 +37,8 @@ abstract class BaseCsiPluginTest extends Specification {
 
   protected static File fetchClass(final Class<?> clazz) {
     final folder = Paths.get(clazz.getResource('/').toURI()).resolve('../../')
-    final classFile = clazz.getName().replaceAll('\\.', File.separator) + '.class'
+    final fileSeparatorPattern = File.separator == "\\" ? "\\\\" : File.separator
+    final classFile = clazz.getName().replaceAll('\\.', fileSeparatorPattern) + '.class'
     final groovy = folder.resolve('groovy/test').resolve(classFile)
     if (Files.exists(groovy)) {
       return groovy.toFile()

buildSrc/call-site-instrumentation-plugin/src/test/groovy/datadog/trace/plugin/csi/impl/ext/IastExtensionTest.groovy

@@ -78,7 +78,8 @@ class IastExtensionTest extends BaseCsiPluginTest {
 
     then:
     final String callSiteWithTelemetryClass = IastExtensionCallSite.name + IastExtension.WITH_TELEMETRY_SUFFIX
-    final resultFile = targetFolder.resolve("${callSiteWithTelemetryClass.replaceAll('\\.', File.separator)}.java")
+    final fileSeparatorPattern = File.separator == "\\" ? "\\\\" : File.separator
+    final resultFile = targetFolder.resolve("${callSiteWithTelemetryClass.replaceAll('\\.', fileSeparatorPattern)}.java")
     assert Files.exists(resultFile)
     final callSiteType = parse(resultFile.toFile())
     validateMethod(callSiteType, 'afterGetHeader') { List<String> statements ->
@@ -101,9 +102,9 @@ class IastExtensionTest extends BaseCsiPluginTest {
       }
       validateMethod(adviceType, 'apply') { List<String> statements ->
         assert statements == [
-          '''if (this.telemetry) {
-    IastTelemetryCollector.add(IastMetric.INSTRUMENTED_SOURCE, 1, SourceTypes.REQUEST_HEADER_VALUE);
-}''',
+          'if (this.telemetry) {' + System.lineSeparator() +
+          '    IastTelemetryCollector.add(IastMetric.INSTRUMENTED_SOURCE, 1, SourceTypes.REQUEST_HEADER_VALUE);' + System.lineSeparator() +
+          '}',
           'handler.dupInvoke(owner, descriptor, StackDupMode.COPY);',
           'handler.method(opcode, owner, name, descriptor, isInterface);',
           'handler.method(Opcodes.INVOKESTATIC, this.callSite, "afterGetHeader", "(Ljavax/servlet/http/HttpServletRequest;Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;", false);'
@@ -118,10 +119,10 @@ class IastExtensionTest extends BaseCsiPluginTest {
       validateMethod(adviceType, 'enableTelemetry') { List<String> statements ->
         assert statements == [
           'this.telemetry = true;',
-          """if (enableRuntime) {
-    this.callSite = "${type.internalName}WithTelemetry";
-    this.helperClassNames = new String[] { "${type.className}WithTelemetry" };
-}""",
+          'if (enableRuntime) {' + System.lineSeparator() +
+          '    this.callSite = "' + type.internalName + 'WithTelemetry";' + System.lineSeparator() +
+          '    this.helperClassNames = new String[] { "' + type.className + 'WithTelemetry" };' + System.lineSeparator() +
+          '}',
         ]
       }
     }