Support opentracing-api 0.32 and 0.33

[codefromthecrypt]

This updates to opentracing 0.32 and 0.33

Here are the following notes:

• 0.31 and 0.32 cannot be supported at the same time.
  • This is due to overloading of setTag and withTag with the type Tag which doesn't exist in 0.31. creating an unresolvable compile conflict.
• 0.32 and 0.33 are supported, with conditional code execution to avoid the flawed deprecated logic from 0.31 still present in 0.32.
• binary formats are non-baggage carrying at the moment, using b3 single the same as brave's normal JMS does.

Fixes #91

[codefromthecrypt]

if we know the type are limited to predefined ones then yes we can do instance of string, number, boolean I dont know the scope of this Tag api, for example if it means to allow you to parse an exception or socket address into a string.

…

On Tue, Apr 2, 2019, 2:15 PM Felix Barnsteiner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/brave/opentracing/BraveSpanBuilder.java <#93 (comment)> : > @@ -88,6 +89,11 @@ return withTag(key, value.toString()); } + @OverRide public <T> BraveSpanBuilder withTag(Tag<T> tag, T value) { + new TagSetterHack<>(this, tag, value); Why can't you do withTag(tag.getKey(), value)? Is it because you don't know of which instance the value is? Could you do instanceof checks? Anyway, adding void set(SpanBuilder s, T value); to Tag sounds like a good solution. Maybe even something like void set(Taggable s, T value); where Span and SpanBuilder implement Taggable. (just brainstorming here) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#93 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD617x7pd_CsBvkuu3cmLDD7fITO9Hlks5vcwOpgaJpZM4cXQxk> .

[felixbarny]

AFAICT, It's meant to be only string, number and boolean. But you are right, nothing stops anyone from creating a ExceptionTag implements Tag<Exception> which extracts the stack trace and sets it as a string tag, for example. I have not seen that in the wild though. I'd suggest relying on an instance of check and assume there can only be Strings, Numbers and Boolean tag values until there is a Tag#set(SpanBuilder sb, T value) method.

[codefromthecrypt]

Ok I will change this to throw IllegalArgumentException (also should be defined on the Tag javadoc) when the input is neither string, number, boolean. Thanks @felixbarny

[tylerbenson]

@felixbarny you beat me to it... This was going to be my suggestion. 😉

[codefromthecrypt]

I will pick this up again tomorrow and cut a release that works ideally up to 0.33 as it will be hard to find a precise version everyone will use uniformly. The general idea is to use the approach of not adding @Override on new methods

[codefromthecrypt]

knackered due to all the re-releasing.. openzipkin/zipkin#2629 will try this again tomorrow.

[codefromthecrypt]

ps it is impossible to make a version that works both with 0.31 and 0.32 due to overloading

  @Override public BraveSpanBuilder withTag(String key, Number value) {
    if (Tags.PEER_PORT.getKey().equals(key)) {
      remotePort = value.intValue();
      return this;
    }
    return withTag(key, value.toString());
  }

  /* @Override 0.32 method: Intentionally no override to ensure 0.31 works! */
  public <T> BraveSpanBuilder withTag(io.opentracing.tag.Tag<T> tag, T value) {
    if (tag == null) throw new NullPointerException("tag == null");
    if (value == null) throw new NullPointerException("value == null");
    if (value instanceof String) return withTag(tag.getKey(), (String) value);
    if (value instanceof Number) return withTag(tag.getKey(), (Number) value);
    if (value instanceof Boolean) return withTag(tag.getKey(), (Boolean) value);
    throw new IllegalArgumentException("tag value not a string, number or boolean: " + tag);
  }

this would only work if it was a different method name.. because they OT the same method name for this useless addition, it is impossible to compile against both 0.32 and 0.31. You end up with class file for io.opentracing.tag.Tag not found.

[INFO] org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project opentracing-0.31: Compilation failure
[INFO] /Users/acole/oss/brave-opentracing/src/test/java/brave/opentracing/BraveTracerTest.java:[63,9] cannot access io.opentracing.tag.Tag
[INFO]   class file for io.opentracing.tag.Tag not found
[INFO] 
[INFO]     at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:215)

This is something we learned a long time ago in brave, as we made a similar mistake. openzipkin/brave#598

[codefromthecrypt]

Added error when opentracing 0.31 is present. cc @trustin @marcingrzejszczak

OpenTracing 0.31 detected. This version is compatible with io.opentracing:opentracing-api 0.32 or 0.33. Use latest io.opentracing.brave:brave-opentracing:0.33 or update you opentracing-api

[codefromthecrypt]

Added notes about this