-
Notifications
You must be signed in to change notification settings - Fork 358
Enable Gradle enhanced graph ordering #12326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| plugins { | ||
| `kotlin-dsl` | ||
| } | ||
|
|
||
| // Shadow 9's plugin API targets Java 17, so keep it isolated from Java 8 build-logic modules. | ||
| java { | ||
| sourceCompatibility = JavaVersion.VERSION_17 | ||
| targetCompatibility = JavaVersion.VERSION_17 | ||
| } | ||
|
|
||
| kotlin { | ||
| compilerOptions { | ||
| jvmTarget.set(org.jetbrains.kotlin.gradle.dsl.JvmTarget.JVM_17) | ||
| } | ||
| } | ||
|
|
||
| dependencies { | ||
| implementation("com.gradleup.shadow:shadow-gradle-plugin:${libs.versions.shadow.get()}") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJar | ||
| import com.github.jengelman.gradle.plugins.shadow.transformers.PropertiesFileTransformer | ||
|
|
||
| plugins { | ||
| id("com.gradleup.shadow") | ||
| } | ||
|
|
||
| tasks.withType<ShadowJar>().configureEach { | ||
| if (name == "shadowJar") { | ||
| // `configurations` is left at its convention, which the Shadow plugin already sets to | ||
| // `runtimeClasspath`; adding it again is a no-op because the property is a `SetProperty`. | ||
|
|
||
| // Spring discovery metadata can occur in multiple dependency jars. With enhanced graph | ||
| // ordering, keeping only the first duplicate may omit required registrations. | ||
| duplicatesStrategy = DuplicatesStrategy.INCLUDE | ||
| mergeServiceFiles() | ||
| append("META-INF/spring.handlers") | ||
| append("META-INF/spring.schemas") | ||
| append("META-INF/spring.tooling") | ||
| transform(PropertiesFileTransformer::class.java) { | ||
| paths.set(listOf("META-INF/spring.factories")) | ||
| mergeStrategy.set(PropertiesFileTransformer.MergeStrategy.Append) | ||
| } | ||
| filesNotMatching( | ||
| listOf( | ||
| "META-INF/services/**", | ||
| "META-INF/spring.handlers", | ||
| "META-INF/spring.schemas", | ||
| "META-INF/spring.tooling", | ||
| "META-INF/spring.factories", | ||
| ) | ||
| ) { | ||
| duplicatesStrategy = DuplicatesStrategy.EXCLUDE | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,5 +28,21 @@ dependencies { | |
| // testRuntimeOnly(project(":components:http:http-lib-jdk")) | ||
| // testRuntimeOnly(project(":components:http:http-lib-okhttp")) | ||
| // Add MockServer for test fixtures | ||
| testFixturesImplementation("org.mock-server:mockserver-junit-jupiter-no-dependencies:5.14.0") | ||
| // Avoid mockserver-junit-jupiter-no-dependencies: its embedded JUnit classes can shadow ours. | ||
| testFixturesImplementation(libs.junit.jupiter) | ||
| // 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. | ||
|
Comment on lines
+33
to
+43
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I don't think this is useful to stay in the code, maybe in the commit message body. |
||
| // | ||
| // Switching to the non-shaded `org.mock-server:mockserver-netty` would remove the hazard | ||
| // entirely by letting Gradle arbitrate slf4j, at the cost of many transitives. | ||
| testFixturesImplementation("org.mock-server:mockserver-netty-no-dependencies:5.14.0") | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| package datadog.http.client; | ||
|
|
||
| import static org.mockserver.integration.ClientAndServer.startClientAndServer; | ||
|
|
||
| import org.junit.jupiter.api.extension.AfterAllCallback; | ||
| import org.junit.jupiter.api.extension.BeforeAllCallback; | ||
| import org.junit.jupiter.api.extension.ExtensionContext; | ||
| import org.junit.jupiter.api.extension.ParameterContext; | ||
| import org.junit.jupiter.api.extension.ParameterResolutionException; | ||
| import org.junit.jupiter.api.extension.ParameterResolver; | ||
| import org.mockserver.client.MockServerClient; | ||
| import org.mockserver.integration.ClientAndServer; | ||
|
|
||
| /** Minimal JUnit lifecycle adapter for the shaded MockServer server artifact. */ | ||
| public final class MockServerExtension | ||
| implements ParameterResolver, BeforeAllCallback, AfterAllCallback { | ||
| private ClientAndServer server; | ||
|
|
||
| @Override | ||
| public void beforeAll(ExtensionContext context) { | ||
| this.server = startClientAndServer(0); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean supportsParameter( | ||
| ParameterContext parameterContext, ExtensionContext extensionContext) { | ||
| return MockServerClient.class.isAssignableFrom(parameterContext.getParameter().getType()); | ||
| } | ||
|
|
||
| @Override | ||
| public Object resolveParameter( | ||
| ParameterContext parameterContext, ExtensionContext extensionContext) { | ||
| if (this.server == null) { | ||
| throw new ParameterResolutionException("MockServer has not been started"); | ||
| } | ||
| return this.server; | ||
| } | ||
|
|
||
| @Override | ||
| public void afterAll(ExtensionContext context) { | ||
| if (this.server != null && this.server.isRunning()) { | ||
| this.server.stop(); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,8 @@ dependencies { | |
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: This looks like inference comment |
||
| testRuntimeOnly project(':dd-java-agent:instrumentation:liberty:liberty-common') | ||
| testRuntimeOnly project(':dd-java-agent:instrumentation:servlet:javax-servlet:javax-servlet-3.0') | ||
|
|
||
| testImplementation(libs.groovy.xml) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| package datadog.trace.instrumentation.liberty20; | ||
|
|
||
| import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; | ||
| import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; | ||
| import static datadog.trace.bootstrap.instrumentation.api.AgentSpan.fromContext; | ||
| import static datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator.DD_CONTEXT_ATTRIBUTE; | ||
|
|
@@ -16,10 +17,14 @@ | |
| import datadog.trace.bootstrap.instrumentation.api.AgentSpan; | ||
| import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
| import net.bytebuddy.asm.Advice; | ||
| import net.bytebuddy.description.type.TypeDescription; | ||
| import net.bytebuddy.matcher.ElementMatcher; | ||
|
|
||
| @AutoService(InstrumenterModule.class) | ||
| public class RequestFinishInstrumentation extends InstrumenterModule.Tracing | ||
| implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { | ||
| implements Instrumenter.ForSingleType, | ||
| Instrumenter.WithTypeStructure, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Are we sure about that ? |
||
| Instrumenter.HasMethodAdvice { | ||
|
|
||
| public RequestFinishInstrumentation() { | ||
| super("liberty"); | ||
|
|
@@ -42,6 +47,12 @@ public String instrumentedType() { | |
| return "com.ibm.ws.webcontainer.srt.SRTServletRequest"; | ||
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher<TypeDescription> structureMatcher() { | ||
| // Liberty keeps this class name across the javax-to-jakarta servlet migration. | ||
| return implementsInterface(named("javax.servlet.http.HttpServletRequest")); | ||
| } | ||
|
|
||
| @Override | ||
| public void methodAdvice(MethodTransformer transformer) { | ||
| transformer.applyAdvice( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| package datadog.trace.instrumentation.liberty20; | ||
|
|
||
| import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; | ||
| import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; | ||
| import static datadog.trace.bootstrap.instrumentation.api.AgentSpan.fromContext; | ||
| import static datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator.DD_CONTEXT_ATTRIBUTE; | ||
|
|
@@ -18,6 +19,8 @@ | |
| import datadog.trace.bootstrap.instrumentation.api.AgentSpan; | ||
| import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
| import net.bytebuddy.asm.Advice; | ||
| import net.bytebuddy.description.type.TypeDescription; | ||
| import net.bytebuddy.matcher.ElementMatcher; | ||
|
|
||
| /** | ||
| * XXX: {@link SRTServletResponse#finish()} is not appropriate method to look at the response | ||
|
|
@@ -28,7 +31,9 @@ | |
| */ | ||
| @AutoService(InstrumenterModule.class) | ||
| public class ResponseFinishInstrumentation extends InstrumenterModule.Tracing | ||
| implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { | ||
| implements Instrumenter.ForSingleType, | ||
| Instrumenter.WithTypeStructure, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Are we sure about that ? |
||
| Instrumenter.HasMethodAdvice { | ||
|
|
||
| public ResponseFinishInstrumentation() { | ||
| super("liberty"); | ||
|
|
@@ -51,6 +56,12 @@ public String instrumentedType() { | |
| return "com.ibm.ws.webcontainer.srt.SRTServletResponse"; | ||
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher<TypeDescription> structureMatcher() { | ||
| // Liberty keeps this class name across the javax-to-jakarta servlet migration. | ||
| return implementsInterface(named("javax.servlet.http.HttpServletResponse")); | ||
| } | ||
|
|
||
| @Override | ||
| public void methodAdvice(MethodTransformer transformer) { | ||
| transformer.applyAdvice( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| plugins { | ||
| id 'dd-trace-java.module.instrumentation' | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I think that
spring-boot-shadow, should be prefixed withsmoke-test, likesmoke-test-spring-boot-appAlso, maybe the existing smoke-test can be renamed to smoke-test-app (like it's custom plugin).