fix computeFrames call#11920
Conversation
make sure it is called before any instrumentation. for CaptureContextInstrumenter, processInstructions is called after some instrumentation at the beginning of the method. so extract computeFrames outside of processInstructions and return directly the frames Map.
|
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.
More details
Reviewed PR moving frame computation before instrumentation in debugger's bytecode processing. All call sites correctly updated across five instrumenter classes, signatures changed consistently, and no behavioral regressions identified. The fix addresses a real problem where analyzing instrumented bytecode could fail, by computing frames on the original bytecode first.
🤖 Datadog Autotest · Commit 6599436 · What is Autotest? · Any feedback? Reach out in #autotest
This comment has been minimized.
This comment has been minimized.
🟢 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. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts debugger probe bytecode instrumentation to ensure ASM frame analysis (computeFrames) runs before any instruction list mutations, then passes the precomputed frames into processInstructions for return-rewrite/stack-cleanup logic.
Changes:
- Refactored
Instrumenter.computeFramesto return the computed frame map, and updatedprocessInstructionsto accept it as an argument. - Updated Span/Metric/Exception/CapturedContext instrumenters to compute frames up-front and pass them into
processInstructions. - Improved the
AnalyzerExceptiondebug log to include the method descriptor.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/SpanInstrumenter.java | Precomputes frames before span instrumentation and passes them to processInstructions. |
| dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumenter.java | Precomputes frames for EXIT instrumentation before rewriting returns. |
| dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/Instrumenter.java | Refactors frame computation API and updates instruction processing to take precomputed frames; enhances debug logging. |
| dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ExceptionInstrumenter.java | Precomputes frames before processing instructions to set up return handling. |
| dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumenter.java | Computes frames before early method-entry instrumentation, then uses them for return processing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected static Map<AbstractInsnNode, Frame<BasicValue>> computeFrames( | ||
| String owner, MethodNode methodNode) { |
What Does This Do
make sure it is called before any instrumentation. for CaptureContextInstrumenter, processInstructions is called after some instrumentation at the beginning of the method. so extract computeFrames outside of processInstructions and return directly the frames Map.
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: DEBUG-5816