-
Notifications
You must be signed in to change notification settings - Fork 311
Add support for Java 25 Structured Concurrency API #9276
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
base: master
Are you sure you want to change the base?
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,55 @@ | ||||||||||||||||||
ext { | ||||||||||||||||||
minJavaVersionForTests = JavaVersion.VERSION_25 | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
apply from: "$rootDir/gradle/java.gradle" | ||||||||||||||||||
apply plugin: 'idea' | ||||||||||||||||||
|
||||||||||||||||||
muzzle { | ||||||||||||||||||
pass { | ||||||||||||||||||
coreJdk('25') | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
idea { | ||||||||||||||||||
module { | ||||||||||||||||||
jdkName = '25' | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/* | ||||||||||||||||||
* Declare previewTest, a test suite that requires the Javac/Java --enable-preview feature flag | ||||||||||||||||||
*/ | ||||||||||||||||||
addTestSuite('previewTest') | ||||||||||||||||||
// Configure groovy test file compilation | ||||||||||||||||||
compilePreviewTestGroovy.configure { | ||||||||||||||||||
javaLauncher = javaToolchains.launcherFor { | ||||||||||||||||||
languageVersion = JavaLanguageVersion.of(25) | ||||||||||||||||||
} | ||||||||||||||||||
options.compilerArgs.add("--enable-preview") | ||||||||||||||||||
} | ||||||||||||||||||
// Configure Java test files compilation | ||||||||||||||||||
compilePreviewTestJava.configure { | ||||||||||||||||||
options.compilerArgs.add("--enable-preview") | ||||||||||||||||||
} | ||||||||||||||||||
// Configure tests execution | ||||||||||||||||||
previewTest.configure { | ||||||||||||||||||
jvmArgs = ['--enable-preview'] | ||||||||||||||||||
} | ||||||||||||||||||
// Require the preview test suite to run as part of module check | ||||||||||||||||||
tasks.named("check").configure { | ||||||||||||||||||
dependsOn "previewTest" | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
dependencies { | ||||||||||||||||||
testImplementation project(':dd-java-agent:instrumentation:trace-annotation') | ||||||||||||||||||
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: FYI, Groovy 5 rc1 is out, https://groovy-lang.org/changelogs/changelog-5.0.0-rc-1.html But requires a JDK 11 to run. |
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// Set all compile tasks to use JDK21 but let instrumentation code targets 1.8 compatibility | ||||||||||||||||||
project.tasks.withType(AbstractCompile).configureEach { | ||||||||||||||||||
setJavaVersion(it, 25) | ||||||||||||||||||
} | ||||||||||||||||||
Comment on lines
+48
to
+51
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:
Suggested change
|
||||||||||||||||||
compileJava.configure { | ||||||||||||||||||
sourceCompatibility = JavaVersion.VERSION_1_8 | ||||||||||||||||||
targetCompatibility = JavaVersion.VERSION_1_8 | ||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
package datadog.trace.instrumentation.java.concurrent.structuredconcurrency; | ||
|
||
import static datadog.trace.bootstrap.instrumentation.java.concurrent.AdviceUtils.capture; | ||
import static java.util.Collections.singletonMap; | ||
import static net.bytebuddy.matcher.ElementMatchers.isConstructor; | ||
|
||
import com.google.auto.service.AutoService; | ||
import datadog.environment.JavaVirtualMachine; | ||
import datadog.trace.agent.tooling.Instrumenter; | ||
import datadog.trace.agent.tooling.InstrumenterModule; | ||
import datadog.trace.bootstrap.ContextStore; | ||
import datadog.trace.bootstrap.InstrumentationContext; | ||
import datadog.trace.bootstrap.instrumentation.java.concurrent.State; | ||
import java.util.Map; | ||
import net.bytebuddy.asm.Advice; | ||
|
||
/** | ||
* This instrumentation captures the active span scope at StructuredTaskScope task creation | ||
* (SubtaskImpl). The scope is then activate and close through the Runnable instrumentation | ||
* (SubtaskImpl implementation Runnable). | ||
*/ | ||
@SuppressWarnings("unused") | ||
@AutoService(InstrumenterModule.class) | ||
public class StructuredTaskScopeInstrumentation extends InstrumenterModule.Tracing | ||
implements Instrumenter.ForBootstrap, Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { | ||
|
||
private static final String SUBTASK_IMPL_CLASS_NAME = | ||
"java.util.concurrent.StructuredTaskScopeImpl.SubtaskImpl"; | ||
|
||
public StructuredTaskScopeInstrumentation() { | ||
super("java_concurrent", "structured_task_scope"); | ||
} | ||
|
||
@Override | ||
public String instrumentedType() { | ||
return SUBTASK_IMPL_CLASS_NAME; | ||
} | ||
|
||
@Override | ||
public boolean isEnabled() { | ||
return JavaVirtualMachine.isJavaVersionAtLeast(25) && super.isEnabled(); | ||
} | ||
|
||
@Override | ||
public Map<String, String> contextStore() { | ||
return singletonMap(SUBTASK_IMPL_CLASS_NAME, State.class.getName()); | ||
} | ||
|
||
@Override | ||
public void methodAdvice(MethodTransformer transformer) { | ||
transformer.applyAdvice(isConstructor(), getClass().getName() + "$ConstructorAdvice"); | ||
} | ||
|
||
public static final class ConstructorAdvice { | ||
@Advice.OnMethodExit | ||
public static <T> void captureScope( | ||
@Advice.This Object task // StructuredTaskScopeImpl.SubtaskImpl | ||
// (the advice are compile against Java 8 so the type from JDK25 can't be referred) | ||
) { | ||
ContextStore<Object, State> contextStore = | ||
InstrumentationContext.get( | ||
SUBTASK_IMPL_CLASS_NAME, | ||
"datadog.trace.bootstrap.instrumentation.java.concurrent.State"); | ||
capture(contextStore, task); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,170 @@ | ||
import datadog.trace.agent.test.AgentTestRunner | ||
import datadog.trace.api.Trace | ||
import spock.lang.Timeout | ||
|
||
import java.util.concurrent.Callable | ||
import java.util.concurrent.StructuredTaskScope | ||
|
||
import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace | ||
import static datadog.trace.agent.test.utils.TraceUtils.runnableUnderTrace | ||
|
||
class StructuredConcurrencyTest extends AgentTestRunner { | ||
/** | ||
* Tests the structured task scope with a single task. | ||
*/ | ||
@Timeout(10) | ||
def "test single task"() { | ||
setup: | ||
def taskScope = StructuredTaskScope.open() | ||
def result = false | ||
|
||
when: | ||
runUnderTrace("parent") { | ||
def task = taskScope.fork(new Callable<Boolean>() { | ||
@Trace(operationName = "child") | ||
@Override | ||
Boolean call() throws Exception { | ||
return true | ||
} | ||
}) | ||
taskScope.join() | ||
result = task.get() | ||
} | ||
taskScope.close() | ||
|
||
then: | ||
result | ||
assertTraces(1) { | ||
sortSpansByStart() | ||
trace(2) { | ||
span(0) { | ||
parent() | ||
operationName "parent" | ||
} | ||
span(1) { | ||
childOfPrevious() | ||
operationName "child" | ||
} | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Tests the structured task scope with a multiple tasks. | ||
* Here is the expected task/span structure: | ||
* <pre> | ||
* parent | ||
* |-- child1 | ||
* |-- child2 | ||
* \-- child3 | ||
* </pre> | ||
*/ | ||
@Timeout(10) | ||
def "test multiple tasks"() { | ||
setup: | ||
def taskScope = StructuredTaskScope.open() | ||
|
||
when: | ||
runUnderTrace("parent") { | ||
taskScope.fork { | ||
runnableUnderTrace("child1") {} | ||
} | ||
taskScope.fork { | ||
runnableUnderTrace("child2") {} | ||
} | ||
taskScope.fork { | ||
runnableUnderTrace("child3") {} | ||
} | ||
taskScope.join() | ||
} | ||
taskScope.close() | ||
|
||
then: | ||
assertTraces(1) { | ||
sortSpansByStart() | ||
trace(4) { | ||
span { | ||
parent() | ||
operationName "parent" | ||
} | ||
def parent = span(0) | ||
span { | ||
childOf(parent) | ||
assert span.operationName.toString().startsWith("child") | ||
} | ||
span { | ||
childOf(parent) | ||
assert span.operationName.toString().startsWith("child") | ||
} | ||
span { | ||
childOf(parent) | ||
assert span.operationName.toString().startsWith("child") | ||
} | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Tests the structured task scope with a multiple nested tasks. | ||
* Here is the expected task/span structure: | ||
* <pre> | ||
* parent | ||
* |-- child1 | ||
* | |-- great-child1-1 | ||
* | \-- great-child1-2 | ||
* \-- child2 | ||
* </pre> | ||
*/ | ||
@Timeout(10) | ||
def "test nested tasks"() { | ||
setup: | ||
def taskScope = StructuredTaskScope.open() | ||
|
||
when: | ||
runUnderTrace("parent") { | ||
taskScope.fork { | ||
runnableUnderTrace("child1") { | ||
taskScope.fork { | ||
runnableUnderTrace("great-child1-1") {} | ||
} | ||
taskScope.fork { | ||
runnableUnderTrace("great-child1-2") {} | ||
} | ||
} | ||
} | ||
taskScope.fork { | ||
runnableUnderTrace("child2") {} | ||
} | ||
taskScope.join() | ||
} | ||
taskScope.close() | ||
|
||
then: | ||
assertTraces(1) { | ||
sortSpansByStart() | ||
trace(5) { | ||
// Check parent span | ||
span { | ||
parent() | ||
operationName "parent" | ||
} | ||
def parent = span(0) | ||
// Check child and great child spans | ||
def child1 = null | ||
for (i in 0..<4) { | ||
span { | ||
def name = span.operationName.toString() | ||
if (name.startsWith("child")) { | ||
childOf(parent) | ||
if (name == "child1") { | ||
child1 = span | ||
} | ||
} else if (name.startsWith("great-child1")) { | ||
childOf(child1) // We can assume child1 will be set as spans are sorted by start time | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,4 +4,4 @@ org.gradle.jvmargs=-XX:MaxMetaspaceSize=1g | |
org.gradle.java.installations.auto-detect=false | ||
org.gradle.java.installations.auto-download=false | ||
# 8 and 11 is needed to build | ||
org.gradle.java.installations.fromEnv=JAVA_8_HOME,JAVA_11_HOME,JAVA_17_HOME,JAVA_21_HOME | ||
org.gradle.java.installations.fromEnv=JAVA_8_HOME,JAVA_11_HOME,JAVA_17_HOME,JAVA_21_HOME,JAVA_25_HOME | ||
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. BTW, should we have 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. Hm 🤔 Since we don't build anything in Java 24 (only test), I don't think we need it. 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. It seems like finding 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. It feels like we should move to some other mechanism to detect JVMs... 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.
It should be for 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. @sarahchen6 I recall that we have idea to rename 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. Yes, I remember we didn't go through with it because of the idea that the build image should always provide the latest "stable" version in addition to specific LTS versions that we want to test. It is on the CI, not the available build images, to avoid duplicate testing (i.e. we do not test stable when stable == LTS). There is a conversation on this PR for more context: DataDog/dd-trace-java-docker-build#107 |
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: how about to use Kotlin DSL for new
build.gradle
?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.
I would rather wait for having a clear migration path rather than trying to build new conventions and plugins as part of this PR. WDYT?
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.
I don't think that using kotlin DSL is gonna change that or not. However it clearly help IDE support now. In my opinion I would try to use kotlin DSL regardless. This can also help exercise our skills in that regard.
That said while I would prefer kotlin dsl usage regardless, I won't push for it either at this time.