Fix opentracing 0.32.0

[tanumats]

Version 0.32.0 of opentracing has been released. Please reflect. https://github.com/opentracing/opentracing-java/releases/tag/release-0.32.0-RC1

Changes https://github.com/opentracing/opentracing-java/issues/314

[codefromthecrypt]

looks like still some changes going on https://github.com/opentracing/opentracing-java/commits/v0.32.0

are you able to raise a pull request for this?

[codefromthecrypt]

this is now released.. there are some problems to deal with as I just now looked at this cc @felixbarny @tylerbenson

for example, OT has no sampling api (still) and we are supposed to interpret Tags.SAMPLING_PRIORITY as a sampling hint. We also play games with the kind tag to figure out if we should share a span ID or not.

this is an issue with the new deferred io.opentracing.tag.Tag api. There's no docs on that type, but presume any tags could be done. For example, we interpret tags on set.. for example, looking if it is a number if it corresponds to the sampling tag when hydrating the builder. However, the signature of the type includes Span, not SpanBuilder. The trick is we'd prefer to know if we are supposed to sample a span before we create one. Allowing Tag to be passed to a builder knowing it has to be attributed to Spans, not builders, creates a problem here.

One way is to make a dummy span implementation that catches the result of Tag.set so that we can inspect the value as if it were made with SpanBuilder directly. For example, if the key matches sampling, we use a special dummy span to record the result of Tag.set similar to how mock libraries work. Then, we can get the value of sampling before we create a span.

Another is for OT to evaluate this api and if it is really appropriate to add to span builder, adding docs around why. I can guess, but I'd like to understand this and I'm sure others would too.

Even better would be to stop doing sampling as a vaguely interpreted flag.

[codefromthecrypt]

significantly revised the text of the above comment

[codefromthecrypt]

PS I'm not sure I agree with the design here, but if one were to try to fix the api you could overload set with a SpanBuilder param.

That way, there's no race and it has parity between the two call sites that are eventually invoked (Span.setTag, SpanBuilder.withTag)

[tylerbenson]

@adriancole I'm a bit confused... what changed in 0.32.0 that is raising this concern? As far as I can tell, the API's you mentioned have been in place for a long time (Tags.SAMPLING_PRIORITY).

[codefromthecrypt]

long time and in 0.31 are different questions. if you change from 0.31 to 0.32 you now have to implement a new method on span and span builder which accepts tag and value.

On Fri, Mar 29, 2019, 10:43 PM Tyler Benson notifications@github.com wrote:

@adriancole https://github.com/adriancole I'm a bit confused... what changed in 0.32.0 that is raising this concern? As far as I can tell, the API's you mentioned have been in place for a long time.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openzipkin-contrib/brave-opentracing/issues/91#issuecomment-478046466, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD612yhqHPaFegYa8ucooHHgFB7o5A1ks5vbjS2gaJpZM4YVyUC .

[codefromthecrypt]

@tylerbenson more literally below:

  @Override public <T> BraveSpanBuilder withTag(Tag<T> tag, T value) {
    // Assuming Tag implementations are only allowed to call span.setTag with the given key once?
    // We could make a dummy span captor impl to catch the value as we have a builder, not span here.
    // The tag could be a pseudo-api (ex sampling priority) desirable prior to building a span.
    // 
    //  public interface Tag<T> {
    //    String getKey();
    //    void set(Span span, T value);
    //  }
    return this;
  }

[codefromthecrypt]

I will later raise a pull request to show the impact of this design

[codefromthecrypt]

added an issue about the tag builder api https://github.com/opentracing/opentracing-java/issues/340

[codefromthecrypt]

added this about things still missing from span context https://github.com/opentracing/opentracing-java/issues/341

[codefromthecrypt]

note: 0.32 doesn't remove any deprecated code, so that means 0.33 will be a pro and a con. pro is code deleted, con is api break again https://github.com/opentracing/opentracing-java/issues/338

[hochraldo]

Any news on this? opentracing 0.33 is already out. Any plans to support this in the near future?

[codefromthecrypt]

yeah sorry weekend got away from me. will take a look tomorrow. main thing is to add invoker tests to prove the version range works.

On Mon, Jun 24, 2019, 9:39 PM hochraldo notifications@github.com wrote:

Any news on this? opentracing 0.33 is already out. Any plans to support this in the near future?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openzipkin-contrib/brave-opentracing/issues/91?email_source=notifications&email_token=AAAPVV2Y2QVWGEEF3DVLVZ3P4DFC3A5CNFSM4GCXEUBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYM6PXY#issuecomment-505014239, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAPVVY2MTIB27ZUHN6VCCLP4DFC3ANCNFSM4GCXEUBA .

[codefromthecrypt]

https://github.com/openzipkin-contrib/brave-opentracing/releases/tag/0.34.0