-
Notifications
You must be signed in to change notification settings - Fork 355
Instrument LambdaMetafactory and preserve Runnable lambda identity during context propagation #12346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
amarziali
wants to merge
18
commits into
master
Choose a base branch
from
andrea.marziali/lambda
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Instrument LambdaMetafactory and preserve Runnable lambda identity during context propagation #12346
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
ec68f8e
wip
amarziali 5318053
perf test
amarziali 938c2a5
Enforce type checks
amarziali 0f6dfee
Correctly manage JPMS
amarziali e34f5fc
match the call owner
amarziali b443c58
Improve smoke test
amarziali 52137c8
Better logs
amarziali 3bbe48b
Bind agent jar to jmh
amarziali 7af9fc4
Switch to default asm version
amarziali 81421d4
skip the cache
amarziali c2c7192
add a guard on runnable
amarziali 11d0748
Better JMH
amarziali 885ffac
refinements
amarziali 422b951
Enable by default
amarziali 6f9490f
update integration golden file
amarziali 5742c20
Disable instrumenting lambdas during graal builds
amarziali 5655a6f
Support java 8/11 factories
amarziali a721063
Improve smoke test
amarziali File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
12 changes: 12 additions & 0 deletions
12
...main/java/datadog/trace/bootstrap/instrumentation/java/lang/invoke/LambdaTransformer.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package datadog.trace.bootstrap.instrumentation.java.lang.invoke; | ||
|
|
||
| /** Transforms a generated lambda class before it is defined. */ | ||
| public interface LambdaTransformer { | ||
| /** | ||
| * @param slashClassName internal (slash-separated) name of the generated lambda class | ||
| * @param targetClass the class declaring the lambda | ||
| * @param classBytes the freshly generated lambda class bytes | ||
| * @return the transformed bytes, or {@code null}/the original bytes if unchanged | ||
| */ | ||
| byte[] transform(String slashClassName, Class<?> targetClass, byte[] classBytes); | ||
| } |
64 changes: 64 additions & 0 deletions
64
...ava/datadog/trace/bootstrap/instrumentation/java/lang/invoke/LambdaTransformerHelper.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| package datadog.trace.bootstrap.instrumentation.java.lang.invoke; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** Transforms eligible lambda bytes before definition, falling back to the original on failure. */ | ||
| public final class LambdaTransformerHelper { | ||
| private static final Logger log = LoggerFactory.getLogger(LambdaTransformerHelper.class); | ||
|
|
||
| // Agent transformation may itself create lambdas. | ||
| private static final ThreadLocal<Boolean> TRANSFORMING = new ThreadLocal<>(); | ||
|
|
||
| private LambdaTransformerHelper() {} | ||
|
|
||
| /** | ||
| * @param classBytes the generated lambda class bytes | ||
| * @param lambdaClassName internal (slash-separated) name of the generated lambda class | ||
| * @param targetClass the class declaring the lambda | ||
| * @param interfaceClass the functional interface implemented by the lambda | ||
| * @return possibly transformed bytes; the original bytes on any failure | ||
| */ | ||
| public static byte[] transform( | ||
| byte[] classBytes, String lambdaClassName, Class<?> targetClass, Class<?> interfaceClass) { | ||
| try { | ||
| // Only exact allowlisted interfaces enter the transformer. | ||
| if (interfaceClass == null || LambdaInterfaceNameTrie.apply(interfaceClass.getName()) != 1) { | ||
| return classBytes; | ||
| } | ||
| LambdaTransformer transformer = LambdaTransformerHolder.get(); | ||
| if (transformer == null) { | ||
| log.debug("Lambda {} skipped: no transformer registered", lambdaClassName); | ||
| return classBytes; | ||
| } | ||
| if (targetClass == null) { | ||
| log.debug("Lambda {} skipped: no target class", lambdaClassName); | ||
| return classBytes; | ||
| } | ||
| // Skip lambdas declared by the agent itself to avoid self-instrumentation and recursion. | ||
| String targetName = targetClass.getName(); | ||
| if (targetName.startsWith("datadog.") || targetName.startsWith("net.bytebuddy.")) { | ||
| log.debug("Lambda {} skipped: declared by the agent", lambdaClassName); | ||
| return classBytes; | ||
| } | ||
| if (Boolean.TRUE.equals(TRANSFORMING.get())) { | ||
| log.debug("Lambda {} skipped: re-entrant transform", lambdaClassName); | ||
| return classBytes; | ||
| } | ||
| TRANSFORMING.set(Boolean.TRUE); | ||
| try { | ||
| byte[] result = transformer.transform(lambdaClassName, targetClass, classBytes); | ||
| if (result == null) { | ||
| log.debug("Lambda {} not transformed", lambdaClassName); | ||
| return classBytes; | ||
| } | ||
| return result; | ||
| } finally { | ||
| TRANSFORMING.set(Boolean.FALSE); | ||
| } | ||
| } catch (Throwable e) { | ||
| log.debug("Lambda {} skipped: {}", lambdaClassName, e.toString()); | ||
| return classBytes; | ||
| } | ||
| } | ||
| } |
19 changes: 19 additions & 0 deletions
19
...ava/datadog/trace/bootstrap/instrumentation/java/lang/invoke/LambdaTransformerHolder.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| package datadog.trace.bootstrap.instrumentation.java.lang.invoke; | ||
|
|
||
| /** | ||
| * Holds the {@link LambdaTransformer} registered by the agent installer. Lives on the bootstrap | ||
| * class path so it is reachable from instrumented {@code java.lang.invoke} code. | ||
| */ | ||
| public final class LambdaTransformerHolder { | ||
| private static volatile LambdaTransformer transformer; | ||
|
|
||
| private LambdaTransformerHolder() {} | ||
|
|
||
| public static void set(LambdaTransformer transformer) { | ||
| LambdaTransformerHolder.transformer = transformer; | ||
| } | ||
|
|
||
| public static LambdaTransformer get() { | ||
| return transformer; | ||
| } | ||
| } |
7 changes: 7 additions & 0 deletions
7
...urces/datadog/trace/bootstrap/instrumentation/java/lang/invoke/lambda_interface_name.trie
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # Generates 'LambdaInterfaceNameTrie.java' | ||
|
|
||
| # Exact functional interfaces whose generated lambda classes should be sent through the agent's | ||
| # matching and transformation pipeline. Keep this list narrow: the lookup runs for every lambda | ||
| # linkage in the application. | ||
|
|
||
| 1 java.lang.Runnable |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
43 changes: 43 additions & 0 deletions
43
...aller/src/main/java11/datadog/trace/agent/tooling/bytebuddy/DDJava9LambdaTransformer.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| package datadog.trace.agent.tooling.bytebuddy; | ||
|
|
||
| import datadog.trace.agent.tooling.bytebuddy.outline.TypePoolFacade; | ||
| import datadog.trace.bootstrap.instrumentation.java.lang.invoke.LambdaTransformer; | ||
| import java.lang.instrument.ClassFileTransformer; | ||
| import java.util.function.Function; | ||
|
|
||
| /** Routes generated lambdas through the module-aware Java 9+ transformer overload. */ | ||
| public final class DDJava9LambdaTransformer implements LambdaTransformer { | ||
|
|
||
| /** Read reflectively by the agent installer, which cannot name {@link Module} itself. */ | ||
| public static final Function<ClassFileTransformer, LambdaTransformer> FACTORY = | ||
| new Function<ClassFileTransformer, LambdaTransformer>() { | ||
| @Override | ||
| public LambdaTransformer apply(ClassFileTransformer classFileTransformer) { | ||
| return new DDJava9LambdaTransformer(classFileTransformer); | ||
| } | ||
| }; | ||
|
|
||
| private final ClassFileTransformer classFileTransformer; | ||
|
|
||
| public DDJava9LambdaTransformer(ClassFileTransformer classFileTransformer) { | ||
| this.classFileTransformer = classFileTransformer; | ||
| } | ||
|
|
||
| @Override | ||
| public byte[] transform(String slashClassName, Class<?> targetClass, byte[] classBytes) { | ||
| TypePoolFacade.beginLambdaTransform(); | ||
| try { | ||
| return classFileTransformer.transform( | ||
| targetClass.getModule(), | ||
| targetClass.getClassLoader(), | ||
| slashClassName, | ||
| null, | ||
| targetClass.getProtectionDomain(), | ||
| classBytes); | ||
| } catch (Throwable ignored) { | ||
| return null; | ||
| } finally { | ||
| TypePoolFacade.endLambdaTransform(); | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,6 +100,8 @@ final class TypeFactory { | |
|
|
||
| boolean createOutlines = OUTLINING_ENABLED; | ||
|
|
||
| boolean transformingLambda; | ||
|
|
||
| ClassLoader originalClassLoader; | ||
|
|
||
| ClassLoader currentClassLoader; | ||
|
|
@@ -159,6 +161,14 @@ void beginTransform(String name, byte[] bytecode) { | |
| } | ||
| } | ||
|
|
||
| void beginLambdaTransform() { | ||
| transformingLambda = true; | ||
| } | ||
|
|
||
| void endLambdaTransform() { | ||
| transformingLambda = false; | ||
| } | ||
|
|
||
| /** Once matching is complete we need full descriptions for the actual transformation. */ | ||
| void enableFullDescriptions() { | ||
| createOutlines = false; | ||
|
|
@@ -258,8 +268,9 @@ private TypeDescription lookupType( | |
| boolean isOutline = typeParser == outlineTypeParser; | ||
| long fromTick = InstrumenterMetrics.tick(); | ||
|
|
||
| // existing type description from same classloader? | ||
| SharedTypeInfo<TypeDescription> sharedType = types.find(name); | ||
| // Same-owner lambdas share a symbolic name, so build their target from the supplied bytes. | ||
| SharedTypeInfo<TypeDescription> sharedType = | ||
| transformingLambda && name.equals(targetName) ? null : types.find(name); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The agent can apply wrong instrumentation to the normal class. Assertion details
Was this helpful? React 👍 or 👎 |
||
| if (null != sharedType | ||
| && (name.startsWith("java.") || sharedType.sameClassLoader(classLoaderId))) { | ||
| InstrumenterMetrics.reuseTypeDescription(fromTick, isOutline); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the same lambda instance is submitted more than once before its first execution consumes the stored continuation—for example, a static or non-capturing Runnable queued concurrently by two requests—skipping the wrapper makes every submission share one
Stateslot. The second capture is rejected, and overlapping executions can either lose a parent or both resume the first submission's continuation, producing missing or incorrectly parented traces; this path needs per-submission state rather than opting every field-backed lambda out of wrapping.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I explcitly already put in the PR description for reviewers:
This is a known limitation and already existing for all the other field injected classes that are going throught threadpool. It's not introduced by this change and can be considered a trade-off we can perhaps live together for now.