Conversation
UnwrappingVisitor adds the TaskWrapper interface to the classes it instruments. Adding an interface is a structural change, which the JVM rejects on retransformation, so this only works because those classes are normally still unloaded when the agent installs. A JDK 24+ AOT cache (JEP 483) breaks that assumption: the cached classes are materialized before premain runs, so the retransformation batch that contains them fails. There is no BatchAllocator configured, so that is the single batch holding every class, and the failure aborts the whole install - the tracer reports itself as healthy but no instrumentation is applied. Clear the flag on failure and retry the batch once, so queueing-time profiling is dropped instead of all instrumentation. Co-Authored-By: Claude Opus 5 <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78d65771a4
ℹ️ 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".
| if (enabledSystems.contains(InstrumenterModule.TargetSystem.IAST)) { | ||
| return TaintableRedefinitionStrategyListener.INSTANCE; | ||
| } else { | ||
| return AgentBuilder.RedefinitionStrategy.Listener.NoOp.INSTANCE; | ||
| return TaskWrapperRedefinitionStrategyListener.INSTANCE; |
There was a problem hiding this comment.
When IAST is enabled on a JDK 24+ AOT-cache launch, this branch installs only TaintableRedefinitionStrategyListener, so the new recovery path never runs. The first failed batch disables TaintableVisitor and is retried, but UnwrappingVisitor.ENABLED remains true; the retry therefore makes the same prohibited TaskWrapper interface change, after which the IAST listener returns no further batches and all instrumentation is still dropped. Use a combined listener (or teach the IAST listener to disable both structural visitors) so this configuration also recovers.
Useful? React with 👍 / 👎.
Mirror TaintableRedefinitionStrategyListener: same debug logging and the same onComplete hook, so both structural visitors behave the same way. Chain the two listeners when IAST is enabled. Both visitors change the structure of a class, so both need a chance to back off; previously only the IAST one ran and an AOT cache still took the whole install down. Add a test asserting that neither the interface nor the generated unwrap method is emitted once the visitor is disabled. Co-Authored-By: Claude Opus 5 <[email protected]>
Silent feature loss was the reported problem, so the one-time recovery notice is logged at info instead of debug. The flag is one-way, so the message cannot repeat; the per-batch retry and completion logs stay at debug to keep startup logs quiet.
|
Pushed 205064e: the one-time notice when queueing time profiling is disabled is now logged at info instead of debug. Since the silent failure was the core problem here, dropping a feature without any visible trace felt like keeping half of it. The flag is one-way so the message cannot repeat, and the per-batch retry/completion logs stay at debug to keep startup logs quiet. Happy to move it back to debug if you prefer consistency with the IAST listener. |
Clearing the flag does not turn queueing-time profiling off. TaskWrapper is only read by QueueTimeEvent.setTask(), and TaskWrapper.getUnwrappedType is guarded by instanceof, so without the interface it returns the object's own class. The event is still recorded with its duration, scheduler, queue type, queue length and span ids -- only the reported task type loses its resolution. Measured with -Ddd.profiling.debug.dump_path and -Ddd.profiling.queueing.time.threshold.millis=0, 500 requests: AOT off 12 datadog.QueueTime, real task types AOT on (flag cleared on retry) 7 datadog.QueueTime, wrapper type queueing.time.enabled=false 0 datadog.QueueTime Co-Authored-By: Claude Opus 5 <[email protected]>
The previous commit renamed the info message and javadoc, but the debug message on the second failure still said queueing time profiling is disabled. It is not: only task unwrapping is.
2 participants
What Does This Do
UnwrappingVisitoradds theTaskWrapperinterface to the classes itinstruments. When that retransformation batch fails, the newly added
TaskWrapperRedefinitionStrategyListenerclears the visitor's flag andretries the batch once, so only the unwrapping is dropped instead of all
instrumentation.
Motivation
Adding an interface is a structural change, which the JVM rejects on
retransformation. This normally works because the affected classes are
still unloaded when the agent installs.
A JDK 24+ AOT cache (JEP 483) breaks that assumption: cached classes are
materialized before
premainruns, so the batch containing them fails.There is no
BatchAllocatorconfigured, so that is the single batchholding every class, and
RedefinitionStrategyaborts the whole install.The failure mode is silent. The tracer starts, prints its configuration
and reports
agent_error: false, but not a single span is produced anddd.trace_id/dd.servicenever reach the logs. Only-Ddd.trace.debug=truerevealsException while retransforming 574 classes. We spent two days finding that-Ddd.profiling.queueing.time.enabled=falseworks around it.Fixes #12540. Related to #10479.
Additional Notes
Measured on a Spring Boot 4 service (JDK 25, agent 1.66.0), 20 requests
after startup, two runs per row:
dd.servicein logsqueueing.timeoffqueueing.timeonWith an AOT cache the batch now fails once and succeeds on retry, and the
result is identical to disabling queueing time by hand. Without an AOT
cache the flag is never cleared and the numbers match the current
behaviour exactly.
Queueing-time profiling itself keeps working.
TaskWrapperis only readby
QueueTimeEvent.setTask(), andgetUnwrappedTypeis guarded byinstanceof, so without the interface it simply returns the wrapper's ownclass. Same run, JFR dumped with
-Ddd.profiling.debug.dump_pathand-Ddd.profiling.queueing.time.threshold.millis=0:datadog.QueueTimetaskfieldAsyncCommand,DnsNameResolver$8, ...)PromiseTask)queueing.timeoffSo the trade is narrower than turning the feature off: the durations,
scheduler, queue type, queue length and span ids are all still recorded,
only the task type loses its resolution.
With an AOT cache the listener logs one line at info, so the degradation is
visible at the default log level (
Disabling task unwrapping for queueing time profiling: ...). The per-batch retry logs stay at debug.A framework-free reproducer is attached to #12540. It uses the documented
setup (plain
-javaagenton both the training and the production run) andfails the same way with the explicit
=aot_trainingargument.Two limitations worth calling out:
lose
TaskWrapper. With an AOT cache the batch fails on the firstattempt, so in practice this is all-or-nothing anyway, but a more
targeted fix would drop only the classes that cannot be retransformed.
change the structure of a class.
Compound.onErrorconcatenates theretry batches, so that combination retransforms the batch once more
than strictly needed. It is a one-off cost at startup, but say the word
if you would rather have the two flags cleared by a single listener.
This does not restore task unwrapping under an AOT cache -- the JVM
restriction stands. It stops one unusable visitor from taking the rest of
the tracer down with it, and keeps queueing-time profiling itself alive.
I cannot apply labels as an outside contributor.
type: bug fixandcomp: profilinglook like the right ones.