opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 344 forks source link

Separate TextMap into read/write interfaces. #315

Closed tylerbenson closed 5 years ago

tylerbenson commented 5 years ago

It currently has both combined into a single interface, but implementations commonly only implement one of the methods. Even the included adapters do this: https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/propagation/TextMapInjectAdapter.java#L39 https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/propagation/TextMapExtractAdapter.java#L43

This seems like an indication of a design problem.

I propose we separate this interface into two different interfaces, then let the current TextMap interface extend from each of them.

At this same time, we could include changes proposed in #305.

Thoughts?

yurishkuro commented 5 years ago

Worth noting that the original design was exactly that, because the extract/inject methods had the signatures

inject(SpanContext, Format<?, OUT_CARRIER>, OUT_CARRIER)
extract(Format<IN_CARRIER, ?>, IN_CARRIER)

In other words, IN and OUT interfaces were separate, and while the Format constants defined both for a given format, the inject/extract methods only cared about one. It was shot down as "too complicated"

I also wonder if we could do away with Format constants altogether. But these would be really breaking changes (-1).

codefromthecrypt commented 5 years ago

The decision to fold into one interface (TextMap) and abandon was done very early with no experience, directly after you mentioned you preferred them split https://github.com/opentracing/opentracing-java/pull/20#discussion_r57633510 There have been numerous concerns raised since.

I don't think folks should be held prey to things like this, especially after knowing it is the wrong design.

An aside, but let's leave format out of this as much as possible as that is a separate topic than whether or not the adapters are split.

my 2p

tylerbenson commented 5 years ago

So I tried to do a quick refactor, and I see that my proposed change is not quite as simple (non-breaking) as I was suggesting due to the single interface usage defined on Format.Builtin. I think I ended up with a reasonable solution without too significant of change.

carlosalberto commented 5 years ago

Shall we close this one @tylerbenson ?

tylerbenson commented 5 years ago

👍