-
Notifications
You must be signed in to change notification settings - Fork 312
Added script that will collect thread and heap dumps in case test timeout on CI. #9414
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?
Conversation
# Conflicts: # dd-java-agent/instrumentation/java-concurrent/java-concurrent-21/src/previewTest/groovy/StructuredConcurrencyTest.groovy # dd-java-agent/instrumentation/lettuce-4/src/test/groovy/Lettuce4ClientTestBase.groovy
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
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.
Thanks that's really useful
dd-java-agent/instrumentation/lettuce-4/src/test/groovy/Lettuce4ClientTestBase.groovy
Outdated
Show resolved
Hide resolved
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.
This looks ok for me.
On a side note, I would prefer to see such features as convention plugins, and written in kotlin.
// Schedule thread and heap dumps collection near test timeout. | ||
tasks.withType(Test).configureEach { | ||
doFirst { | ||
def isCI = System.getenv('CI') != null |
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: This if ok, but you might prefer providers.environmentVariable("CI")
String buildDir = layout.buildDirectory.asFile.get().absolutePath | ||
if (isCI) { | ||
// Move reports into the folder collected by the collect_reports.sh script. | ||
buildDir = buildDir.replace('dd-trace-java/dd-java-agent', 'dd-trace-java/workspace/dd-java-agent') | ||
} |
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: This is also fine, but prefer provider mappers, something like
String buildDir = layout.buildDirectory.asFile.get().absolutePath | |
if (isCI) { | |
// Move reports into the folder collected by the collect_reports.sh script. | |
buildDir = buildDir.replace('dd-trace-java/dd-java-agent', 'dd-trace-java/workspace/dd-java-agent') | |
} | |
def buildDir = layout.buildDirectory.asFile.map { | |
if (!isCI) { | |
// Move reports into the folder collected by the collect_reports.sh script. | |
it.absolutePath.replace('dd-trace-java/dd-java-agent', 'dd-trace-java/workspace/dd-java-agent') | |
} else { | |
it.asbsolutePath | |
} | |
} |
Also, might be worth making dd-trace-java/workspace/dd-java-agent
an explicit variable or constant.
// Calculate delay for taking dumps as test timeout minus 2 minutes, but no less than 5 minutes. | ||
def delay = timeout.get().minusMinutes(2) | ||
long delayMinutes = Math.max(5L, delay.toMinutes()) |
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.
nitpick: long startDelayMinutes
, maybe also delay
to delayMinutes
// Only process 'Gradle test executors'. | ||
if (!line.contains('Gradle Test Executor')) return | ||
|
||
def pid = line.substring(0, line.indexOf(' ')) |
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.
polish: indentation
def pid = line.substring(0, line.indexOf(' ')) | |
def pid = line.substring(0, line.indexOf(' ')) |
def pid = line.substring(0, line.indexOf(' ')) | ||
|
||
// Collect thread dump. | ||
def threadDumpFile = new File(dumpDir, "${pid}-thread-dump-${System.currentTimeMillis()}.log") |
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: Might be worth adding the path of the task, testTask.getPath()
, which might return something like :dd-java-agent:instrumentation:mule-4:test
, so it probably needs some cleaning to be used as a filename.
finally { | ||
scheduler.shutdown() | ||
} |
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.
question: This task stops after gathering a single thread dump by processes. I wonder if it would be useful to collect a sequence of thread dumps every X ms.
@@ -126,7 +128,7 @@ if (!project.property("activePartition")) { | |||
} | |||
} | |||
|
|||
tasks.withType(Test) { | |||
tasks.withType(Test).configureEach { |
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.
praise: Good fix !
What Does This Do
Added script that will collect thread and heap dumps in case test timeout on CI.
Motivation
Green CI.
Additional Notes
Script will schedule taking thread and heap dump at
test_timout_minus_2_minutes
.Dumps will be collected and stored as part of test GitLab Job artifact.