Update MethodParameters detection for fixed JDK#12025
Update MethodParameters detection for fixed JDK#12025gh-worker-dd-mergequeue-cf854d[bot] merged 3 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb6a47891e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ClassNode classNode, List<ProbeDefinition> definitions, String fullyQualifiedClassName) { | ||
| if (JAVA_AT_LEAST_19) { | ||
| // bug is fixed since JDK19, no need to perform check | ||
| if (JAVA_AT_LEAST_19 || JAVA_AT_LEAST_17_0_20) { |
There was a problem hiding this comment.
Gate the 17.0.20 shortcut to Java 17
On Java 18, JavaVirtualMachine.isJavaVersionAtLeast(17, 0, 20) is true because it compares the major version first, even though the adjacent comment says this JDK bug is fixed only in JDK 19 and in the 17.0.20 backport. In Java 18 Spring 6 apps with -parameters classes, this makes checkMethodParameters return early and allows retransformation to strip the MethodParameters attribute again; the same predicate was added in ConfigurationUpdater, so please combine the backport check with isJavaVersion(17) while keeping the JDK 19+ shortcut.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
true, but JDK18 is marginal, don't bother
There was a problem hiding this comment.
@jpbempel, I disagree. The version detection should be correct, and it would not be rocket science to implement it correctly.
There was a problem hiding this comment.
JDK18 is not supported anymore by any vendors
JDK18 represents a very very marginal number for dd-trace-java installations.
There was a problem hiding this comment.
You mentioned that already. IMO, fixing the condition would still be quicker than explaining why you won't fix it. Besides, it would be cleaner, too, and the code would be nicely self-documenting.
| int result = Reflect.onClass(testClass).call("main", "1").get(); | ||
| assertEquals(3, result); | ||
| if (JavaVirtualMachine.isJavaVersion(17)) { | ||
| if (JavaVirtualMachine.isJavaVersionBetween(17, 0, 0, 17, 0, 19)) { |
There was a problem hiding this comment.
Include Java 17.0.19 in the affected test range
On Java 17.0.19 this branch is false because isJavaVersionBetween(..., 17, 0, 19) uses an exclusive upper bound, but the fix being modeled is 17.0.20 and the setup above still loads the Spring class for any Java 17. The transformer will therefore reject the MethodParameters instrumentation on 17.0.19 while the test expects a snapshot, so this should use 17.0.20 as the exclusive upper bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
More details
PR correctly updates Method Parameters detection to skip checks on JDK 17.0.20+ where the OpenJDK bug (JDK-8240908) is fixed, alongside existing JDK 19+ handling. Version logic is correct and tests properly scope expectations. Significant scope creep includes unrelated annotation parameter reordering and benchmark deletion, but these changes are cosmetic and pose no behavioral risk.
📊 Validated against 11 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit cb6a478 · What is Autotest? · Any feedback? Reach out in #autotest
|
🎯 Code Coverage (details) 🔗 Commit SHA: 6e248fb | Docs | Datadog PR Page | Give us feedback! |
Debugger benchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 9 metrics, 6 unstable metrics. See unchanged results
Request duration reports for reportsgantt
title reports - request duration [CI 0.99] : candidate=None, baseline=None
dateFormat X
axisFormat %s
section baseline
noprobe (354.325 µs) : 284, 425
. : milestone, 354,
basic (297.95 µs) : 289, 307
. : milestone, 298,
loop (8.995 ms) : 8989, 9001
. : milestone, 8995,
section candidate
noprobe (333.798 µs) : 302, 366
. : milestone, 334,
basic (295.761 µs) : 289, 303
. : milestone, 296,
loop (8.989 ms) : 8984, 8994
. : milestone, 8989,
|
evanchooly
left a comment
There was a problem hiding this comment.
looks like a repeat comment/concern on the version checks but otherwise lgtm
| List<Class<?>> changedClasses) { | ||
| if (JAVA_AT_LEAST_19) { | ||
| // bug is fixed since JDK19, no need to perform detection | ||
| if (JAVA_AT_LEAST_19 || JAVA_AT_LEAST_17_0_20) { |
There was a problem hiding this comment.
nit: could we just check for the new version? checking for java 19 feels redundant now.
There was a problem hiding this comment.
What? No! The correct condition is something like
if (JAVA_AT_LEAST_19 || JAVA_AT_LEAST_17_0_20 && !JAVA_AT_LEAST_18) {Why not just make it correct? Why did you choose incorrect heuristics instead of this simple, correct, efficient and not significantly more complicated solution?
JDK 17.0.20 fixes the Method Parameters bug (JDK-8240908) so we are updating the detection to filter out for this JDK version and onward # Conflicts: # dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
8271c3a to
6e248fb
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do
JDK 17.0.20 fixes the Method Parameters bug (JDK-8240908) so we are updating the detection to filter out for this JDK version and onward
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]