Conversation
|
@DataDog review |
There was a problem hiding this comment.
On JDK 8 and JDK 11, the new structure matcher requires a field that does not exist. The lambda transformer does not run, and Runnable lambdas still use wrappers.
🤖 Datadog Autotest · Commit 054841c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
054841c to
5742c20
Compare
|
@DataDog review |
There was a problem hiding this comment.
More details
The PR instruments generated Runnable lambdas so executor propagation keeps task identity, with supported JDK paths and a Graal build guard. This update fixes the earlier JDK 8 and 11 samBase mismatch and adds no new finding; the documented concurrent-reuse limitation remains, and no review finding is open.
🤖 Datadog Autotest · Commit 5655a6f · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🟢 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. |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
|
@DataDog review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a721063dff
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
A hidden Runnable lambda can put its type description in the shared name cache. A later real class with the same legal binary name can receive wrong instrumentation on JDK 21 and later.
🤖 Datadog Autotest · Commit a721063 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
@DataDog review |
There was a problem hiding this comment.
The new module has no Gradle lock file. Its dependency graph can change between builds.
🤖 Datadog Autotest · Commit b11c8c6 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
bric3
left a comment
There was a problem hiding this comment.
It looks good overall, but I think some parts need to be adjusted a bit, e.g. in the AgentInstaller.
I made a flow diagram to actually try to understand what this will provide ... when we have ForLambda instrumentation? Is my understanding correct ?
flowchart TB
subgraph install["1 · Install lambda instrumentation"]
direction TB
A["Collect enabled <code>Instrumenter.ForLambda</code> implementations"]:::tracer
B["<code>AgentBuilder</code> builds the decorated <code>ClassFileTransformer</code>"]:::third
C["<code>onBeforeInstall</code> publishes the transformer to <code>LambdaTransformerHolder</code>"]:::tracer
D["Register the transformer and retransform loaded <code>InnerClassLambdaMetafactory</code>"]:::jdk
E["Inject a pre-definition call to <code>LambdaTransformerHelper.transform(...)</code> into <code>InnerClassLambdaMetafactory</code>"]:::tracer
A --> B --> C --> D --> E
end
subgraph generate["2 · Prepare the lambda implementation (linkage) and invoke the lambda"]
direction TB
F["Application or library first executes an unlinked lambda <code>invokedynamic</code> call site"]:::third
G["<code>InnerClassLambdaMetafactory</code> generates the lambda implementation bytes"]:::jdk
H{"Can the agent transform this lambda?"}:::tracer
O["Return the original generated bytes"]:::tracer
F --> G --> H
H -->|no| O
subgraph transform["Transform before definition"]
direction TB
I["Call the Byte Buddy transformer with the declaring class's module, loader and protection domain"]:::tracer
J["Byte Buddy runs Datadog's matching and transformation pipeline"]:::third
K["Select <code>ForLambda</code> transformations by exact interface, lambda matcher, class-loader matcher and muzzle compatibility"]:::tracer
L["Apply advice, inject helpers and add eligible context fields"]:::tracer
M["Run cleanup: <code>SharedTypePools.endTransform(...)</code> releases owned type-pool state, then clear per-thread lambda markers"]:::tracer
I --> J --> K --> L --> M
J -. no change or failure .-> M
end
N["<code>InnerClassLambdaMetafactory</code> defines the lambda class from the returned bytes"]:::jdk
P["Application invokes the lambda; injected behavior runs if a <code>ForLambda</code> transformation matched"]:::third
H -->|yes| I
M -->|transformed or original bytes| N
O --> N
N --> P
end
%% Connect groups to preserve their top-to-bottom direction within the left-to-right layout.
install -. transformer published and generation hook installed .-> generate
style install fill:#f3e8ff,stroke:#9333ea,stroke-width:2px,color:#581c87
style generate fill:#ecfeff,stroke:#0891b2,stroke-width:2px,color:#164e63
style transform fill:#fff1f2,stroke:#e11d48,stroke-width:2px,color:#881337
classDef jdk fill:#dbeafe,stroke:#2563eb,color:#172554
classDef tracer fill:#dcfce7,stroke:#16a34a,color:#14532d
classDef third fill:#ffedd5,stroke:#ea580c,color:#7c2d12
| Class<?> classBeingRedefined, | ||
| ProtectionDomain pd) { | ||
|
|
||
| String lambdaInterface = lambdaMatchers.isEmpty() ? null : TypePoolFacade.lambdaInterface(); |
There was a problem hiding this comment.
could we find a quicker way to shortcut this when the feature is not installed?
I'd like to avoid repeatedly calling HashMap.isEmpty() during premain when we know this will never be false - we have a static config flag gating this feature which we could store as a static local constant
|
|
||
| static BitSet memoizeHierarchy(TypeDescription type, Map<String, BitSet> localMemos) { | ||
| if (noMatchFilter.contains(type.getName())) { | ||
| if (isCacheable(type) && noMatchFilter.contains(type.getName())) { |
There was a problem hiding this comment.
noMatchFilter is designed to be a very quick filter - I'm concerned that we're adding an instanceOf and method call to isCacheable here before it.
|
|
||
| long fromTick = InstrumenterMetrics.tick(); | ||
| SharedTypeInfo<BitSet> sharedMemo = memos.find(name); | ||
| SharedTypeInfo<BitSet> sharedMemo = cacheable ? memos.find(name) : null; |
There was a problem hiding this comment.
I'm struggling a bit to work out how isCacheable layers on top of memoization.
Also note that this code will very soon move out to dd-instrument-java, where it will be memoization of a more general matching process (i.e. outside of byte-buddy.)
Is this isCacheable property something that I'll need to also move over, or is it specific to how we use byte-buddy?
There was a problem hiding this comment.
Good question. The goal was to avoid reusing cached matcher results for generated lambda classes. They are parsed from temporary in-memory bytes, and the same class name may later refer to different bytecode, so a cached result could be stale. In 94d2bd4, TypeFactory.isLambdaTarget(...) identifies that specific target directly, and the shared type and matcher caches skip it. So this is specific to this lambda transformation path so IMHO I don't think that there’s any general property that needs to move to dd-instrument-java.
There was a problem hiding this comment.
ok, are these names like lambda names with slashes? or can they re-use general class names?
There was a problem hiding this comment.
Yes, the definition mechanism varies by JDK: older JDKs use anonymous classes and newer JDKs use hidden classes. In both cases our hook runs before definition, so it sees the nominal bytecode name without the runtime /... suffix.
| * Cleans up local caches if this callback owns the active transformation. Reference identity is | ||
| * the ownership token, so transformer wrappers must pass the original callback buffer unchanged. | ||
| */ | ||
| void endTransform(byte[] classFileBuffer) { |
There was a problem hiding this comment.
Quick question - is this needed because we're now nesting transform calls?
There was a problem hiding this comment.
Yes as far as I could see, while transforming one class, class loading can trigger another transformation callback on the same thread. I remember having had failing tests. Byte Buddy blocks the inner callback from doing a full transformation, but its wrapper still runs the usual cleanup afterward (endTransform). That cleanup clears the temporary TypeFactory state used to describe the class currently being transformed. Without this check, the inner callback could accidentally clear the outer transformation’s state before it has finished.
This is documented in 5feb161 and tested in 790a7a8
There was a problem hiding this comment.
Thanks - I suspect it could be a side-effect of how we're plugging into the metafactory
| } | ||
|
|
||
| @Override | ||
| public boolean isCacheable() { |
There was a problem hiding this comment.
This property is quite invasive, causing changes across a lot of the type caching layer - I'm thinking there might be a better way to do this with less changes (especially as I want to move parts of this to the dd-instrument-java library)
mcculls
left a comment
There was a problem hiding this comment.
I like the idea, but would like to see this more of a "no-op" when the feature is not enabled.
There are also some changes in code which will soon move out of this codebase, and we'll need to decide whether those changes will still be required.
jordan-wong
left a comment
There was a problem hiding this comment.
LGTM, other than 1 test I would add for coverage
| !Platform.isNativeImageBuilder() | ||
| && InstrumenterConfig.get() | ||
| .isIntegrationEnabled(Collections.singleton("lambda"), false); | ||
| if (lambdaTransformationEnabled) { |
There was a problem hiding this comment.
I would consider adding a regression/ unit test for lambdaTransformationEnabled
PR description mentions
Skips the mechanism during GraalVM Native Image builds.
as a noteable feature
What Does This Do?
Adds opt-in infrastructure for applying Datadog instrumentations to classes generated by
LambdaMetafactory.Generated lambda classes are normally defined without passing through the standard Java agent transformation path. This change instruments
InnerClassLambdaMetafactoryat the byte-generation point and routes eligible lambda bytecode through the existing transformer before class definition.The infrastructure is disabled by default and can be enabled with:
Enabling it alone does not select any production lambda classes. An enabled instrumentation must explicitly implement
Instrumenter.ForLambdaand register the exact functional interface it supports.Notable Changes
Instrumenter.ForLambdafor instrumentations targeting generated implementations of an exact functional interface.java.lang.invoke.InnerClassLambdaMetafactoryimmediately after lambda bytecode generation.ClassFileAPI path used by newer JDKs.ClassFileTransformerentry point on JDK 9+.Compatibility and Limitations
Lambda transformation is intentionally best-effort:
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]