diff --git a/.github/workflows/build-common.yml b/.github/workflows/build-common.yml index 687e3650b886..28c26b257b45 100644 --- a/.github/workflows/build-common.yml +++ b/.github/workflows/build-common.yml @@ -223,7 +223,7 @@ jobs: path: "sboms/*.json" test: - name: test${{ matrix.test-partition }} (${{ matrix.test-java-version }}, ${{ matrix.vm }}) + name: test${{ matrix.test-partition }} (${{ matrix.test-java-version }}, ${{ matrix.vm }}, indy ${{ matrix.test-indy }}) runs-on: ubuntu-latest strategy: matrix: @@ -241,6 +241,9 @@ jobs: - 1 - 2 - 3 + test-indy: + - false + - true exclude: - vm: ${{ inputs.skip-openj9-tests && 'openj9' || '' }} fail-fast: false @@ -307,6 +310,7 @@ jobs: ${{ env.test-tasks }} -PtestJavaVersion=${{ matrix.test-java-version }} -PtestJavaVM=${{ matrix.vm }} + -PtestIndy=${{ matrix.test-indy }} -Porg.gradle.java.installations.paths=${{ steps.setup-test-java.outputs.path }} -Porg.gradle.java.installations.auto-download=false ${{ inputs.no-build-cache && ' --no-build-cache' || '' }} @@ -324,15 +328,22 @@ jobs: with: result-encoding: string script: | - const { data: workflow_run } = await github.rest.actions.listJobsForWorkflowRun({ - owner: context.repo.owner, - repo: context.repo.repo, - run_id: context.runId, - per_page: 100 - }); const matrix = JSON.parse(process.env.matrix); - const job_name = `common / test${ matrix['test-partition'] } (${ matrix['test-java-version'] }, ${ matrix.vm })`; - return workflow_run.jobs.find((job) => job.name === job_name).html_url; + const job_name = `common / test${ matrix['test-partition'] } (${ matrix['test-java-version'] }, ${ matrix.vm }, indy ${ matrix['test-indy'] })`; + + const workflow_jobs_nested = await github.paginate( + github.rest.actions.listJobsForWorkflowRun, + { + owner: context.repo.owner, + repo: context.repo.repo, + run_id: context.runId, + per_page: 100 + }, + (response) => { + return response.data; + }, + ); + return workflow_jobs_nested.flat().find((job) => job.name === job_name).html_url; - name: Flaky test report if: ${{ !cancelled() }} @@ -349,7 +360,7 @@ jobs: if: failure() uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1 with: - name: deadlock-detector-test-${{ matrix.test-java-version }}-${{ matrix.vm }}-${{ matrix.test-partition }} + name: deadlock-detector-test-${{ matrix.test-java-version }}-${{ matrix.vm }}-${{ matrix.test-partition }}-indy-${{ matrix.test-indy }} path: /tmp/deadlock-detector-* if-no-files-found: ignore diff --git a/.github/workflows/build-daily-no-build-cache.yml b/.github/workflows/build-daily-no-build-cache.yml index 50d7a33424b1..1039fb64c520 100644 --- a/.github/workflows/build-daily-no-build-cache.yml +++ b/.github/workflows/build-daily-no-build-cache.yml @@ -24,13 +24,6 @@ jobs: secrets: FLAKY_TEST_REPORTER_ACCESS_KEY: ${{ secrets.FLAKY_TEST_REPORTER_ACCESS_KEY }} - test-indy: - uses: ./.github/workflows/reusable-test-indy.yml - with: - no-build-cache: true - secrets: - FLAKY_TEST_REPORTER_ACCESS_KEY: ${{ secrets.FLAKY_TEST_REPORTER_ACCESS_KEY }} - # muzzle is not included here because it doesn't use gradle cache anyway and so is already covered # by the normal daily build diff --git a/.github/workflows/build-daily.yml b/.github/workflows/build-daily.yml index ddc5c5a12543..0d441610a24c 100644 --- a/.github/workflows/build-daily.yml +++ b/.github/workflows/build-daily.yml @@ -20,11 +20,6 @@ jobs: secrets: FLAKY_TEST_REPORTER_ACCESS_KEY: ${{ secrets.FLAKY_TEST_REPORTER_ACCESS_KEY }} - test-indy: - uses: ./.github/workflows/reusable-test-indy.yml - secrets: - FLAKY_TEST_REPORTER_ACCESS_KEY: ${{ secrets.FLAKY_TEST_REPORTER_ACCESS_KEY }} - muzzle: uses: ./.github/workflows/reusable-muzzle.yml diff --git a/.github/workflows/build-pull-request.yml b/.github/workflows/build-pull-request.yml index 6caad07f7d4f..7f5b943c1b97 100644 --- a/.github/workflows/build-pull-request.yml +++ b/.github/workflows/build-pull-request.yml @@ -29,11 +29,6 @@ jobs: with: cache-read-only: true - test-indy: - uses: ./.github/workflows/reusable-test-indy.yml - with: - cache-read-only: true - test-native: uses: ./.github/workflows/reusable-native-tests.yml with: diff --git a/.github/workflows/reusable-test-indy.yml b/.github/workflows/reusable-test-indy.yml deleted file mode 100644 index 82e619faf96a..000000000000 --- a/.github/workflows/reusable-test-indy.yml +++ /dev/null @@ -1,115 +0,0 @@ -name: Reusable - Test latest deps - -on: - workflow_call: - inputs: - cache-read-only: - type: boolean - required: false - no-build-cache: - type: boolean - required: false - secrets: - FLAKY_TEST_REPORTER_ACCESS_KEY: - required: false - -permissions: - contents: read - -jobs: - test-indy: - name: testIndy${{ matrix.test-partition }} - runs-on: ubuntu-latest - strategy: - matrix: - test-partition: - - 0 - - 1 - - 2 - - 3 - fail-fast: false - steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - - - name: Free disk space - run: .github/scripts/gha-free-disk-space.sh - - - name: Set up JDK for running Gradle - uses: actions/setup-java@3a4f6e1af504cf6a31855fa899c6aa5355ba6c12 # v4.7.0 - with: - distribution: temurin - java-version-file: .java-version - - - name: Increase gradle daemon heap size - run: | - sed -i "s/org.gradle.jvmargs=/org.gradle.jvmargs=-Xmx3g /" gradle.properties - - # vaadin 14 tests fail with node 18 - - name: Set up Node - uses: actions/setup-node@1d0ff469b7ec7b3cb9d8673fde0c81c44821de2a # v4.2.0 - with: - node-version: 16 - - # vaadin tests use pnpm - - name: Cache pnpm modules - uses: actions/cache@d4323d4df104b026a6aa633fdb11d772146be0bf # v4.2.2 - with: - path: ~/.pnpm-store - key: ${{ runner.os }}-test-latest-cache-pnpm-modules - - - name: Setup Gradle - uses: gradle/actions/setup-gradle@94baf225fe0a508e581a564467443d0e2379123b # v4.3.0 - with: - cache-read-only: ${{ inputs.cache-read-only }} - - - name: List tests - run: > - ./gradlew - check -x spotlessCheck - listTestsInPartition - -PtestPartition=${{ matrix.test-partition }} - - - name: Set test tasks - run: | - echo "test-tasks=$(cat test-tasks.txt | xargs echo | sed 's/\n/ /g')" >> $GITHUB_ENV - - - name: Test - run: > - ./gradlew - ${{ env.test-tasks }} - -PtestIndy=true - ${{ inputs.no-build-cache && ' --no-build-cache' || '' }} - - - name: Build scan - if: ${{ !cancelled() && hashFiles('build-scan.txt') != '' }} - run: cat build-scan.txt - - - name: Get current job url - id: jobs - if: ${{ !cancelled() }} - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 - env: - matrix: ${{ toJson(matrix) }} - with: - result-encoding: string - script: | - const { data: workflow_run } = await github.rest.actions.listJobsForWorkflowRun({ - owner: context.repo.owner, - repo: context.repo.repo, - run_id: context.runId, - per_page: 100 - }); - const matrix = JSON.parse(process.env.matrix); - const job_name = `test-indy / testIndy${ matrix['test-partition'] }`; - return workflow_run.jobs.find((job) => job.name === job_name).html_url; - - - name: Flaky test report - if: ${{ !cancelled() }} - env: - FLAKY_TEST_REPORTER_ACCESS_KEY: ${{ secrets.FLAKY_TEST_REPORTER_ACCESS_KEY }} - JOB_URL: ${{ steps.jobs.outputs.result }} - run: | - if [ -s build-scan.txt ]; then - export BUILD_SCAN_URL=$(cat build-scan.txt) - fi - ./gradlew :test-report:reportFlakyTests diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java index f619782f5024..a18778912b6a 100644 --- a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java @@ -6,7 +6,6 @@ package io.opentelemetry.javaagent.instrumentation.internal.classloader; import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.extendsClass; -import static java.util.logging.Level.WARNING; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isProtected; import static net.bytebuddy.matcher.ElementMatchers.isPublic; @@ -17,17 +16,14 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import static net.bytebuddy.matcher.ElementMatchers.takesArguments; -import io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder; import io.opentelemetry.javaagent.bootstrap.CallDepth; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import io.opentelemetry.javaagent.tooling.Constants; -import java.lang.invoke.MethodHandle; -import java.lang.invoke.MethodHandles; -import java.lang.invoke.MethodType; -import java.util.List; -import java.util.logging.Logger; +import io.opentelemetry.javaagent.tooling.Utils; +import io.opentelemetry.javaagent.tooling.bytebuddy.ExceptionHandlers; +import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -53,7 +49,7 @@ public ElementMatcher typeMatcher() { @Override public void transform(TypeTransformer transformer) { - transformer.applyAdviceToMethod( + ElementMatcher.Junction methodMatcher = isMethod() .and(named("loadClass")) .and( @@ -64,37 +60,14 @@ public void transform(TypeTransformer transformer) { .and(takesArgument(0, String.class)) .and(takesArgument(1, boolean.class)))) .and(isPublic().or(isProtected())) - .and(not(isStatic())), - BootDelegationInstrumentation.class.getName() + "$LoadClassAdvice"); - } - - public static class Holder { - - public static final List bootstrapPackagesPrefixes = findBootstrapPackagePrefixes(); - - /** - * We have to make sure that {@link BootstrapPackagePrefixesHolder} is loaded from bootstrap - * class loader. After that we can use in {@link LoadClassAdvice}. - */ - private static List findBootstrapPackagePrefixes() { - try { - Class holderClass = - Class.forName( - "io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder", true, null); - MethodHandle methodHandle = - MethodHandles.publicLookup() - .findStatic( - holderClass, "getBoostrapPackagePrefixes", MethodType.methodType(List.class)); - //noinspection unchecked - return (List) methodHandle.invokeExact(); - } catch (Throwable e) { - Logger.getLogger(Holder.class.getName()) - .log(WARNING, "Unable to load bootstrap package prefixes from the bootstrap CL", e); - return Constants.BOOTSTRAP_PACKAGE_PREFIXES; - } - } - - private Holder() {} + .and(not(isStatic())); + // Inline instrumentation to prevent problems with invokedynamic-recursion + transformer.applyTransformer( + new AgentBuilder.Transformer.ForAdvice() + .include(Utils.getBootstrapProxy(), Utils.getAgentClassLoader()) + .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler()) + .advice( + methodMatcher, BootDelegationInstrumentation.class.getName() + "$LoadClassAdvice")); } @SuppressWarnings("unused") @@ -113,7 +86,7 @@ public static Class onEnter(@Advice.Argument(0) String name) { } try { - for (String prefix : Holder.bootstrapPackagesPrefixes) { + for (String prefix : BootstrapPackagesHelper.bootstrapPackagesPrefixes) { if (name.startsWith(prefix)) { try { return Class.forName(name, false, null); @@ -134,13 +107,12 @@ public static Class onEnter(@Advice.Argument(0) String name) { } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - @Advice.AssignReturned.ToReturned - public static Class onExit( - @Advice.Return Class result, @Advice.Enter Class resultFromBootstrapLoader) { + public static void onExit( + @Advice.Return(readOnly = false) Class result, + @Advice.Enter Class resultFromBootstrapLoader) { if (resultFromBootstrapLoader != null) { - return resultFromBootstrapLoader; + result = resultFromBootstrapLoader; } - return result; } } } diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootstrapPackagesHelper.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootstrapPackagesHelper.java new file mode 100644 index 000000000000..4afe3a683f96 --- /dev/null +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootstrapPackagesHelper.java @@ -0,0 +1,45 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.internal.classloader; + +import static java.util.logging.Level.WARNING; + +import io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder; +import io.opentelemetry.javaagent.tooling.Constants; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; +import java.util.List; +import java.util.logging.Logger; + +public class BootstrapPackagesHelper { + + public static final List bootstrapPackagesPrefixes = findBootstrapPackagePrefixes(); + + /** + * We have to make sure that {@link BootstrapPackagePrefixesHolder} is loaded from bootstrap class + * loader. After that we can use in {@link BootDelegationInstrumentation.LoadClassAdvice}. + */ + private static List findBootstrapPackagePrefixes() { + try { + Class holderClass = + Class.forName( + "io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder", true, null); + MethodHandle methodHandle = + MethodHandles.publicLookup() + .findStatic( + holderClass, "getBoostrapPackagePrefixes", MethodType.methodType(List.class)); + //noinspection unchecked + return (List) methodHandle.invokeExact(); + } catch (Throwable e) { + Logger.getLogger(BootstrapPackagesHelper.class.getName()) + .log(WARNING, "Unable to load bootstrap package prefixes from the bootstrap CL", e); + return Constants.BOOTSTRAP_PACKAGE_PREFIXES; + } + } + + private BootstrapPackagesHelper() {} +} diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentationModule.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentationModule.java index 8707d329b222..441fdc0b8002 100644 --- a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentationModule.java +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentationModule.java @@ -10,11 +10,14 @@ 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 io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties; +import java.util.Arrays; import java.util.List; @AutoService(InstrumentationModule.class) -public class ClassLoaderInstrumentationModule extends InstrumentationModule { +public class ClassLoaderInstrumentationModule extends InstrumentationModule + implements ExperimentalInstrumentationModule { public ClassLoaderInstrumentationModule() { super("internal-class-loader"); } @@ -26,10 +29,15 @@ public boolean defaultEnabled(ConfigProperties config) { } @Override - public boolean isHelperClass(String className) { - // TODO: this can be removed when we drop inlined-advice support - // The advices can directly access this class in the AgentClassLoader with invokedynamic Advice - return className.equals("io.opentelemetry.javaagent.tooling.Constants"); + public List getAdditionalHelperClassNames() { + return Arrays.asList( + "io.opentelemetry.javaagent.instrumentation.internal.classloader.BootstrapPackagesHelper", + "io.opentelemetry.javaagent.tooling.Constants"); + } + + @Override + public List injectedClassNames() { + return getAdditionalHelperClassNames(); } @Override diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/LoadInjectedClassInstrumentation.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/LoadInjectedClassInstrumentation.java index c967eef599d0..fcf37f8a24b6 100644 --- a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/LoadInjectedClassInstrumentation.java +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/LoadInjectedClassInstrumentation.java @@ -18,7 +18,11 @@ import io.opentelemetry.javaagent.bootstrap.InjectedClassHelper; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; +import io.opentelemetry.javaagent.tooling.Utils; +import io.opentelemetry.javaagent.tooling.bytebuddy.ExceptionHandlers; +import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -35,7 +39,7 @@ public ElementMatcher typeMatcher() { @Override public void transform(TypeTransformer transformer) { - transformer.applyAdviceToMethod( + ElementMatcher.Junction methodMatcher = isMethod() .and(named("loadClass")) .and( @@ -46,8 +50,15 @@ public void transform(TypeTransformer transformer) { .and(takesArgument(0, String.class)) .and(takesArgument(1, boolean.class)))) .and(isPublic().or(isProtected())) - .and(not(isStatic())), - LoadInjectedClassInstrumentation.class.getName() + "$LoadClassAdvice"); + .and(not(isStatic())); + // Inline instrumentation to prevent problems with invokedynamic-recursion + transformer.applyTransformer( + new AgentBuilder.Transformer.ForAdvice() + .include(Utils.getBootstrapProxy(), Utils.getAgentClassLoader()) + .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler()) + .advice( + methodMatcher, + LoadInjectedClassInstrumentation.class.getName() + "$LoadClassAdvice")); } @SuppressWarnings("unused") @@ -65,13 +76,11 @@ public static Class onEnter( } @Advice.OnMethodExit(onThrowable = Throwable.class) - @Advice.AssignReturned.ToReturned - public static Class onExit( - @Advice.Return Class result, @Advice.Enter Class loadedClass) { + public static void onExit( + @Advice.Return(readOnly = false) Class result, @Advice.Enter Class loadedClass) { if (loadedClass != null) { - return loadedClass; + result = loadedClass; } - return result; } } }