Support instrumentations that make structural changes and tolerate already-loaded target classes - #12610
Support instrumentations that make structural changes and tolerate already-loaded target classes#12610mcculls wants to merge 14 commits into
Conversation
…erate already-loaded target classes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This in turn revealed a gap in KafkaIastDeserializerTest when checking pass-through byte-buffers
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ed class is not structurally changed
mcculls
added
tag: ai generated
🟢 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. |
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
|
Kafka / consumer-benchmarkParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 2 metrics, 0 unstable metrics.
See unchanged results
|
![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: 058842ae68
ℹ️ 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".
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Please try again by commenting @autotest review.
amarziali
left a comment
amarziali
left a comment
There was a problem hiding this comment.
looks clean to me. Thanks for the improvement and for having updated the docs
| if (ENABLE_ADVICE_TRANSFORMER) { | ||
| transformer.applyAdvice(new TaintableVisitor(instrumentedType())); | ||
| } | ||
| transformer.applyAdvice(new TaintableVisitor(instrumentedType())); |
There was a problem hiding this comment.
I like the simplification of not needing the if.
I wonder if we could do the same with TaintableVisitor, too.
There was a problem hiding this comment.
Thanks, could you elaborate on the second point? The ENABLE_ADVICE_TRANSFORMER flag was a workaround for testing because the old approach interacted badly with the tests. The new approach doesn't need such a flag.
But TaintableVisitor is still required because that's how the Taintable interface gets applied.
There was a problem hiding this comment.
I'm just saying that TaintableVisitor seems to be a constant throughout the IAST code, so maybe, we could figure out a way to add it implicitly rather than explicitly. But it isn't in enough places that I feel strongly about it.
There was a problem hiding this comment.
oh, I see - yes there is scope for further cleanup/consolidation there
| int structuralTransformationId = allocateRuntimeTransformationId(); | ||
| matchers.add(new MatchRecorder.CopyMatch(transformationId, structuralTransformationId)); | ||
| addStructuralNarrowing(structuralChange, structuralTransformationId); | ||
| structuralChange.typeAdvice(this); |
There was a problem hiding this comment.
When an instrumentation implements both WithStructuralChange and HasMethodAdvice and gets split into two transformations here, the split-out structural-change transformation never receives the shared helperTransformer/contextRequestRewriter — they're added to advice earlier in buildTypeAdvice, but finishAdviceStack(transformationId) clears advice before structuralChange.typeAdvice(this) runs here.
None of the three instrumentations migrated in this PR trip this (their typeAdvice() only applies a raw AsmVisitorWrapper), so it's not a regression from this change specifically, but it's a latent gap in the new split path: any future dual-interface instrumentation whose type-level advice relies on injected helper classes or context-store rewriting would silently lose that support on the structural-change transformation, likely surfacing as a hard-to-trace NoClassDefFoundError/NoSuchFieldError at instrumentation time. Might be worth re-adding helperTransformer/contextRequestRewriter to advice before this call, or documenting that typeAdvice() on the structural-change path can't rely on them.
There was a problem hiding this comment.
Type advice cannot rely on helpers or context rewriting because it acts at a different level - those concepts are specific to method advice.
I'll add a small note to make this clear.
There was a problem hiding this comment.
That was Claude's analysis. I wasn't sure myself, so I passed it along.
…cted helper classes or context-stores
ygree
left a comment
ygree
left a comment
There was a problem hiding this comment.
LGTM. I was missing the motivation part, which I’d summarize as follows:
Make structural instrumentation safe, regardless of the class-loading order. Apply additions on initial load. Preserve them during retransformation. Skip them for classes loaded without them. This prevents JVM retransformation failures while keeping method advice active and avoiding the global disabling of IAST structural instrumentation.
This branch has not been deployed
5 participants
Summary
Instrumenter.WithStructuralChange, a marker interface for instrumentations whose type advice adds fields, methods, or interfaces to a target type. Each such instrumentation declares astructuralChangeMarker()interface that gets woven into the type when the structural change is applied.MatchRecorder.PreserveLoadedStructure, which narrows a match so an already-loaded class is only retransformed if it already declares the structural-change marker — avoidingVerifyError/redefinition failures from applying structural bytecode changes to classes the JVM has already loaded, while still allowing retransformation of classes that were already structurally changed pre-load.MatchRecorder.CopyMatchto reuse an already-computed match under another id instead of re-running the type matcher.Taintablevisitors,TaskUnwrappingInstrumentation, Vert.xRequestImplInstrumentation) onto the newWithStructuralChangepattern, removing the now-redundantTaintableRedefinitionStrategyListener,MrReturnAdvice, andLoadedTaintableClass.TaintableVisitor,UnwrappingVisitor, andRequestImplInstrumentation, extracting a sharedCollectionUtils.arrayContainshelper.TaintedMapbug where the last element wasn't deduplicatedENABLE_ADVICE_TRANSFORMERtest workaround, no longer needed now that structural-change retransformation is handled properly.PreserveLoadedStructureRetransformationForkedTestcovering: structural changes surviving retransformation, and an already-loaded (not structurally changed) class correctly not being retransformed.Test plan
PreserveLoadedStructureRetransformationForkedTestCollectionUtilsTestcovers the newarrayContainsutility🤖 Generated with Claude Code