-
Notifications
You must be signed in to change notification settings - Fork 315
Introduces testJvmConstraints Gradle extension to replace extra properties
#9892
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
Conversation
…sion # Conflicts: # dd-java-agent/instrumentation/aerospike-4.0/build.gradle # dd-java-agent/instrumentation/armeria/armeria-jetty-1.24/build.gradle
… test jvm constraints
…includeJdk directive
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
| tracerJava { | ||
| addSourceSetFor(JavaVersion.VERSION_11) | ||
| } | ||
|
|
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.
just curious -- how come in some files like this one, we use tracerJava instead of testJvmConstraint like this:
testJvmConstraint {
minJavaVersion = JavaVersion.VERSION_11
}
?
e.g. dd-java-agent/instrumentation/java/java-lang/java-lang-17.0/build.gradle now uses testJvmConstraint vs. dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-12.0/build.gradle now uses tracerJava whereas both previously had:
ext {
minJavaVersionForTests = JavaVersion.VERSION_17
}
and e.g. dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/build.gradle has both
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.
Good catch !
Before minJavaVersionForTests was used for two purposes, as a constraint testJvm only, i.e. they were no java source with this version.
When there are source set, addSourceSetFor actually sets by default the minimum JVM for test as well.
I chose to keep things explicit. But thinking about it maybe I can make another pass to avoid setting this twice.
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 see. Thanks for the clarification!
...dSrc/src/main/kotlin/datadog/gradle/plugin/testJvmConstraints/TestJvmConstraintsExtension.kt
Outdated
Show resolved
Hide resolved
sarahchen6
left a comment
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.
Pre-approving because I think these suggestions could be included in a follow-up PR as well, but wow thanks for this great build improvement!
buildSrc/src/main/kotlin/datadog/gradle/plugin/testJvmConstraints/TestJvmConstraintsUtils.kt
Outdated
Show resolved
Hide resolved
buildSrc/src/main/kotlin/datadog.test-jvm-contraints.gradle.kts
Outdated
Show resolved
Hide resolved
bric3
left a comment
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 for the review
buildSrc/src/main/kotlin/datadog.test-jvm-contraints.gradle.kts
Outdated
Show resolved
Hide resolved
...dSrc/src/main/kotlin/datadog/gradle/plugin/testJvmConstraints/TestJvmConstraintsExtension.kt
Outdated
Show resolved
Hide resolved
| tracerJava { | ||
| addSourceSetFor(JavaVersion.VERSION_11) | ||
| } | ||
|
|
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.
Good catch !
Before minJavaVersionForTests was used for two purposes, as a constraint testJvm only, i.e. they were no java source with this version.
When there are source set, addSourceSetFor actually sets by default the minimum JVM for test as well.
I chose to keep things explicit. But thinking about it maybe I can make another pass to avoid setting this twice.
testJvmConstraint Gradle extension to replace extra propertiestestJvmConstraints Gradle extension to replace extra properties
…s-to-extensions # Conflicts: # dd-java-agent/instrumentation/jakarta-jms/build.gradle # dd-java-agent/instrumentation/karate/build.gradle # dd-java-agent/instrumentation/selenium/build.gradle # dd-java-agent/instrumentation/testng/testng-7/build.gradle
97d50b4 to
b2345ac
Compare
|
🎯 Code Coverage 🔗 Commit SHA: b2345ac | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
amarziali
left a comment
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.
Is really neat and now declaring test compatibilities with that kind of directive is way more clear! Thanks for having refactored this. As a future improvement (not needed for this PR) it could be nice to change the documentation
Kafka / producer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 54 metrics, 11 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.55.0-SNAPSHOT~b2345ac263, baseline=1.56.0-SNAPSHOT~0ab0769ee0
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.05 s) : 0, 1050031
Total [baseline] (8.645 s) : 0, 8644890
Agent [candidate] (1.062 s) : 0, 1062356
Total [candidate] (8.676 s) : 0, 8676354
section iast
Agent [baseline] (1.177 s) : 0, 1176669
Total [baseline] (9.261 s) : 0, 9261368
Agent [candidate] (1.185 s) : 0, 1184858
Total [candidate] (9.29 s) : 0, 9289516
gantt
title insecure-bank - break down per module: candidate=1.55.0-SNAPSHOT~b2345ac263, baseline=1.56.0-SNAPSHOT~0ab0769ee0
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.476 ms) : 0, 1476
crashtracking [candidate] (1.481 ms) : 0, 1481
BytebuddyAgent [baseline] (707.798 ms) : 0, 707798
BytebuddyAgent [candidate] (715.071 ms) : 0, 715071
GlobalTracer [baseline] (245.781 ms) : 0, 245781
GlobalTracer [candidate] (248.928 ms) : 0, 248928
AppSec [baseline] (32.233 ms) : 0, 32233
AppSec [candidate] (32.93 ms) : 0, 32930
Debugger [baseline] (6.349 ms) : 0, 6349
Debugger [candidate] (6.547 ms) : 0, 6547
Remote Config [baseline] (738.2 µs) : 0, 738
Remote Config [candidate] (713.076 µs) : 0, 713
Telemetry [baseline] (15.04 ms) : 0, 15040
Telemetry [candidate] (13.352 ms) : 0, 13352
Flare Poller [baseline] (5.769 ms) : 0, 5769
Flare Poller [candidate] (8.183 ms) : 0, 8183
section iast
crashtracking [baseline] (1.46 ms) : 0, 1460
crashtracking [candidate] (1.449 ms) : 0, 1449
BytebuddyAgent [baseline] (826.806 ms) : 0, 826806
BytebuddyAgent [candidate] (831.042 ms) : 0, 831042
GlobalTracer [baseline] (233.833 ms) : 0, 233833
GlobalTracer [candidate] (236.332 ms) : 0, 236332
AppSec [baseline] (26.219 ms) : 0, 26219
AppSec [candidate] (28.208 ms) : 0, 28208
Debugger [baseline] (6.004 ms) : 0, 6004
Debugger [candidate] (6.002 ms) : 0, 6002
Remote Config [baseline] (595.028 µs) : 0, 595
Remote Config [candidate] (601.26 µs) : 0, 601
Telemetry [baseline] (8.27 ms) : 0, 8270
Telemetry [candidate] (8.535 ms) : 0, 8535
Flare Poller [baseline] (4.164 ms) : 0, 4164
Flare Poller [candidate] (4.099 ms) : 0, 4099
IAST [baseline] (34.632 ms) : 0, 34632
IAST [candidate] (33.805 ms) : 0, 33805
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.55.0-SNAPSHOT~b2345ac263, baseline=1.56.0-SNAPSHOT~0ab0769ee0
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.053 s) : 0, 1052969
Total [baseline] (10.805 s) : 0, 10805166
Agent [candidate] (1.047 s) : 0, 1047065
Total [candidate] (10.798 s) : 0, 10798346
section appsec
Agent [baseline] (1.228 s) : 0, 1228464
Total [baseline] (10.895 s) : 0, 10894726
Agent [candidate] (1.224 s) : 0, 1224144
Total [candidate] (10.944 s) : 0, 10943949
section iast
Agent [baseline] (1.176 s) : 0, 1176453
Total [baseline] (11.128 s) : 0, 11127514
Agent [candidate] (1.187 s) : 0, 1186765
Total [candidate] (11.264 s) : 0, 11264423
section profiling
Agent [baseline] (1.203 s) : 0, 1202794
Total [baseline] (10.973 s) : 0, 10972532
Agent [candidate] (1.199 s) : 0, 1198748
Total [candidate] (10.978 s) : 0, 10977524
gantt
title petclinic - break down per module: candidate=1.55.0-SNAPSHOT~b2345ac263, baseline=1.56.0-SNAPSHOT~0ab0769ee0
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.47 ms) : 0, 1470
crashtracking [candidate] (1.454 ms) : 0, 1454
BytebuddyAgent [baseline] (708.635 ms) : 0, 708635
BytebuddyAgent [candidate] (704.271 ms) : 0, 704271
GlobalTracer [baseline] (246.942 ms) : 0, 246942
GlobalTracer [candidate] (245.963 ms) : 0, 245963
AppSec [baseline] (32.656 ms) : 0, 32656
AppSec [candidate] (32.495 ms) : 0, 32495
Debugger [baseline] (6.448 ms) : 0, 6448
Debugger [candidate] (6.391 ms) : 0, 6391
Remote Config [baseline] (716.436 µs) : 0, 716
Remote Config [candidate] (723.212 µs) : 0, 723
Telemetry [baseline] (12.248 ms) : 0, 12248
Telemetry [candidate] (14.395 ms) : 0, 14395
Flare Poller [baseline] (8.946 ms) : 0, 8946
Flare Poller [candidate] (6.6 ms) : 0, 6600
section appsec
crashtracking [baseline] (1.471 ms) : 0, 1471
crashtracking [candidate] (1.455 ms) : 0, 1455
BytebuddyAgent [baseline] (735.118 ms) : 0, 735118
BytebuddyAgent [candidate] (730.473 ms) : 0, 730473
GlobalTracer [baseline] (238.058 ms) : 0, 238058
GlobalTracer [candidate] (237.481 ms) : 0, 237481
AppSec [baseline] (174.634 ms) : 0, 174634
AppSec [candidate] (175.611 ms) : 0, 175611
Debugger [baseline] (5.994 ms) : 0, 5994
Debugger [candidate] (6.025 ms) : 0, 6025
Remote Config [baseline] (647.257 µs) : 0, 647
Remote Config [candidate] (652.915 µs) : 0, 653
Telemetry [baseline] (8.574 ms) : 0, 8574
Telemetry [candidate] (8.632 ms) : 0, 8632
Flare Poller [baseline] (3.989 ms) : 0, 3989
Flare Poller [candidate] (4.021 ms) : 0, 4021
IAST [baseline] (24.852 ms) : 0, 24852
IAST [candidate] (24.809 ms) : 0, 24809
section iast
crashtracking [baseline] (1.458 ms) : 0, 1458
crashtracking [candidate] (1.465 ms) : 0, 1465
BytebuddyAgent [baseline] (825.509 ms) : 0, 825509
BytebuddyAgent [candidate] (834.22 ms) : 0, 834220
GlobalTracer [baseline] (234.004 ms) : 0, 234004
GlobalTracer [candidate] (235.642 ms) : 0, 235642
AppSec [baseline] (28.305 ms) : 0, 28305
AppSec [candidate] (27.168 ms) : 0, 27168
Debugger [baseline] (6.03 ms) : 0, 6030
Debugger [candidate] (6.056 ms) : 0, 6056
Remote Config [baseline] (601.499 µs) : 0, 601
Remote Config [candidate] (609.531 µs) : 0, 610
Telemetry [baseline] (8.407 ms) : 0, 8407
Telemetry [candidate] (8.407 ms) : 0, 8407
Flare Poller [baseline] (4.113 ms) : 0, 4113
Flare Poller [candidate] (4.127 ms) : 0, 4127
IAST [baseline] (33.283 ms) : 0, 33283
IAST [candidate] (34.217 ms) : 0, 34217
section profiling
crashtracking [baseline] (1.474 ms) : 0, 1474
crashtracking [candidate] (1.462 ms) : 0, 1462
BytebuddyAgent [baseline] (735.273 ms) : 0, 735273
BytebuddyAgent [candidate] (732.973 ms) : 0, 732973
GlobalTracer [baseline] (223.155 ms) : 0, 223155
GlobalTracer [candidate] (223.23 ms) : 0, 223230
AppSec [baseline] (32.294 ms) : 0, 32294
AppSec [candidate] (32.131 ms) : 0, 32131
Debugger [baseline] (8.356 ms) : 0, 8356
Debugger [candidate] (8.205 ms) : 0, 8205
Remote Config [baseline] (729.404 µs) : 0, 729
Remote Config [candidate] (725.925 µs) : 0, 726
Telemetry [baseline] (14.544 ms) : 0, 14544
Telemetry [candidate] (14.481 ms) : 0, 14481
Flare Poller [baseline] (4.111 ms) : 0, 4111
Flare Poller [candidate] (4.171 ms) : 0, 4171
ProfilingAgent [baseline] (112.138 ms) : 0, 112138
ProfilingAgent [candidate] (111.263 ms) : 0, 111263
Profiling [baseline] (112.802 ms) : 0, 112802
Profiling [candidate] (111.92 ms) : 0, 111920
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 3 performance regressions! Performance is the same for 7 metrics, 12 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.55.0-SNAPSHOT~b2345ac263, baseline=1.56.0-SNAPSHOT~0ab0769ee0
dateFormat X
axisFormat %s
section baseline
no_agent (1.201 ms) : 1189, 1213
. : milestone, 1201,
iast (3.205 ms) : 3158, 3252
. : milestone, 3205,
iast_FULL (5.833 ms) : 5775, 5891
. : milestone, 5833,
iast_GLOBAL (3.651 ms) : 3601, 3701
. : milestone, 3651,
profiling (2.335 ms) : 2312, 2358
. : milestone, 2335,
tracing (1.834 ms) : 1819, 1849
. : milestone, 1834,
section candidate
no_agent (1.187 ms) : 1176, 1199
. : milestone, 1187,
iast (3.28 ms) : 3241, 3319
. : milestone, 3280,
iast_FULL (5.788 ms) : 5729, 5846
. : milestone, 5788,
iast_GLOBAL (3.64 ms) : 3586, 3693
. : milestone, 3640,
profiling (1.945 ms) : 1928, 1962
. : milestone, 1945,
tracing (1.823 ms) : 1809, 1838
. : milestone, 1823,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.55.0-SNAPSHOT~b2345ac263, baseline=1.56.0-SNAPSHOT~0ab0769ee0
dateFormat X
axisFormat %s
section baseline
no_agent (19.119 ms) : 18923, 19315
. : milestone, 19119,
appsec (18.92 ms) : 18728, 19113
. : milestone, 18920,
code_origins (17.757 ms) : 17581, 17934
. : milestone, 17757,
iast (17.769 ms) : 17592, 17945
. : milestone, 17769,
profiling (18.628 ms) : 18440, 18817
. : milestone, 18628,
tracing (18.87 ms) : 18676, 19065
. : milestone, 18870,
section candidate
no_agent (19.768 ms) : 19561, 19974
. : milestone, 19768,
appsec (18.689 ms) : 18499, 18879
. : milestone, 18689,
code_origins (17.618 ms) : 17441, 17796
. : milestone, 17618,
iast (18.698 ms) : 18509, 18886
. : milestone, 18698,
profiling (19.613 ms) : 19411, 19815
. : milestone, 19613,
tracing (17.688 ms) : 17512, 17865
. : milestone, 17688,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.55.0-SNAPSHOT~b2345ac263, baseline=1.56.0-SNAPSHOT~0ab0769ee0
dateFormat X
axisFormat %s
section baseline
no_agent (14.767 s) : 14767000, 14767000
. : milestone, 14767000,
appsec (14.874 s) : 14874000, 14874000
. : milestone, 14874000,
iast (18.587 s) : 18587000, 18587000
. : milestone, 18587000,
iast_GLOBAL (18.138 s) : 18138000, 18138000
. : milestone, 18138000,
profiling (15.068 s) : 15068000, 15068000
. : milestone, 15068000,
tracing (14.923 s) : 14923000, 14923000
. : milestone, 14923000,
section candidate
no_agent (14.977 s) : 14977000, 14977000
. : milestone, 14977000,
appsec (14.943 s) : 14943000, 14943000
. : milestone, 14943000,
iast (18.376 s) : 18376000, 18376000
. : milestone, 18376000,
iast_GLOBAL (17.839 s) : 17839000, 17839000
. : milestone, 17839000,
profiling (14.979 s) : 14979000, 14979000
. : milestone, 14979000,
tracing (14.892 s) : 14892000, 14892000
. : milestone, 14892000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.55.0-SNAPSHOT~b2345ac263, baseline=1.56.0-SNAPSHOT~0ab0769ee0
dateFormat X
axisFormat %s
section baseline
no_agent (1.473 ms) : 1461, 1484
. : milestone, 1473,
appsec (3.692 ms) : 3474, 3910
. : milestone, 3692,
iast (2.206 ms) : 2143, 2270
. : milestone, 2206,
iast_GLOBAL (2.235 ms) : 2171, 2298
. : milestone, 2235,
profiling (2.071 ms) : 2017, 2124
. : milestone, 2071,
tracing (2.03 ms) : 1980, 2080
. : milestone, 2030,
section candidate
no_agent (1.471 ms) : 1460, 1483
. : milestone, 1471,
appsec (3.709 ms) : 3491, 3928
. : milestone, 3709,
iast (2.206 ms) : 2142, 2269
. : milestone, 2206,
iast_GLOBAL (2.248 ms) : 2184, 2311
. : milestone, 2248,
profiling (2.075 ms) : 2022, 2128
. : milestone, 2075,
tracing (2.016 ms) : 1966, 2065
. : milestone, 2016,
|
Kafka / consumer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
What Does This Do
Refactor the mechanism that relied on extra properties like
minJavaVersionForTeststo a Gradle extension mechanism.Also,
minJavaVersionForTestswas serving two distinct purposeThis PR explicitly segregates these two concerns.
Motivation
The way the extra properties were programmed prevents to remove some
project.afterEvaluatecalls. And also prevents to migrate to convention plugins (these are ultimately pre-compiled script plugins).Additional Notes
At this time the logic remains very similar, and focuses on the refactoring from extra properties (
ext { }) to extensions to avoid too many change.However, in the future it might be interesting to revisit the way the JDK is specified, from
JavaVersionto something more toolchain aware viaorg.gradle.jvm.toolchain.JavaToolchainSpec.Some projects still rely on regular Gradle directives to set the sourceset. These will be migrate in a follow-up PR :
Contributor Checklist
type:and (comp:orinst:) labels in addition to any useful labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]