Enable Gradle enhanced graph ordering - #12326
Conversation
AlexeyKuznetsov-DD
added
tag: no release notes
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The changes use established repository patterns for dependency exclusions, MockServer setup, and Spring metadata merge. The static review finds no concrete failure.
🤖 Datadog Autotest · Commit c6e34cd · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
🎯 Code Coverage (details) 🔗 Commit SHA: 5da36a4 | Docs | View more details | Give us feedback! |
🟢 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. |
bdb9131 to
88bf0ee
Compare
Avoid Maven Central fallback when proxy is configured Co-authored-by: alexey.kuznetsov <alexey.kuznetsov@datadoghq.com>
6cda9a3 to
5da36a4
Compare
bric3
left a comment
bric3
left a comment
There was a problem hiding this comment.
That description !
I understand this is still draft, but ENHANCED_GRAPH_ORDERING is still a preview, and this is the very first version of Gradle that ships with it. I'd rather hold off a bit before enabling this. Especially with that much changes. Note some comments feels like over narrating the inferrence.
That being said, I think there are interesting change that should land separately, and that should work without the graph ordering toggle.
- The spring boot smoke test convention, looks like the first thing I'll try to land separately. (Also, I'm suggesting a gradle module rename)
- Some dependency ordering changes like with mockserver, should apply fine.
- The liberty change looks interesting, but the need for type hierarchy change is something to watch for. And given it might require IDM approval, I think it's better to extract that.
Then a last PR that just toggles the new graph ordering.
| include(":spring-boot-shadow") | ||
| include(":smoke-test") |
There was a problem hiding this comment.
suggestion: I think that spring-boot-shadow, should be prefixed with smoke-test, like smoke-test-spring-boot-app
Also, maybe the existing smoke-test can be renamed to smoke-test-app (like it's custom plugin).
| // DO NOT BUMP THIS VERSION WITHOUT CHECKING THE JAR CONTENTS. | ||
| // | ||
| // The `-no-dependencies` artifacts relocate most of their dependencies under `shaded_package`, | ||
| // but leave `org.slf4j` unrelocated. That jar sorts ahead of `slf4j-api` on the test runtime | ||
| // classpath, so whichever slf4j API it embeds is the one that gets loaded: | ||
| // * 5.14.0 embeds the slf4j 1.7 API, which matches the slf4j-api version used here, and | ||
| // ships no binding of its own, so logback-classic still binds normally. | ||
| // * 5.15.0 embeds the slf4j 2.0 API plus a | ||
| // `META-INF/services/org.slf4j.spi.SLF4JServiceProvider` pointing at | ||
| // `org.slf4j.jul.JULServiceProvider`. slf4j 2.0 ignores logback 1.2's | ||
| // `org.slf4j.impl.StaticLoggerBinder`, so test logging silently reroutes to JUL. |
There was a problem hiding this comment.
suggestion: I don't think this is useful to stay in the code, maybe in the commit message body.
| public class RequestFinishInstrumentation extends InstrumenterModule.Tracing | ||
| implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { | ||
| implements Instrumenter.ForSingleType, | ||
| Instrumenter.WithTypeStructure, |
There was a problem hiding this comment.
question: Are we sure about that ?
| public class ResponseFinishInstrumentation extends InstrumenterModule.Tracing | ||
| implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { | ||
| implements Instrumenter.ForSingleType, | ||
| Instrumenter.WithTypeStructure, |
There was a problem hiding this comment.
question: Are we sure about that ?
| testImplementation project(':dd-java-agent:appsec:appsec-test-fixtures') | ||
| testRuntimeOnly project(':dd-java-agent:instrumentation:osgi-4.3') | ||
| testRuntimeOnly files(filterLogbackClassic.map { it.destinationDir }) | ||
| // Keep shared classloader naming and multipart hooks under test without duplicating them. |
There was a problem hiding this comment.
nitpick: This looks like inference comment
| // `sofa-rpc-all:5.14.2` brings `netty-all:4.1.44.Final`, a fat jar carrying the classes of | ||
| // every netty module, while `grpc-netty:1.53.0` brings the individual `netty-*:4.1.79.Final` | ||
| // modules. Both supply `io.netty.**` under unrelated coordinates, so Gradle cannot reconcile | ||
| // them and whichever jar sorts first wins class loading. | ||
| // | ||
| // The `sofa-rpc-all:5.14.2` brings `netty-all:4.1.44.Final`, which is a fat jar, | ||
| // while `grpc-netty:1.53.0` brings individual `netty-*:4.1.79.Final` modules. | ||
| // Gradle 9.6.0 introduced a regression in the classpath ordering, as they are | ||
| // working on revamping the dependency traversal. The result is that the `4.1.44` | ||
| // fat jar classes appear earlier and its classes are loaded from it ; this | ||
| // produces a `NoSuchMethodError` at runtime in netty's `AbstractReferenceCountedByteBuf`. | ||
| // Forcing 4.1.79 removes the duplicate rather than reordering it: `netty-all` has shipped zero | ||
| // classes since 4.1.75, so it becomes an empty aggregator that instead pulls in the complete, | ||
| // version-aligned 4.1.79 module set - including the dns/epoll/kqueue/sctp modules that the fat | ||
| // jar was otherwise the only source of. | ||
| // | ||
| // The workaround is to force `netty-all` to `4.1.79` so the fat jar and the individual | ||
| // modules are aligned on the same version. The fix is behind Gradle 9.7's | ||
| // ENHANCED_GRAPH_ORDERING preview, which currently exposes other fat-jar conflicts. | ||
| // See https://github.com/gradle/gradle/issues/38057 for more details. | ||
| // This is deliberately independent of ENHANCED_GRAPH_ORDERING. That preview only changes where | ||
| // the fat jar lands: without it the jar sorts between `netty-buffer` and `netty-common`, so | ||
| // `AbstractReferenceCountedByteBuf` (4.1.79) is paired with `ReferenceCountUpdater` (4.1.44) | ||
| // and fails with a `NoSuchMethodError`; with it the jar sorts ahead of every modular jar, which | ||
| // hides that symptom only because all of netty then silently loads from 4.1.44. Either way the | ||
| // classpath still holds two copies of `io.netty.**`, so do not drop this constraint on the | ||
| // grounds that the preview is enabled. |
There was a problem hiding this comment.
nitpick: I'd humanly reword this comment. In particular if ENHANCED_GRAPH_ORDERING is enabled but that doesn't change the need of constraint (which is very fine, and expressive), then this comment can drop referring to ENHANCED_GRAPH_ORDERING.
There was a problem hiding this comment.
Yep, I'm kind of think now that I will close this PR for now and open couple of small ones that make sense without ENHANCED_GRAPH_ORDERING.
There was a problem hiding this comment.
Let's reopen it when your other changes land
By then maybe Gradle will probably released 9.8 or 9.9.
|
Closing since it is too early to switch to |
3 participants
What Does This Do
ENHANCED_GRAPH_ORDERINGfeature preview.mockserver-netty-no-dependencies:5.14.0and a minimal local JUnit extension, avoiding embedded JUnit and incompatible SLF4J classes.build-logic/spring-boot-shadowconvention that merges service and Spring discovery metadata.javax.transactionAPI, and refreshes ordering-sensitive dependency locks.javaxorjakartageneration.Motivation
Gradle 9.6 changed dependency traversal and could place an older fat JAR before newer modular dependencies. Gradle 9.7 addresses that traversal behavior behind
ENHANCED_GRAPH_ORDERING; the preview becomes the default in Gradle 10.Enabling the preview also reveals classpaths that previously depended on incidental traversal order. Some are normal dependency conflicts that can be made deterministic through exclusions or direct dependencies. Others involve classes or resources hidden inside fat/no-dependencies JARs, which Gradle cannot reconcile by module version because they belong to different coordinates.
SofaRPC is in the latter category:
sofa-rpc-all:5.14.2brings the class-carryingnetty-all:4.1.44.Final, while gRPC brings individual Netty 4.1.79 modules. Enhanced ordering changes which copy ofio.netty.**loads first, but does not eliminate the duplicate classes. Retaining the constraint onnetty-all:4.1.79.Finalreplaces the fat JAR with Netty's class-free aggregator and an aligned modular dependency set.The ordering change also exposed two structural issues. Spring Boot fat JARs normally need discovery metadata from every dependency merged rather than retaining an arbitrary duplicate, and Liberty 20 and Liberty 23 reuse IBM implementation class names even though their servlet APIs use different namespaces. The Spring convention makes metadata handling consistent for normal fixtures, while the Liberty common module removes an unnecessary cross-generation test dependency and namespace matchers prevent incompatible servlet advice from being applied.
Additional Notes
This is stacked on #12323 and should be reviewed and merged after it.
The SofaRPC regression can otherwise combine
AbstractReferenceCountedByteBuffrom Netty 4.1.79 withReferenceCountUpdaterfrom the 4.1.44 fat JAR and fail with aNoSuchMethodError. With enhanced ordering, the old fat JAR may instead win for all Netty classes, hiding that particular linkage error while silently testing the wrong Netty version. The retained constraint removes both outcomes.MockServer's previous
mockserver-junit-jupiter-no-dependenciesartifact embeds an older JUnit Platform under the original packages. Enhanced ordering allowed those classes to shadow JUnit Platform 1.14.1, producing:The replacement uses
mockserver-netty-no-dependencies, which contains the server implementation without embedded JUnit classes, plus a small extension implementing only the lifecycle and parameter injection used by the HTTP test fixtures. Version 5.14.0 is intentional: its unrelocated SLF4J API matches this module's SLF4J 1.7 line and it ships no logging provider, whereas 5.15.0 embeds SLF4J 2.0 and a JUL provider.The OpenAI test classpath contains both Datadog's forked OkHttp/Okio and OpenAI's newer upstream versions under identical Java packages. Runtime-only exclusions preserve the tested OpenAI versions and avoid failures such as:
The Spring Boot Shadow convention merges
META-INF/services, Spring handlers, schemas, tooling, and factories. It lives in a separate Java 17 build-logic module because Shadow 9's plugin API targets Java 17, while other build-logic modules still target Java 8.appsec/springbootand the legacy Spring Boot 1.5 IAST fixture intentionally keep raw Shadow behavior plus a directspring-boot-autoconfiguredependency. Merging their complete factory sets changes established request/error behavior: the AppSec fixture stops observing expected blocking, and the Java 8/11 IAST fixture reportsRIPEMD128 MessageDigest not available. The direct dependency makes their historical first-file selection deterministic under enhanced ordering.The Quartz
version40Testsuite is pinned to Quartz 2.4.0 so it remains distinct fromlatestDepTest. Quartz 2.4 usesjavax.transaction.UserTransaction; Quartz 2.5 and later use Jakarta Transactions.Liberty 23 previously loaded the complete Liberty 20 instrumentation module in tests to reuse deployment classloader naming. The shared module now contains the namespace-neutral classloader naming and multipart filename hooks. Request and response finish instrumentations retain symmetric
javax.servletandjakarta.servletstructure matchers because the production agent packages both generations. As a diagnostic, both module suites also passed with those matchers temporarily disabled after the test dependency was decoupled, but that isolated test setup does not model both generations being loaded by the production agent.Validation:
./gradlew :components:http:http-api:check :components:http:http-api:forkedTest :dd-java-agent:instrumentation:openai-java:openai-java-3.0:test :dd-java-agent:instrumentation:openai-java:openai-java-3.0:forkedTest :dd-java-agent:instrumentation:openai-java:openai-java-3.0:latestDepTest :dd-java-agent:instrumentation:sofarpc:sofarpc-5.0:test :dd-java-agent:instrumentation:sofarpc:sofarpc-5.0:forkedTest :dd-java-agent:instrumentation:sofarpc:sofarpc-5.0:latestDepTest :dd-smoke-tests:spring-boot-2.5-webflux:test -PtestJvm=17 --rerun-tasks(197 tests passed, 0 failed; 3 intentionally disabled HTTP-provider tests skipped)./gradlew :components:http:http-api:compileTestFixturesJava :components:http:http-api:forkedTest -PtestJvm=11 --rerun-tasks./gradlew :components:http:http-api:compileTestFixturesJava :components:http:http-api:forkedTest -PtestJvm=25 --rerun-tasks./gradlew :dd-java-agent:instrumentation:quartz-2.0:test :dd-java-agent:instrumentation:quartz-2.0:version40Test :dd-java-agent:instrumentation:quartz-2.0:latestDepTest --rerun-tasks(4 tests passed in each suite)./gradlew :dd-java-agent:instrumentation:spring:spring-webflux:spring-webflux-5.0:iastTest --rerun-tasks(8 tests passed)./gradlew :dd-smoke-tests:spring-boot-2.4-webflux:test :dd-smoke-tests:springboot-mongo:test -PtestJvm=17 --rerun-tasks(3 tests passed in each suite)./gradlew :dd-smoke-tests:appsec:springboot:test -PtestJvm=17 --rerun-tasks./gradlew :dd-smoke-tests:springboot:test -PtestJvm=8 --rerun-tasks(211 tests passed, 0 failed)./gradlew :dd-java-agent:instrumentation:liberty:liberty-20.0:test :dd-java-agent:instrumentation:liberty:liberty-20.0:forkedTest :dd-java-agent:instrumentation:liberty:liberty-23.0:test :dd-java-agent:instrumentation:liberty:liberty-23.0:forkedTest -PtestJvm=17 --rerun-tasks(527 tests passed, 0 failed)./gradlew :dd-java-agent:instrumentation:liberty:liberty-20.0:muzzle :dd-java-agent:instrumentation:liberty:liberty-23.0:muzzle(2 checks passed)./gradlew :dd-java-agent:instrumentation:liberty:liberty-common:build./gradlew -p build-logic :spring-boot-shadow:check./gradlew :dd-java-agent:shadowJarGradle issue: gradle/gradle#38057
Contributor 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 issueJira ticket: [PROJ-IDENT]