introduce a Format type in Java #33
Conversation
Apparently IntelliJ decided to "intelligently" (sic) move some files to the wrong directories, resulting in some noise here. This change re-introduces much of the type-enforcement lost during #32
| */ | ||
| void inject(SpanContext spanContext, Object carrier); | ||
| <C> void inject(SpanContext spanContext, Format<C> format, C carrier); |
There was a problem hiding this comment.
obviously the changes to inject and extract are the main events here.
| @@ -16,7 +16,7 @@ | |||
| import java.util.Iterator; | |||
| import java.util.Map; | |||
|
|
|||
| public final class TextMapImpl implements TextMapWriter, TextMapReader { | |||
| public final class TextMapImpl implements TextMap { | |||
There was a problem hiding this comment.
I would rather keep TextMapWriter and TextMapReader (instead of Impl) as concrete implementations, and let them throw NotImplementedException from one of the methods.
It's also problematic that this class is defined in the -impl jar, because it is intended for the end user of the API, and the end users have no need to depend on the -impl module.
There was a problem hiding this comment.
In other languages we haven't provided text map implementations as part of the core API. Not that it's needed by -impl, either: the only caller of TextMapImpl is test code at the moment.
As for splitting it into reader/writer pieces: I can see it either way. If we're going to provide helpers for the Map<String, String> case, though, I think we can make the intention clearer through naming. I'll give it a shot.
There was a problem hiding this comment.
Okay, done. I went with TextMapInjectAdapter and TextMapExtractAdapter which is clearer and more self-documenting (at least in my brain).
There was a problem hiding this comment.
In other languages we haven't provided text map implementations as part of the core API.
We do have TextMapCarrier in Go. That one is actually R/W, but it's ok because in Go the accepted carrier interfaces are separated to R and W.
|
I personally prefer |
| import java.nio.ByteBuffer; | ||
|
|
||
| /** | ||
| * Format instances control the behavier of Tracer.inject and Tracer.extract (and also constrain the type of the carrier parameter to same). |
|
@adriancole @michaelsembwever curious if either of you has an opinion about this. IMO the self-documentation benefits make this worthwhile independent of anything else, though there are also some wins for implementations from a compile-time type-checking standpoint. I realize it's been a contentious (and long-standing) issue, so at this point I'm looking less for fanatical consensus and more for "this seems reasonable". Of course fanatical consensus is welcome, too. :) |
This sums it up well. The second point I feel is moot. The first not so simple, because adding constructs can also be seen as a burden for a concept that the user has to understand anyway. Try and make an API extra simple but adding more and more types and by underestimating rather basic intelligence of the user you end up achieving the opposite. Whether that's true in this situation I really need to read more examples to make a decision. For me, I was never against the introduction of the Format parameter, I just saw it as potentially superfluous and wanted to see time prove that true or not. For that it is (3) in Yuri's comment #31 (comment) that provides enough rationale to go ahead with this change. I appreciate the patience and explanations that have gone into this. |
|
@michaelsembwever thank you kindly for giving this a look. You wrote:
Absolutely agree. Without an explicit Re patience/explanations: same to you. |
Yes this is true. |
|
Well... I am going to merge this in ~24h or so if there are no further objections. |
TL;DR, this is option 3 described at #31 (comment)
I will make a few comments inline to focus attention on the important bits.