-
Notifications
You must be signed in to change notification settings - Fork 344
Reinstate BaseSpan interface for backwards compatibility #238
Conversation
Signed-off-by: Yuri Shkuro <ys@uber.com>
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.
Looks good! (and nice to have a full note on why it's needed ;) )
As a small question: maybe it's worth adding a @deprecated annotation to the interface? Else, LGTM :)
Signed-off-by: Yuri Shkuro <ys@uber.com>
Doh! Forgot to hit Cmd-S. |
If no one blocks, I propose we merge this tomorrow afternoon (Friday jan 5th). |
I have compiled jaegertracing/jaeger-client-java#313 with and I sill get other compatibility issues (CNDF) for other classes see: https://pastebin.com/7eLFbhxs (output of UPDATE: |
I was very curious why this actually works and is necessary because However, there is an anonymous class on line 107 https://github.com/uber/tchannel-java/blob/tchannel-0.7.7/tchannel-core/src/main/java/com/uber/tchannel/tracing/Tracing.java#L107 and compiler decided to cast parameters when setting the tag to So with this PR there is This is some kind of hack which uses "bridge method" (part of type erasure), I am still not sure why/how it works. |
Thanks for the detailed review @pavolloffay! After running this by a number of people, I feel like it should go in. |
My comment #238 (comment) is not entirely correct. So why does it actually work? Tchannel code does not import BaseSpan explicitly but java compiler adds cast to BaseSpan in all This fix works in jaeger-client because the branch https://github.com/uber/tchannel-java/blob/tchannel-0.7.7/tchannel-core/src/main/java/com/uber/tchannel/tracing/Tracing.java#L111 - java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.codehaus.mojo.exec.ExecJavaMojo$1.run(ExecJavaMojo.java:294)
at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.NoSuchMethodError: io.opentracing.tag.BooleanTag.set(Lio/opentracing/BaseSpan;Ljava/lang/Boolean;)V
at sk.loffay.ot.test.TracedClass.invoke(TracedClass.java:20)
at sk.loffay.ot.test.module2.Main.main(Main.java:17)
... 6 more So this fix helps only if the code is actually never executed and introduces even more difficult runtime issues. If somebody wants to play with it https://github.com/pavolloffay/ot-compatibility-test |
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.
See #238 (comment)
Good catch. |
let's release without this. I spoke to our teams, we will coordinate a new release of Jaeger and TChannel libs to go in sync. Most services internally do not register these dependencies explicitly, but via internal wrapper libs, so we mostly need to bump deps in those. |
Fixes #237