Prevent duplicate class inclusion in runtime artifacts#11853
Conversation
The `TracerJavaExtension.addSourceSetFor` added the extra source-set output to the `implementation` configuration. A project dependency on the runtime classpath resolves the producer's `runtimeElements` variant. That variant publishes the project jar and runtime dependencies derived from `implementation`, so consumers (the instrumentation project in particular) received the same classes twice: 1. once from the jar configured with `jar.from(mainForJavaVersionSourceSet.output)`, 2. once from the raw class directory published as a dependency This change keeps the additional source set local to the producer module's _compile_ and _test_ classpaths respectively with `compileOnly` and `testImplementation`. The jar remains the runtime artifact consumed by downstream aggregating projects, and it still contains the additional source-set classes. What was considered: dropping the `jar.from(output)` config, it would remove the duplicate, but it would make the consumed module jar incomplete and leave consuming projects relying on class directories instead of the artifact. This would also a the side effect of making Gradle to track individual classes for up-to-date tracking, which consumes more memory. This keeps the existing "jar contract" and only prevent publishing the additional source-set output through `runtimeElements`. This removes the exception-profiling and instrumentation duplicate exclusions that were needed in the instrumentation `shadowJar`.
The `agent-installer` project added its Java-version-specific source-set outputs to the `runtimeOnly` configuration. A project dependency on the runtime classpath resolves the producer's `runtimeElements` variant. That variant publishes the project jar and runtime dependencies derived from `runtimeOnly`, so consumers (the instrumentation project in particular) received the same classes twice: 1. once from the jar configured with `from sourceSets.main_java11.output` and `from sourceSets.main_java25.output`, 2. once from the raw class directories published as runtime dependencies This change keeps the Java-version-specific source sets local to the producer module's test classpath with `testImplementation`. The jar remains the runtime artifact consumed by downstream aggregating projects, and it still contains the Java 11 and Java 25 source-set classes. Dropping the jar configuration would remove the duplicate, but it would make the consumed `agent-installer` jar incomplete and leave consuming projects relying on class directories instead of the artifact. This keeps the existing "jar contract" and only prevents publishing the Java-version-specific source-set output through `runtimeElements`. This removes the agent-installer duplicate exclusions that were needed in the instrumentation `shadowJar`.
The `jakarta-servlet-5.0` project added the relocated javax-to-jakarta
advice jar to the `implementation` configuration. And a project
dependency on the runtime classpath resolves the producer's
`runtimeElements` variant. That variant publishes the project jar and
its runtime dependencies that are derived from `implementation`, so
consumers (the instrumentation project in particular) received the same
classes twice:
1. once from the module jar configured with
`from zipTree(relocatedJavaxJar.outputs.files.asPath)`,
2. once from the relocated jar published as a runtime dependency
This change keeps the relocated jar local to the producer module's
_compile_ and _test_ classpaths with `compileOnly` and
`testImplementation` respectively.
The module jar remains the runtime artifact consumed by downstream
projects, and it still contains the relocated servlet5 advice classes.
With this, the last duplicate is gone, so drop the remaining
`filesMatching { EXCLUDE }` workaround and refresh the comment.
bric3
added
tag: no release notes
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adcf141c62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Publishing the relocated jar there would expose the same servlet5 advice classes | ||
| // twice to downstream projects, once from the module jar and once from the | ||
| // relocated jar. | ||
| compileOnly files(relocatedJavaxJarFile) |
There was a problem hiding this comment.
In the jakarta-servlet-5.0 project, putting relocatedJavaxJarFile only on compileOnly/testImplementation drops it from sourceSets.main.runtimeClasspath. The muzzle task builds its instrumentation loader from allMainSourceSet.runtimeClasspath in MuzzleTask.createAgentClassPath, and MuzzleVersionScanPlugin then locates every helperClassNames() entry for helper injection. Servlet5RequestBodyInstrumentation lists BufferedReaderWrapper, AbstractServletInputStreamWrapper, and Servlet31InputStreamWrapper, which are produced only by relocatedJavaxJar; when running :dd-java-agent:instrumentation:servlet:jakarta-servlet-5.0:muzzle, those helpers are no longer visible even though they are later unpacked into the jar.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Note this doesn't affect the content of the final build, but does mean that
./gradlew :dd-java-agent:instrumentation:servlet:jakarta-servlet-5.0:muzzle
currently fails - this just needs fixing before merge
There was a problem hiding this comment.
Yes I didn't account muzzle for jakarta-servlet, since it was working for the other instrumentations. I'll look at how to fix that.
There was a problem hiding this comment.
Done in f873f22
I made the muzzle task able to be provided an extra classpath. I evaluated another approach by feeding the relocated jar into the runtimeClasspath, but I think this wrong and could be confusing.
Re-ran jardiff on the jar produced by this commit
$ jardiff -c classdata --include '**/*.class,**/*.classdata' \
../master-273405/dd-java-agent/build/libs/dd-java-agent-1.64.0-SNAPSHOT.jar \
dd-java-agent/dd-java-agent/build/libs/dd-java-agent-1.64.0-SNAPSHOT.jar
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
…path Teach `MuzzleTask` to include an extra classpath (if provided). The goal is to allow it to see the relocated servlet5 jar. It is done by introducing `extraAgentClasspath` on the `MuzzelTask`. Another approach would have been to include the relcated jar on the runtimeClasspath: `sourceSets.main.runtimeClasspath += files(relocatedJavaxJarFile)` However this approach creates confusion ; anything that consume this configuration would see the relocated jar (e.g. during debug).
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
bdu/avoid-duplicate-classes
branch
2 participants
What Does This Do
This pull request resolves issues with duplicate class inclusion in runtime artifacts of various components:
TracerJavaExtension.addSourceSetForare kept local to compile and test classpaths.jakarta-servlet-5.0jar is now excluded fromruntimeElementsand kept local to the producer module's compile and test classpaths.agent-installerproject and ensures its runtime artifacts remain complete.These changes eliminate duplicate classes in consumer projects while maintaining existing artifact contracts.
Note
Now, duplicate classes fail the build of the instrumentation shadow jar.
The basics of this change is to remove the declaration of the additional source set output to the
implementationconfiguration. And instead make the classes available to the more precise configurationcompileOnlyandtestImplementation.The additional classes are still included in the project's jar artifact.
The reason is that when a project like
...:instrumentationconsumes another project as a dependency to be on its classpath, Gradle resolves theruntimeElements_consumable configuration of the project dependency. AndruntimeElementscontains by default the project jar artifact and runtime dependencies that were added to theimplementationconfiguration. Breaking this link solves the class duplication.Motivation
The duplicates being published in runtime dependencies were causing redundant class loading downstream, especially in instrumentation projects, and requiring workarounds in the intrumentation shadow jar. While projects themselves should have proper configuration.
This PR ensures cleaner dependency resolution and eliminates the need for exclusions in shadow jars.
Additional Notes
Follow-up to
Kept the "jar contract", i.e. it's the jar that is being consumed that should have the classes form the extra source sets.
At first, I considered removing the
jar.from(additionalSourceSet.output)config, it would remove the duplicate, but it would make the consumed module jar incomplete and leave consuming projects relying on class directories instead of the artifact. This would also have the side effect of making Gradle to track individual classes for up-to-date tracking, which consumes more memory (the doc is on a related topic, but it would have applied here as well).I ran this command, and it reported no changes in the final jar.
$ jardiff -c classdata --include '**/*.class,**/*.classdata' \ ../master-273405/dd-java-agent/build/libs/dd-java-agent-1.64.0-SNAPSHOT.jar \ dd-java-agent/build/libs/dd-java-agent-1.64.0-SNAPSHOT.jarContributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]