Split Inject and Extract Builtin interfaces #316
Conversation
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 opentracing#315.
|
So I like this change definitely, but I'm wondering about breaking the API (which we could actually do for Wondering if there's something we could do to help with such API break. |
| * @see Format.Builtin#HTTP_HEADERS | ||
| */ | ||
| void put(String key, String value); | ||
| public interface TextMap extends TextMapInject, TextMapExtract { |
There was a problem hiding this comment.
are we deprecating this then?
There was a problem hiding this comment.
I think it is still useful if someone wants to create a single wrapper for both inject/extract purposes. I don't think it's necessary to deprecate.
|
@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. |
| @@ -13,6 +13,7 @@ | |||
| */ | |||
| package io.opentracing.propagation; | |||
|
|
|||
| import io.opentracing.SpanContext; | |||
There was a problem hiding this comment.
Do we really need it here? ;)
There was a problem hiding this comment.
Since it's referenced in the javadoc and not fully qualified, it does need to be imported.
|
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 |
|
@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. |
|
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
Thoughts? @opentracing/opentracing-java-maintainers |
|
@carlosalberto We have a few additional approvals. I think this is ready for merge now. |
* Deprecate the StringTag.set() overload taking a StringTag. (#262) * Implement Trace Identifiers. (#280) * Bump JaCoCo to use a Java 10 friendly version (#306) * Remove finish span on close (#301) * Deprecate finishSpanOnClose on activation. * Add ScopeManager.activeSpan() and Tracer.activateSpan(). * Clarify the API changes and deprecations. * Add an error reporting sample to opentracing-testbed. * Simple layer on top of ByteBuffer for BINARY format. (#276) * Add generic typed setTag/withTag (#311) * Allow injecting into maps of type Map<String,Object> (#310) * Add simple registerIfAbsent to global tracer (#289) * Split Inject and Extract Builtin interfaces (#316) * Deprecate ScopeManager.active() (#326) * Make Tracer extends Closable. (#329) * Do not make isRegistered() synchronized. (#333) * Deprecate AutoFinishScopeManager (#335)
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.