Transform inline advice to delegating advice and test indy modules#9508
Conversation
| // We need to update our documentation on that | ||
| // Added back the call to contextProvider.rewriteVirtualFieldsCalls(extendableAgentBuilder) | ||
| // to get more instrumentations running. | ||
| extendableAgentBuilder = contextProvider.rewriteVirtualFieldsCalls(extendableAgentBuilder); |
There was a problem hiding this comment.
Adding back contextProvider.rewriteVirtualFieldsCalls got some more tests passing (I don't remember which one, would need to retest). I don't know why it was removed.
There was a problem hiding this comment.
I don't know why it was removed
I removed it becauseAdviceswould now useVirtualFieldin static variables instead. Because the advices are not inlined, the virtual field calls are not rewritten anyway.
However, I forgot about the fact there also is the possibility of inserting virtual field calls via the low-level ASM api.
I would assume that the instrumentations which were failing without this added code do exactly that.
Maybe it would make sense to double check if this is true and then change the comment accordingly. (e.g. "rewriteVirtualFieldsCalls is required in case instrumentations make use of TypeTransformer.applyTransformer and insert lookups for virtual fields")?
There was a problem hiding this comment.
Makes sense. Part of what rewriteVirtualFieldsCalls was still necessary here, I split that out from it and called just that part.
| String adviceMethodName, | ||
| MethodType adviceMethodType, | ||
| Object[] args) { | ||
| CallDepth callDepth = CallDepth.forClass(IndyBootstrap.class); |
There was a problem hiding this comment.
Whoopise, good catch. In the elastic agent we also have a CallDepth class, but it encapsulates a ThreadLocal. Definitely a difference to keep in mind when we port over additional instrumentations.
| * result.put("foo", foo); | ||
| * result.put("bar", bar); |
There was a problem hiding this comment.
Instead of referring to locals by their names, could we just put them into the array directly and refer by indexes in OnExit? And just keep a <String, Integer> mapping here, in the code generation, if we need that.
There was a problem hiding this comment.
We could and it definitely would make sense if we intended to keep this code, but as this is a temporary hack I think it isn't really important. Currently instrumenting methods doesn't have shared state, this would require having it. I'll think about it.
There was a problem hiding this comment.
Right, kinda makes sense if it's a temporary thing. Feel free to ignore it then 👍
This pr implements asm transformation logic to automatically transform our existing inline advice to delegating advice used with indy instrumentation modules. When reviewing keep in mind that this is intended as a temporary solution. I'd expect that finally we'll do the conversion manually. Automatic transformation lets us run tests with indy logic on our existing code and discover the instrumentations that need additional work. Currently the following transformations are performed:
inline = falseonAdvice.OnMethodEnterandAdvice.OnMethodExit@Advice.Argument(readOnly = false)to@Advice.AssignReturned.ToArguments.ToArgument@Advice.Return(readOnly = false)to@Advice.AssignReturned.ToReturned@Advice.Localto entry advice return value and read it with@Advice.Enterin exit adviceThere are instrumentations where these transformations do not work such as using both
@Advice.Argument(readOnly = false)and@Advice.Localin same adviceLinkageErrorbecause instrumentation class loader prefers the classes from the agent.