diff --git a/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/BadAdvice.java b/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/BadAdvice.java index 3bc0d08f3c6b..a150caadebe2 100644 --- a/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/BadAdvice.java +++ b/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/BadAdvice.java @@ -11,8 +11,7 @@ public class BadAdvice { @Advice.OnMethodExit(suppress = Throwable.class) - public static void throwAnException(@Advice.Return(readOnly = false) boolean returnVal) { - returnVal = true; + public static boolean throwAnException() { throw new IllegalStateException("Test Exception"); } diff --git a/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/ExceptionHandlerTest.java b/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/ExceptionHandlerTest.java index 92dd4046ba69..2e0489769ade 100644 --- a/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/ExceptionHandlerTest.java +++ b/javaagent-tooling/src/testExceptionHandler/java/io/opentelemetry/javaagent/tooling/bytebuddy/ExceptionHandlerTest.java @@ -20,6 +20,7 @@ import net.bytebuddy.agent.ByteBuddyAgent; import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.agent.builder.ResettableClassFileTransformer; +import net.bytebuddy.asm.Advice; import net.bytebuddy.dynamic.ClassFileLocator; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; @@ -34,20 +35,24 @@ class ExceptionHandlerTest { @BeforeAll static void setUp() { + Advice.WithCustomMapping customMapping = + Advice.withCustomMapping() + // required for AssignReturned annotation and throwable suppression + .with(new Advice.AssignReturned.Factory().withSuppressed(Throwable.class)); AgentBuilder builder = new AgentBuilder.Default() .disableClassFormatChanges() .with(AgentBuilder.RedefinitionStrategy.RETRANSFORMATION) .type(named(ExceptionHandlerTest.class.getName() + "$SomeClass")) .transform( - new AgentBuilder.Transformer.ForAdvice() + new AgentBuilder.Transformer.ForAdvice(customMapping) .with( new AgentBuilder.LocationStrategy.Simple( ClassFileLocator.ForClassLoader.of(BadAdvice.class.getClassLoader()))) .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler()) .advice(isMethod().and(named("isInstrumented")), BadAdvice.class.getName())) .transform( - new AgentBuilder.Transformer.ForAdvice() + new AgentBuilder.Transformer.ForAdvice(customMapping) .with( new AgentBuilder.LocationStrategy.Simple( ClassFileLocator.ForClassLoader.of(BadAdvice.class.getClassLoader()))) @@ -80,7 +85,10 @@ void exceptionHandlerInvoked() { int initLogEvents = testHandler.getRecords().size(); // Triggers classload and instrumentation - assertThat(SomeClass.isInstrumented()).isTrue(); + assertThat(SomeClass.isInstrumented()) + .describedAs("method return value should be preserved when exception is thrown in advice") + .isFalse(); + assertThat(testHandler.getRecords()) .hasSize(initLogEvents + 1) .last() diff --git a/javaagent-tooling/src/testMissingType/java/io/opentelemetry/javaagent/tooling/bytebuddy/MissingTypeTest.java b/javaagent-tooling/src/testMissingType/java/io/opentelemetry/javaagent/tooling/bytebuddy/MissingTypeTest.java index d86733e56cd8..f9cf847c69e3 100644 --- a/javaagent-tooling/src/testMissingType/java/io/opentelemetry/javaagent/tooling/bytebuddy/MissingTypeTest.java +++ b/javaagent-tooling/src/testMissingType/java/io/opentelemetry/javaagent/tooling/bytebuddy/MissingTypeTest.java @@ -16,6 +16,7 @@ import net.bytebuddy.agent.ByteBuddyAgent; import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.agent.builder.ResettableClassFileTransformer; +import net.bytebuddy.asm.Advice; import net.bytebuddy.dynamic.ClassFileLocator; import net.bytebuddy.dynamic.scaffold.MethodGraph; import net.bytebuddy.utility.JavaModule; @@ -33,6 +34,8 @@ class MissingTypeTest { @BeforeAll static void setUp() { + Advice.WithCustomMapping customMapping = + Advice.withCustomMapping().with(new Advice.AssignReturned.Factory()); AgentBuilder builder = new AgentBuilder.Default( new ByteBuddy().with(MethodGraph.Compiler.ForDeclaredMethods.INSTANCE)) @@ -54,7 +57,7 @@ public void onError( }) .type(named(MissingTypeTest.class.getName() + "$SomeClass")) .transform( - new AgentBuilder.Transformer.ForAdvice() + new AgentBuilder.Transformer.ForAdvice(customMapping) .with( new AgentBuilder.LocationStrategy.Simple( ClassFileLocator.ForClassLoader.of(TestAdvice.class.getClassLoader()))) diff --git a/javaagent-tooling/src/testMissingType/java/io/opentelemetry/javaagent/tooling/bytebuddy/TestAdvice.java b/javaagent-tooling/src/testMissingType/java/io/opentelemetry/javaagent/tooling/bytebuddy/TestAdvice.java index 892f7d46efdf..fe7cc9b5d491 100644 --- a/javaagent-tooling/src/testMissingType/java/io/opentelemetry/javaagent/tooling/bytebuddy/TestAdvice.java +++ b/javaagent-tooling/src/testMissingType/java/io/opentelemetry/javaagent/tooling/bytebuddy/TestAdvice.java @@ -6,12 +6,14 @@ package io.opentelemetry.javaagent.tooling.bytebuddy; import net.bytebuddy.asm.Advice; +import net.bytebuddy.asm.Advice.AssignReturned; @SuppressWarnings("unused") public class TestAdvice { + @AssignReturned.ToReturned @Advice.OnMethodExit - public static void returnTrue(@Advice.Return(readOnly = false) boolean returnVal) { - returnVal = true; + public static boolean returnTrue() { + return true; } } diff --git a/testing-common/integration-tests/src/main/java/context/ContextTestInstrumentation.java b/testing-common/integration-tests/src/main/java/context/ContextTestInstrumentation.java index b733e5102dc5..14e26e8de4d3 100644 --- a/testing-common/integration-tests/src/main/java/context/ContextTestInstrumentation.java +++ b/testing-common/integration-tests/src/main/java/context/ContextTestInstrumentation.java @@ -5,15 +5,17 @@ package context; +import static context.ContextTestSingletons.CONTEXT; +import static context.ContextTestSingletons.INTEGER; +import static context.ContextTestSingletons.INTERFACE_INTEGER; import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; import static net.bytebuddy.matcher.ElementMatchers.named; -import io.opentelemetry.instrumentation.api.util.VirtualField; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import library.KeyClass; -import library.KeyInterface; import net.bytebuddy.asm.Advice; +import net.bytebuddy.asm.Advice.AssignReturned; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -42,40 +44,35 @@ public void transform(TypeTransformer transformer) { @SuppressWarnings("unused") public static class MarkInstrumentedAdvice { + @AssignReturned.ToReturned @Advice.OnMethodExit - public static void methodExit(@Advice.Return(readOnly = false) boolean isInstrumented) { - isInstrumented = true; + public static boolean methodExit() { + return true; } } @SuppressWarnings("unused") public static class StoreAndIncrementApiUsageAdvice { + @AssignReturned.ToReturned @Advice.OnMethodExit - public static void methodExit( - @Advice.This KeyClass thiz, @Advice.Return(readOnly = false) int contextCount) { - - VirtualField virtualField = - VirtualField.find(KeyClass.class, Context.class); - - Context context = virtualField.get(thiz); + public static int methodExit(@Advice.This KeyClass thiz) { + Context context = CONTEXT.get(thiz); if (context == null) { context = new Context(); - virtualField.set(thiz, context); + CONTEXT.set(thiz, context); } - contextCount = ++context.count; + return ++context.count; } } @SuppressWarnings("unused") public static class GetApiUsageAdvice { + @AssignReturned.ToReturned @Advice.OnMethodExit - public static void methodExit( - @Advice.This KeyClass thiz, @Advice.Return(readOnly = false) int contextCount) { - VirtualField virtualField = - VirtualField.find(KeyClass.class, Context.class); - Context context = virtualField.get(thiz); - contextCount = context == null ? 0 : context.count; + public static int methodExit(@Advice.This KeyClass thiz) { + Context context = CONTEXT.get(thiz); + return context == null ? 0 : context.count; } } @@ -83,11 +80,9 @@ public static void methodExit( public static class PutApiUsageAdvice { @Advice.OnMethodExit public static void methodExit(@Advice.This KeyClass thiz, @Advice.Argument(0) int value) { - VirtualField virtualField = - VirtualField.find(KeyClass.class, Context.class); Context context = new Context(); context.count = value; - virtualField.set(thiz, context); + CONTEXT.set(thiz, context); } } @@ -95,9 +90,7 @@ public static void methodExit(@Advice.This KeyClass thiz, @Advice.Argument(0) in public static class RemoveApiUsageAdvice { @Advice.OnMethodExit public static void methodExit(@Advice.This KeyClass thiz) { - VirtualField virtualField = - VirtualField.find(KeyClass.class, Context.class); - virtualField.set(thiz, null); + CONTEXT.set(thiz, null); } } @@ -105,15 +98,10 @@ public static void methodExit(@Advice.This KeyClass thiz) { public static class UseMultipleFieldsAdvice { @Advice.OnMethodExit public static void methodExit(@Advice.This KeyClass thiz) { - VirtualField field1 = VirtualField.find(KeyClass.class, Context.class); - VirtualField field2 = VirtualField.find(KeyClass.class, Integer.class); - VirtualField interfaceField = - VirtualField.find(KeyInterface.class, Integer.class); - - Context context = field1.get(thiz); + Context context = CONTEXT.get(thiz); int count = context == null ? 0 : context.count; - field2.set(thiz, count); - interfaceField.set(thiz, count); + INTEGER.set(thiz, count); + INTERFACE_INTEGER.set(thiz, count); } } } diff --git a/testing-common/integration-tests/src/main/java/context/ContextTestInstrumentationModule.java b/testing-common/integration-tests/src/main/java/context/ContextTestInstrumentationModule.java index a1be8d62cc66..dbb88cea989c 100644 --- a/testing-common/integration-tests/src/main/java/context/ContextTestInstrumentationModule.java +++ b/testing-common/integration-tests/src/main/java/context/ContextTestInstrumentationModule.java @@ -10,21 +10,29 @@ import com.google.auto.service.AutoService; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; +import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule; import java.util.List; @AutoService(InstrumentationModule.class) -public class ContextTestInstrumentationModule extends InstrumentationModule { +public class ContextTestInstrumentationModule extends InstrumentationModule + implements ExperimentalInstrumentationModule { public ContextTestInstrumentationModule() { super("context-test-instrumentation"); } @Override public boolean isHelperClass(String className) { - return className.equals(getClass().getPackage().getName() + ".Context"); + String pkg = getClass().getPackage().getName(); + return className.equals(pkg + ".Context") || className.equals(pkg + ".ContextTestSingletons"); } @Override public List typeInstrumentations() { return singletonList(new ContextTestInstrumentation()); } + + @Override + public boolean isIndyReady() { + return true; + } } diff --git a/testing-common/integration-tests/src/main/java/context/ContextTestSingletons.java b/testing-common/integration-tests/src/main/java/context/ContextTestSingletons.java new file mode 100644 index 000000000000..9c56302adbbc --- /dev/null +++ b/testing-common/integration-tests/src/main/java/context/ContextTestSingletons.java @@ -0,0 +1,22 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package context; + +import io.opentelemetry.instrumentation.api.util.VirtualField; +import library.KeyClass; +import library.KeyInterface; + +public class ContextTestSingletons { + + public static final VirtualField CONTEXT = + VirtualField.find(KeyClass.class, Context.class); + public static final VirtualField INTEGER = + VirtualField.find(KeyClass.class, Integer.class); + public static final VirtualField INTERFACE_INTEGER = + VirtualField.find(KeyInterface.class, Integer.class); + + private ContextTestSingletons() {} +} diff --git a/testing-common/integration-tests/src/main/java/field/VirtualFieldTestInstrumentationModule.java b/testing-common/integration-tests/src/main/java/field/VirtualFieldTestInstrumentationModule.java index 3a81438eb0e7..26980a893591 100644 --- a/testing-common/integration-tests/src/main/java/field/VirtualFieldTestInstrumentationModule.java +++ b/testing-common/integration-tests/src/main/java/field/VirtualFieldTestInstrumentationModule.java @@ -14,6 +14,7 @@ import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import java.util.List; import net.bytebuddy.asm.Advice; +import net.bytebuddy.asm.Advice.AssignReturned; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -50,10 +51,11 @@ public void transform(TypeTransformer transformer) { @SuppressWarnings("unused") public static class TestAdvice { + @AssignReturned.ToReturned @Advice.OnMethodExit - public static void onExit(@Advice.Return(readOnly = false) boolean result) { + public static boolean onExit() { VirtualFieldTestHelper.test(); - result = true; + return true; } } } diff --git a/testing-common/integration-tests/src/main/java/helper/DuplicateHelperInstrumentation.java b/testing-common/integration-tests/src/main/java/helper/DuplicateHelperInstrumentation.java index 8cf4db9008cd..09e4f2f8d6ce 100644 --- a/testing-common/integration-tests/src/main/java/helper/DuplicateHelperInstrumentation.java +++ b/testing-common/integration-tests/src/main/java/helper/DuplicateHelperInstrumentation.java @@ -10,6 +10,7 @@ import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import net.bytebuddy.asm.Advice; +import net.bytebuddy.asm.Advice.AssignReturned; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -26,9 +27,10 @@ public void transform(TypeTransformer transformer) { @SuppressWarnings("unused") public static class TestAdvice { + @AssignReturned.ToReturned @Advice.OnMethodExit(suppress = Throwable.class) - public static void addSuffix(@Advice.Return(readOnly = false) String string) { - string = DuplicateHelper.addSuffix(string, " foo"); + public static String addSuffix(@Advice.Return String string) { + return DuplicateHelper.addSuffix(string, " foo"); } } } diff --git a/testing-common/integration-tests/src/main/java/io/opentelemetry/javaagent/IndyInstrumentationTestModule.java b/testing-common/integration-tests/src/main/java/io/opentelemetry/javaagent/IndyInstrumentationTestModule.java index c6424d8fea65..ad077f6f11cc 100644 --- a/testing-common/integration-tests/src/main/java/io/opentelemetry/javaagent/IndyInstrumentationTestModule.java +++ b/testing-common/integration-tests/src/main/java/io/opentelemetry/javaagent/IndyInstrumentationTestModule.java @@ -176,7 +176,6 @@ public static void onMethodEnter() { throw new RuntimeException("This exception should be suppressed"); } - @Advice.AssignReturned.ToReturned @Advice.OnMethodExit( suppress = Throwable.class, onThrowable = Throwable.class,