indy instrumentation - leftovers migration #13074
Conversation
…nstrumentation into indy-leftovers
| @@ -28,8 +32,17 @@ public boolean defaultEnabled(ConfigProperties config) { | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean isIndyModule() { | |||
| return false; | |||
| public void injectClasses(ClassInjector injector) { | |||
There was a problem hiding this comment.
I don't think having any invokedynamic-based instrumentation/proxies is a good idea for the lambda instrumentation: The reason is that we unecessarily risk recursions for the invokedynamic bootstrapping if lambdas are used in there in some unfortunate places, even if this is not the case right now.
I feel we should rather simply move those two classes to the agent bootstrap package, lambda instrumentation imo is special enough to justify this. Alternatively, we could directly inject the classes via injectedClassNames().
There was a problem hiding this comment.
Good point, until moving those to the bootstrap package is further discussed I've switched to using injectedClassNames() in 45ead47
.../main/java/io/opentelemetry/javaagent/instrumentation/internal/lambda/LambdaTransformer.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public byte[] transform( |
There was a problem hiding this comment.
[for reviewer] here we override the pre-java9 API and then resolve the class module from the class, this allows to transparently call it from code compiled with Java 8 API level.
In practice, there is only a single usage for this in the internal lambda instrumentation, which makes it equivalent to the (now removed) Java9LambdaTransformer.
| instrumentationInstalled = true; | ||
| ClassFileTransformerHolder.setClassFileTransformer(resettableClassFileTransformer); | ||
| if (JavaModule.isSupported()) { | ||
| // wrapping in a JPMS compliant implementation |
There was a problem hiding this comment.
[for reviewer] wrapping at registration time helps to make the consumer transparently use it with java 8 API.
| public class InlineIbmResourceLevelInstrumentationModule extends InstrumentationModule { | ||
| public InlineIbmResourceLevelInstrumentationModule() { | ||
| super("inline-ibm-resource-level"); | ||
| public class IbmResourceLevelInstrumentationModule extends InstrumentationModule { |
There was a problem hiding this comment.
[for reviewer] there is no need to maintain an inline and indy variants of this instrumentation, so we only need one that is compatible with both.
| * | ||
| * @return class transformer for defining lambdas | ||
| */ | ||
| public static ClassFileTransformer getLambdaClassFileTransformer() { |
There was a problem hiding this comment.
[for reviewer] this is only used for lambda, and the ClassFileTransformer instances that are used here are very specific to lambda instrumentation, so renaming helps keep it clear.
.../main/java/io/opentelemetry/javaagent/instrumentation/internal/lambda/LambdaTransformer.java
Outdated
Show resolved
Hide resolved
| public List<String> injectedClassNames() { | ||
| return Arrays.asList( | ||
| "io.opentelemetry.javaagent.instrumentation.kotlinxcoroutines.instrumentationannotations.AnnotationSingletons", | ||
| "io.opentelemetry.javaagent.instrumentation.kotlinxcoroutines.instrumentationannotations.AnnotationInstrumentationHelper", | ||
| "io.opentelemetry.javaagent.instrumentation.kotlinxcoroutines.instrumentationannotations.MethodRequest", | ||
| "io.opentelemetry.javaagent.instrumentation.kotlinxcoroutines.instrumentationannotations.MethodRequestCodeAttributesGetter"); | ||
| } |
There was a problem hiding this comment.
instead of injecting all the helper did you consider adding a proxy for AnnotationInstrumentationHelper similarly to
There was a problem hiding this comment.
I am not sure we have the choice here, as MethodRequest is being used directly used in WithSpanInstrumentation which uses ASM, also if MethodRequest is being injected then it's also pulling AnnotationSingletons which is using it to be injected as well.
There was a problem hiding this comment.
We could replace MethodRequest with Object in method signatures. We had similar issue in indy instrumentation where advice methods could not have helper classes in the signature, see #11873 What do you think?
There was a problem hiding this comment.
I think that could work, I will try to see if we can easily do it.
If I understand what you aim to achieve here is only exposing the AnnotationInstrumentationHelper class to the instrumented code through an injected proxy, and then leave other classes called by the real AnnotationInstrumentationHelper class implementation as implementation details.
One of the downsides of this approach is that we loose the type-safety of the method signatures and need to introduce explicit casts to make it work.
Should be the final step of #11457, hopefully completing it.
There are currently 5 overrides of
isIndyModule()and the goal here is to removeallmost of them, hence allowing to move forward with the next steps described in #13031