Fix netty dependency and class version checking #411
Conversation
tylerbenson
force-pushed
the
tyler/class-refs
branch
2 times, most recently
from
8155233
to
781bea7
Compare
tylerbenson
force-pushed
the
tyler/class-refs
branch
from
781bea7
to
7577eb9
Compare
| public static void registerFeature(@Advice.This final Object builder) { | ||
| if (builder instanceof ClientBuilder) { | ||
| ((ClientBuilder) builder).register(ClientTracingFeature.class); | ||
| } |
There was a problem hiding this comment.
@tylerbenson could you explain why this is helpful: how instrumented class may not be present when its method is called?
There was a problem hiding this comment.
@tylerbenson and I were looking into this yesterday, and it seems that when @Advice.This is used with a specific class (i.e. ClientBuilder in this case), that class must to be already loaded at transformation time. Or ByteBuddy would throw the Cannot resolve type description error.
There was a problem hiding this comment.
Thanks for explaining!
To me this seems like a problem on bytebuddy side (or on the way we run it) - and it makes our code much uglier than it should be in my opinion. Also as @ark mentions there are probably other places where same problem should arise but apparently doesn't. I hope there might be better way to fix this ran manually casting all advice's parameters :)
| @@ -83,11 +83,11 @@ public static Scope startSpan( | |||
| GlobalTracer.get() | |||
There was a problem hiding this comment.
There's an @Advice.This here for the MessageProducer in ProducerAdvice's startSpan, should this be an Object?
There was a problem hiding this comment.
Would this be okay if the This param is an interface and the instrumentation applies to its implementations? Missing the full context here, but that type of pattern is okay for other types of advice/classloading issues.
There was a problem hiding this comment.
That was the issue that we were seeing with Netty: https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/instrumentation/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyChannelPipelineInstrumentation.java#L100
Where for the @Advice.This param, ChannelPipeline is an interface, and the implementation being instrumented is DefaultChannelPipeline.
There was a problem hiding this comment.
- Is it possible to fix this at the bytebuddy level? Can we tell bytebuddy to not attempt to load classes in the method sig?
- Why doesn't this affect other method params as well? For example, we're also referencing the
ChannelHandlerclass in the netty instrumentation. I would expect the same classloading gotchas to apply.
tylerbenson
force-pushed
the
tyler/class-refs
branch
from
7577eb9
to
9a6efe6
Compare
tylerbenson
changed the title
There was a problem hiding this comment.
Confirmed this works with #417 applied for Spring boot 2.
🎉
Still requires #417 to work all the way.