Split Inject and Extract Builtin interfaces

[tylerbenson]

If we had separate formats for inject and extract this refactor would have been much cleaner.

This allows us to better avoid partially implementing the TextMap interface and throwing an exception for the other method.

Since Binary is a new addition, I applied a similar refactoring to that.

Closes #315.

[coveralls]

Coverage decreased (-0.6%) to 76.0% when pulling 875022c0b738961bd66b9c5ba6c3c56c1303fc4d on tylerbenson:tyler/text-map-split into b6f63246af9688042097f65781357d66e29141d9 on opentracing:v0.32.0.

[carlosalberto]

So I like this change definitely, but I'm wondering about breaking the API (which we could actually do for BINARY as we are breaking it anyway, and mostly because nobody uses it currently, AFAIK).

Wondering if there's something we could do to help with such API break.

[tylerbenson]

@carlosalberto what API breakage are you referring to that you'd like to avoid? I thought the way I made this change still maintained backwards compatibility.

I would still like to consider #305 which would definitely be a breaking change, but that isn't included in this yet.

[carlosalberto]

Hey @tylerbenson

Sorry for the late feedback, was busy with other things. So I think it indeed does not actually break the API, but I'm a little bit curious (more than concerned) with users having to change their injection code to:

tracer.inject(span.context(), Format.Builtin.TEXT_MAP_INJECT, carrier);

Or else have tracer authors check for both TEXT_MAP and TEXT_MAP_INJECT (and TEXT_MAP and TEXT_MAP_EXTRACT for the extraction scenario, etc). Probably this last one is the one making more sense, backwards-compatibility wise (interestingly enough, in MockTracer we wouldn't need to change the handling at all, as we don't check the Format<C> object, but only its C type ;) )

[tylerbenson]

@carlosalberto I believe the way I wrote it allows the new built-in formats to be opt-in for API usage. For tracer authors, I think it depends on implementation, though I think using the type is probably the best option.

[carlosalberto]

Hey all,

I'd like to get this PR moving - specially as it will not break things (as @tylerbenson refactored things nicely).

One thing I still wonder is whether we should deprecate TextMap or keep it, as @tyler also mentioned:

I think it is still useful if someone wants to create a single wrapper for both inject/extract purposes

Thoughts? @opentracing/opentracing-java-maintainers

[tylerbenson]

@carlosalberto We have a few additional approvals. I think this is ready for merge now.