Reinstate BaseSpan interface for backwards compatibility

[yurishkuro]

Fixes #237

[coveralls]

Coverage remained the same at 81.842% when pulling 21925aa1cb2e0bea9a7bc8f352c1f2f4c2cfd2a9 on v0.31.0-base-span into 85fab98119bb686f7ef3bf85e4e9fef646ffb93b on v0.31.0.

[yurishkuro]

As a small question: maybe it's worth adding a @deprecated

Doh! Forgot to hit Cmd-S.

[coveralls]

Coverage remained the same at 81.842% when pulling 803eddb20788289b61e98aa2fcd893777ac09dfa on v0.31.0-base-span into 85fab98119bb686f7ef3bf85e4e9fef646ffb93b on v0.31.0.

[tedsuo]

If no one blocks, I propose we merge this tomorrow afternoon (Friday jan 5th).

[pavolloffay]

I have compiled https://github.com/jaegertracing/jaeger-client-java/pull/313 with tchannel 0.7.7 and OT-API with this PR (local diff https://pastebin.com/tLAu1WxM).

and I sill get other compatibility issues (CNDF) for other classes see: https://pastebin.com/7eLFbhxs (output of ./gradlew test). I am still not sure what this PR solves.

UPDATE: I forgot to pin OT version in xdock module: now tests pass

[pavolloffay]

I was very curious why this actually works and is necessary because tchannel does not use BaseSpan https://github.com/uber/tchannel-java/blob/tchannel-0.7.7/tchannel-core/src/main/java/com/uber/tchannel/tracing/Tracing.java it's not imported in the whole repository, therefore it should work without this PR.

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 BaseSpan - in Tags.ERROR.set(span, true);

See decompiled code:

So with this PR there is BaseSpan symbol present. Then why does it not produce ClassCastException? Becase in 0.30.0 AbstractTag.set accepts generic BaseSpan<?> https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/tag/AbstractTag.java#L29 and if the last section from https://docs.oracle.com/javase/tutorial/java/generics/bridgeMethods.html holds. Then there is AbstractTag.set(object, ...) which is then internally casted to Span. But this is also weird because at runtime there is 0.31.0 which does not use wildcard in AbstractSpan

This is some kind of hack which uses "bridge method" (part of type erasure), I am still not sure why/how it works.

[tedsuo]

Thanks for the detailed review @pavolloffay! After running this by a number of people, I feel like it should go in.

[pavolloffay]

My comment https://github.com/opentracing/opentracing-java/pull/238#issuecomment-355632684 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 AbstractTag.set(AbstractSpan) calls. It can be seen in the screenshot above.

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 - if(response.isError()) is never executed so JVM is happy even with incompatible class introduced in this PR. If the code executes it produces:

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

[tedsuo]

Good catch.

[yurishkuro]

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.