withTag(Tag<T>, T) implementation violates spec

[epkugelmass]

Requirement - what kind of business use case are you trying to solve?

jaeger-java-client should implement the specification in opentracing-java
Specifically, JaegerTracer should implement io.opentracing.Tracer according to the spec, and JaegerSpan should implement io.opentracing.Span according to the spec.

Problem - what in Jaeger blocks you from solving the requirement?

JaegerTracer::withTag(Tag<T>, T) and JaegerSpan::setTag(Tag<T>, T) implementations are incompatible with the OpenTracing specification.

Tracer::withTag is specified to function like (Abstract)Tag::set(Span, T). However, JaegerTracer's implementation does not call Tag::set. The "injectable tagging behavior" represented by Tag is lost.

The same issue is present in JaegerSpan::setTag(Tag<T>, T).

The test for JaegerSpan passes because it asserts the incorrect result. See https://github.com/jaegertracing/jaeger-client-java/blob/master/jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSpanTest.java#L176

Proposal - what do you suggest to solve the problem or improve the existing situation?

The test should be amended to assert that the value is null, not this.

The implementations should be amended similar to this:

- this.tags.put(tag.getKey(), value);
+ tag.set(this, value);

though perhaps with a little more finesse to short-circuit in the case of non-sampled contexts.

Any open questions to address

None that I'm aware of. Happy to be wrong!

[epkugelmass]

I am happy to prepare a PR, but perhaps no earlier than late next week.

[yurishkuro]

There are different issues with Tag API opentracing/opentracing-java#340

I don't think there's actually a clear expectation that span/builder implementations must do a double-dispatch to tag.set(), it was certainly not how the original PR opentracing/opentracing-java#311 implemented it for mock tracer.

[epkugelmass]

Thanks for the link to the underlying issues in opentracing-java.

I think the OpenTracing Java spec is quite clear.
https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/Tracer.java#L192
/** Same as {@link AbstractTag#set(Span, T)}, but for the span being built. */
I'm having trouble reading this as anything except requiring dispatch to the Tag's set implementation. Whether or not this specification is itself implemented correctly by OpenTracing-Mock or even makes sense is secondary.