Problems around the new io.opentracing.tag.Tag api

[codefromthecrypt]

While I can't quite trace the background for it, io.opentracing.tag.Tag appears to have been added last week. There are a number of problems around it, which I'll catalog here:

1. there are no api docs for this, for example, clarifying if anything is allowed to be null, if it is allowed to call anything multiple times, or the apis it is allowed to call... if it should be called in no-op etc. Neither is it clear why you would implement this. Basically docs are needed both to make safe implementations and to decide if one should implement this.
2. this broke 0.31 api to add this to Span, but as you can see below, it is actually more characters to invoke it this way. tag.set(span, value) is 3 characters less than directly calling span.setTag(tag, value). It would be nice to know why this should have broken api.
3. This was added to SpanBuilder also, but as a builder overload isn't defined, this causes a problem in implementation. I'll describe that at length in the next section.

On problems with adding withTag(Tag<T> tag, T value) to SpanBuilder, especially in context to the sampling flag.

OT has no sampling api (still) and we are supposed to interpret Tags.SAMPLING_PRIORITY as a sampling hint. This means we need to know the value during SpanBuilder hydration.

However, the signature of io.opentracing.tag.Tag is Span, not SpanBuilder. This api gap causes problems.

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.

  @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;
  }

Other options are to remove the withTag(Tag<T> tag, T value) signature from the builder, or add set(SpanBuilder builder, T value); to Tag.

[codefromthecrypt]

added a pull request for the major features needed. a little more work to do. pausing a bit

notably you can see the hack here https://github.com/openzipkin-contrib/brave-opentracing/pull/93/files#r271152691

[yurishkuro]

@tylerbenson correct me if I am wrong, but it looks like your original intention (#311) with this method

<T> Span setTag(AbstractTag<T> key, T value);

was NOT to require that implementations call key.set(this, value) (kind of hinted at by calling the argument key instead of tag).

However, later on in 9b6ca36 it was changed to

<T> Span setTag(Tag<T> tag, T value);

I think we need to fix this by documenting that indeed the implementations of these span/builder methods are expected to do a double-dispatch by delegating to the passed tag object, which also must implement set(SpanBuilder, builder, T value) method to be consistent. Without double-dispatch the implementation cannot handle tag values in type-safe manner, which can be observed in the original PR #311 where mock tracer ignores the type:

public <T> Tracer.SpanBuilder withTag(Tag<T> tag, T value) {
            this.initialTags.put(tag.getKey(), value);
            return this;
}

Alternatively, we could remove/deprecate these two methods from span/builder, as they seem to be causing more issues than they are worth.

cc @carlosalberto

[carlosalberto]

Hey hey

was NOT to require that implementations call key.set(this, value) (kind of hinted at by calling the argument key instead of tag).

Going through the history of this change, I can also see that we ended up with an actual different thing.

My feeling is that we should document the expected behavior (the double dispatch part) should be enough for this API (as we won't have many upcoming releases).

[yurishkuro]

For double-dispatch to work on SpanBuilder the Tag interface needs an extra method set(SpanBuilder, T)

[tylerbenson]

@yurishkuro I guess I don't fully understand the problem you're referring to with the double-dispatch and how it relates to SpanBuilder. Is your issue with the need for double dispatch?

[yurishkuro]

Sorry, double dispatch is probably a jargon used for Visitor-like pattern, which is very similar to Tag:

span.Set(tag,v) => tag.Set(span,v) => span Set(tag.key, v)

The same doesn't work for span builder today because the middle part is missing from Tag interface: tag.Set(spBldr, v)

[tylerbenson]

@yurishkuro on SpanBuilder, maybe the implementation could be just:

@Override
public <T> SpanBuilder withTag(final Tag<T> tag, final T value) {
  return withTag(tag.getKey(), value);
}

I realize this isn't as nice as with Tag, but this is effectively what was being done before these changes.

I must be missing something... Where does this break down?

[yurishkuro]

It won't compile 😄

withTag methods are strongly typed, but here we don't know the type of the value. Double dispatch through concrete tag works because it makes the value type concrete.

[tylerbenson]

@yurishkuro I see my problem now... we had a private method in our implementation that each of the withTag methods delegated to:

withTag(final String tag, final Object value)

Which is why I didn't discover this problem when doing integration testing.

[yurishkuro]

@tylerbenson exactly, the same happened in MockTracer in the original PR, we could've caught it there.