-
Notifications
You must be signed in to change notification settings - Fork 324
Remove remaining use of JAVA_X_HOME
#10340
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
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 58 metrics, 7 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.59.0-SNAPSHOT~e6645d3778, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.084 s) : 0, 1084270
Total [baseline] (10.799 s) : 0, 10799345
Agent [candidate] (1.087 s) : 0, 1086922
Total [candidate] (10.847 s) : 0, 10847491
section appsec
Agent [baseline] (1.265 s) : 0, 1265180
Total [baseline] (11.001 s) : 0, 11000617
Agent [candidate] (1.269 s) : 0, 1268894
Total [candidate] (11.09 s) : 0, 11090257
section iast
Agent [baseline] (1.222 s) : 0, 1221798
Total [baseline] (11.18 s) : 0, 11179934
Agent [candidate] (1.225 s) : 0, 1225441
Total [candidate] (11.244 s) : 0, 11243944
section profiling
Agent [baseline] (1.206 s) : 0, 1205651
Total [baseline] (10.918 s) : 0, 10917717
Agent [candidate] (1.207 s) : 0, 1207053
Total [candidate] (10.943 s) : 0, 10943204
gantt
title petclinic - break down per module: candidate=1.59.0-SNAPSHOT~e6645d3778, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.189 ms) : 0, 1189
crashtracking [candidate] (1.182 ms) : 0, 1182
BytebuddyAgent [baseline] (650.239 ms) : 0, 650239
BytebuddyAgent [candidate] (652.557 ms) : 0, 652557
GlobalTracer [baseline] (282.939 ms) : 0, 282939
GlobalTracer [candidate] (283.381 ms) : 0, 283381
AppSec [baseline] (32.583 ms) : 0, 32583
AppSec [candidate] (32.647 ms) : 0, 32647
Debugger [baseline] (68.391 ms) : 0, 68391
Debugger [candidate] (68.07 ms) : 0, 68070
Remote Config [baseline] (634.338 µs) : 0, 634
Remote Config [candidate] (637.109 µs) : 0, 637
Telemetry [baseline] (8.949 ms) : 0, 8949
Telemetry [candidate] (9.112 ms) : 0, 9112
Flare Poller [baseline] (3.841 ms) : 0, 3841
Flare Poller [candidate] (3.816 ms) : 0, 3816
section appsec
crashtracking [baseline] (1.179 ms) : 0, 1179
crashtracking [candidate] (1.197 ms) : 0, 1197
BytebuddyAgent [baseline] (691.092 ms) : 0, 691092
BytebuddyAgent [candidate] (695.287 ms) : 0, 695287
GlobalTracer [baseline] (258.856 ms) : 0, 258856
GlobalTracer [candidate] (258.588 ms) : 0, 258588
AppSec [baseline] (173.85 ms) : 0, 173850
AppSec [candidate] (173.379 ms) : 0, 173379
Debugger [baseline] (66.22 ms) : 0, 66220
Debugger [candidate] (66.717 ms) : 0, 66717
Remote Config [baseline] (785.26 µs) : 0, 785
Remote Config [candidate] (776.711 µs) : 0, 777
Telemetry [baseline] (9.376 ms) : 0, 9376
Telemetry [candidate] (9.292 ms) : 0, 9292
Flare Poller [baseline] (3.712 ms) : 0, 3712
Flare Poller [candidate] (3.691 ms) : 0, 3691
IAST [baseline] (24.617 ms) : 0, 24617
IAST [candidate] (24.457 ms) : 0, 24457
section iast
crashtracking [baseline] (1.18 ms) : 0, 1180
crashtracking [candidate] (1.182 ms) : 0, 1182
BytebuddyAgent [baseline] (789.373 ms) : 0, 789373
BytebuddyAgent [candidate] (791.865 ms) : 0, 791865
GlobalTracer [baseline] (256.527 ms) : 0, 256527
GlobalTracer [candidate] (256.687 ms) : 0, 256687
AppSec [baseline] (33.418 ms) : 0, 33418
AppSec [candidate] (35.004 ms) : 0, 35004
Debugger [baseline] (66.399 ms) : 0, 66399
Debugger [candidate] (65.469 ms) : 0, 65469
Remote Config [baseline] (577.646 µs) : 0, 578
Remote Config [candidate] (580.35 µs) : 0, 580
Telemetry [baseline] (8.476 ms) : 0, 8476
Telemetry [candidate] (8.516 ms) : 0, 8516
Flare Poller [baseline] (3.543 ms) : 0, 3543
Flare Poller [candidate] (3.611 ms) : 0, 3611
IAST [baseline] (26.896 ms) : 0, 26896
IAST [candidate] (27.061 ms) : 0, 27061
section profiling
crashtracking [baseline] (1.211 ms) : 0, 1211
crashtracking [candidate] (1.216 ms) : 0, 1216
BytebuddyAgent [baseline] (702.251 ms) : 0, 702251
BytebuddyAgent [candidate] (702.701 ms) : 0, 702701
GlobalTracer [baseline] (221.235 ms) : 0, 221235
GlobalTracer [candidate] (221.701 ms) : 0, 221701
AppSec [baseline] (32.26 ms) : 0, 32260
AppSec [candidate] (32.08 ms) : 0, 32080
Debugger [baseline] (68.502 ms) : 0, 68502
Debugger [candidate] (68.814 ms) : 0, 68814
Remote Config [baseline] (620.411 µs) : 0, 620
Remote Config [candidate] (636.336 µs) : 0, 636
Telemetry [baseline] (9.089 ms) : 0, 9089
Telemetry [candidate] (8.84 ms) : 0, 8840
Flare Poller [baseline] (3.786 ms) : 0, 3786
Flare Poller [candidate] (3.767 ms) : 0, 3767
ProfilingAgent [baseline] (96.732 ms) : 0, 96732
ProfilingAgent [candidate] (97.354 ms) : 0, 97354
Profiling [baseline] (97.333 ms) : 0, 97333
Profiling [candidate] (97.949 ms) : 0, 97949
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.59.0-SNAPSHOT~e6645d3778, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.087 s) : 0, 1086982
Total [baseline] (8.808 s) : 0, 8807589
Agent [candidate] (1.099 s) : 0, 1099104
Total [candidate] (8.818 s) : 0, 8817782
section iast
Agent [baseline] (1.231 s) : 0, 1230858
Total [baseline] (9.374 s) : 0, 9374418
Agent [candidate] (1.23 s) : 0, 1230453
Total [candidate] (9.359 s) : 0, 9358953
gantt
title insecure-bank - break down per module: candidate=1.59.0-SNAPSHOT~e6645d3778, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.217 ms) : 0, 1217
crashtracking [candidate] (1.209 ms) : 0, 1209
BytebuddyAgent [baseline] (652.04 ms) : 0, 652040
BytebuddyAgent [candidate] (660.533 ms) : 0, 660533
GlobalTracer [baseline] (284.264 ms) : 0, 284264
GlobalTracer [candidate] (286.564 ms) : 0, 286564
AppSec [baseline] (33.011 ms) : 0, 33011
AppSec [candidate] (33.215 ms) : 0, 33215
Debugger [baseline] (66.617 ms) : 0, 66617
Debugger [candidate] (68.307 ms) : 0, 68307
Remote Config [baseline] (632.539 µs) : 0, 633
Remote Config [candidate] (630.612 µs) : 0, 631
Telemetry [baseline] (9.078 ms) : 0, 9078
Telemetry [candidate] (8.948 ms) : 0, 8948
Flare Poller [baseline] (4.598 ms) : 0, 4598
Flare Poller [candidate] (3.803 ms) : 0, 3803
section iast
crashtracking [baseline] (1.19 ms) : 0, 1190
crashtracking [candidate] (1.194 ms) : 0, 1194
BytebuddyAgent [baseline] (796.271 ms) : 0, 796271
BytebuddyAgent [candidate] (796.625 ms) : 0, 796625
GlobalTracer [baseline] (258.498 ms) : 0, 258498
GlobalTracer [candidate] (258.154 ms) : 0, 258154
AppSec [baseline] (32.724 ms) : 0, 32724
AppSec [candidate] (35.09 ms) : 0, 35090
Debugger [baseline] (66.629 ms) : 0, 66629
Debugger [candidate] (64.061 ms) : 0, 64061
Remote Config [baseline] (589.527 µs) : 0, 590
Remote Config [candidate] (568.252 µs) : 0, 568
Telemetry [baseline] (8.588 ms) : 0, 8588
Telemetry [candidate] (8.524 ms) : 0, 8524
Flare Poller [baseline] (3.64 ms) : 0, 3640
Flare Poller [candidate] (3.631 ms) : 0, 3631
IAST [baseline] (27.121 ms) : 0, 27121
IAST [candidate] (27.113 ms) : 0, 27113
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 5 performance regressions! Performance is the same for 15 metrics, 16 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.59.0-SNAPSHOT~e6645d3778, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section baseline
no_agent (17.885 ms) : 17699, 18070
. : milestone, 17885,
appsec (18.344 ms) : 18161, 18526
. : milestone, 18344,
code_origins (17.632 ms) : 17457, 17808
. : milestone, 17632,
iast (17.816 ms) : 17639, 17993
. : milestone, 17816,
profiling (18.743 ms) : 18553, 18934
. : milestone, 18743,
tracing (17.826 ms) : 17652, 18001
. : milestone, 17826,
section candidate
no_agent (17.983 ms) : 17800, 18166
. : milestone, 17983,
appsec (19.494 ms) : 19295, 19693
. : milestone, 19494,
code_origins (17.811 ms) : 17635, 17988
. : milestone, 17811,
iast (19.792 ms) : 19594, 19991
. : milestone, 19792,
profiling (18.949 ms) : 18759, 19139
. : milestone, 18949,
tracing (17.598 ms) : 17426, 17770
. : milestone, 17598,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.59.0-SNAPSHOT~e6645d3778, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section baseline
no_agent (1.174 ms) : 1163, 1186
. : milestone, 1174,
iast (3.326 ms) : 3287, 3364
. : milestone, 3326,
iast_FULL (5.731 ms) : 5674, 5789
. : milestone, 5731,
iast_GLOBAL (3.51 ms) : 3460, 3561
. : milestone, 3510,
profiling (2.05 ms) : 2031, 2068
. : milestone, 2050,
tracing (1.823 ms) : 1809, 1837
. : milestone, 1823,
section candidate
no_agent (1.183 ms) : 1171, 1195
. : milestone, 1183,
iast (3.315 ms) : 3270, 3359
. : milestone, 3315,
iast_FULL (5.753 ms) : 5696, 5811
. : milestone, 5753,
iast_GLOBAL (3.681 ms) : 3619, 3743
. : milestone, 3681,
profiling (1.899 ms) : 1883, 1915
. : milestone, 1899,
tracing (1.828 ms) : 1812, 1845
. : milestone, 1828,
DacapoParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 0 unstable metrics.
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.59.0-SNAPSHOT~e6645d3778, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section baseline
no_agent (1.483 ms) : 1472, 1495
. : milestone, 1483,
appsec (3.697 ms) : 3481, 3914
. : milestone, 3697,
iast (2.238 ms) : 2172, 2304
. : milestone, 2238,
iast_GLOBAL (2.279 ms) : 2213, 2346
. : milestone, 2279,
profiling (2.111 ms) : 2055, 2166
. : milestone, 2111,
tracing (2.063 ms) : 2011, 2115
. : milestone, 2063,
section candidate
no_agent (1.476 ms) : 1464, 1487
. : milestone, 1476,
appsec (2.488 ms) : 2435, 2541
. : milestone, 2488,
iast (2.235 ms) : 2169, 2301
. : milestone, 2235,
iast_GLOBAL (2.27 ms) : 2204, 2337
. : milestone, 2270,
profiling (2.113 ms) : 2057, 2169
. : milestone, 2113,
tracing (2.059 ms) : 2007, 2111
. : milestone, 2059,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.59.0-SNAPSHOT~e6645d3778, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section baseline
no_agent (14.958 s) : 14958000, 14958000
. : milestone, 14958000,
appsec (14.667 s) : 14667000, 14667000
. : milestone, 14667000,
iast (17.943 s) : 17943000, 17943000
. : milestone, 17943000,
iast_GLOBAL (17.823 s) : 17823000, 17823000
. : milestone, 17823000,
profiling (15.074 s) : 15074000, 15074000
. : milestone, 15074000,
tracing (14.896 s) : 14896000, 14896000
. : milestone, 14896000,
section candidate
no_agent (15.403 s) : 15403000, 15403000
. : milestone, 15403000,
appsec (14.786 s) : 14786000, 14786000
. : milestone, 14786000,
iast (17.723 s) : 17723000, 17723000
. : milestone, 17723000,
iast_GLOBAL (18.069 s) : 18069000, 18069000
. : milestone, 18069000,
profiling (14.748 s) : 14748000, 14748000
. : milestone, 14748000,
tracing (14.904 s) : 14904000, 14904000
. : milestone, 14904000,
|
a1e732b to
e6e93dd
Compare
…A_*_HOME env vars
ad686ad to
bea5f86
Compare
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.
Wow this is cool! Left a few small wording nits, but otherwise looks good!
I was wondering -- how were you able to test this? My guess is by removing local Java installations and confirming that non-JDK 21 tests can still run.
| } | ||
|
|
||
| // "stable" is calculated as the largest X found in JAVA_X_HOME | ||
| // "stable" is calculated as the largest Java version found via toolchains or JAVA_X_HOME |
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.
misc: I still do not like stable name... can we rename it to latest or something similar?
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'd rather not change it in this PR.
AlexeyKuznetsov-DD
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.
LGTM. Just have a question - would it be possible to have JVMs from various vendors to test for example Oracle 8 and Zulu on my laptop?
PerfectSlayer
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.
Erf, I just notice my review was left in draft. Submitting now
docs/how_instrumentations_work.md
Outdated
| ``` | ||
|
|
||
| Running tests that require JDK-21 will require the `JAVA_21_HOME` env var set and can be done like this: | ||
| Running tests that require JDK-21 to be installed, use the `-PtestJvm=21` flag, for example: |
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 will it work to run with a specific JVM?
For JDK version based criteria, we can set the version number. But what happens for the case where you want to run against a JDK installed in a specific path?
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.
The toolchain will be automatically downloaded with a number or (zulu11) e.g. :
// -------------> 1% CONFIGURING [10s]
// > :dd-java-agent > :dd-java-agent:agent-iast > Downloading toolchain from URI https://api.foojay.io/disco/v3.0/ids/dc9716d3e8f8baffc294fb182ceca2c2/redirect > https://api.foojay.io/disco/v3.0/ids/dc9716d3e8f8baffc294fb182ceca2c2/redirect > 80 MiB/104.5 MiB downloaded
That said provisioning won't work oracle as it cannot be provisonned automatically, so it has to be installed manually in a location that Gradle know about.
About JVM paths, this was already handled with the dd-trace-java.test-jvm-contraints plugin, so passing -PtestJvm=/path/to/fancy/jvm/ will use that jvm for tests.
dd-trace-java/buildSrc/src/main/kotlin/datadog/gradle/plugin/testJvmConstraints/TestJvmSpec.kt
Lines 159 to 161 in f13392e
| private val Project.javaToolchains: JavaToolchainService | |
| get() = | |
| extensions.getByName("javaToolchains") as JavaToolchainService |
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.
Interesting, it's indeed the expected behavior.
This should be described from the documentation as it is a recurrent question and it just changes.
CONTRIBUTING.md
Outdated
| For IntelliJ IDEA, we suggest the following settings and plugin. | ||
|
|
||
| The default JVM to build and run tests from the command line should be Java 8. | ||
| The default JVM to build and run tests from the command line should be Java 21. |
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: Is this still needed? Won't it be picked automatically?
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.
Your comment made me realize the gradle-daemon-jvm.properties was missing something for this to work. Next commit fixes that.
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'm removing this line as it shouldn't be a concern anymore.
BUILDING.md
Outdated
| #### macOS | ||
|
|
||
|
|
||
| Use your JDK manager ([mise](https://mise.jdx.dev/), [sdkman](https://sdkman.io/), etc.) or set-up the required JDKs with `brew` |
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.
💭 thought: This is confusing as the next section gives explanations for brew. We choose to explain the brew way as it is the simplest as already available tools for most MacOS users not accustomed to Java development. For the others, they already know how to install a JDK and use their preferred tool.
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 reworded
| check-jvm "JAVA_21_HOME" "21" | ||
| check-jvm "JAVA_25_HOME" "25" | ||
| check-jvm "JAVA_GRAALVM17_HOME" "17" | ||
| echo "ℹ️ Other JDK versions will be automatically downloaded by Gradle toolchain resolver." |
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.
Looks like some part of my review were lost at some point but I had a comment about the onboarded script that was lost.
Here is a follow up PR to address what I originally posted: #10384
We can proceed with yours first and I will update / resolve conflict on mine after.
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.
Actually, is this even necessary as JAVA_HOME is not strictly required anymore ?
| return launcher | ||
| } as Closure<Provider<JavaLauncher>> | ||
|
|
||
| ext.getLazyJavaHomeFor = (int javaVersionInteger, boolean hasNativeImage = false) -> { |
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.
note: Eventually this should be moved to the test-jvm-contraints plugin.
Just use |
URLs are acquired from the foojay resolver, when running this task ./gradlew updateDaemonJvm --jvm-version=21 See https://docs.gradle.org/8.14.3/userguide/gradle_daemon.html#configuring_the_daemon_jvm
fd1d2a7 to
e55a0a4
Compare
What Does This Do
This PR allows to completely get rid of setting up JAVA_X_HOME on local dev environment.
The
stablespecial word has special handling though, that is using Gradle internal toolchain registry, and fallback to env vars. However, this mainly affect CI.Motivation
Easier project setup
Additional Notes
After this PR, setting up JAVA_X_HOME won't be needed anymore. One can use
-PtestJvm=..., e.g.-PtestJvm=11,-PtestJvm=graalvm17, ...