Please consider to designate constructor of SpanProtocol as a public. I would leke to extend an implementation of the class or consider to replce inner class Factory with a builder instead to get rid of constructor telescoping.

Thanks in advance!

Currently, when a message of type EXCEPTION is returned by a thrift service, the server-side span is tagged as an error. However, no details of the exception are logged to the span. When the SpanProcessor catches an exception, it creates error logs and adds them to the span. Can the library do the same when the exception is returned by thrift? Just the type of the exception being returned would be useful.

@malafeev IIRC (please correct me if I'm wrong), this approach requires that servers upgrade before clients. That is operationally sketchy/dangerous.

Am I missing something about the name-prepending-based approach?

An alternative would be to use a "magic" tag number within the top-level message. I would favor that approach for the forwards- and backwards-compatibility benefits.

Because the trace context information is written in writeFieldStop(), it will be included at the end of each Thrift structure that gets encoded on the protocol. This is problematic when the request includes collections of other Thrift structures, as the trace context information will be added many many times at the end of each "object", adding a considerable amount of overhead to the network exchanges.

That being said, I'm not sure there is another way to do this given the hooks given to us by the TProtocol interface, unless there is somehow a way to use writeMessageEnd()?

Hi @malafeev,

Thanks for the quick turnaround on merging #3 and #4! With those two fixes I got java-thrift to correctly trace my Thrift RPC calls, and pass along trace context from client to server as desired. Would you be willing to make a 0.0.4 release and deploy that to Maven Central? It would make dependency management much easier.

Thanks in advance!

add json tests

I was seeing a protocol id exception using TCompactProtocol with some complicated unit tests (multiple clients threads, complex thrift structures, variety of interactions). I fixed this issue by replacing "injected" with an integer level that follows structure nesting:
jspofford@e3b54db#diff-7571f63b5051b15d8bf04742cfc4e517

I would have submitted a pull request but had trouble reproducing in your simplified unit tests. I'd need to dig further to isolate how the stream position is being upset. You may want to include this change anyway simply to ensure that the span data is sent in the last field stop instead of the first one, assuming you want to measure until the end of the incoming data serialization instead of just the first structure.

Unit tests pass with this change.

per https://github.com/opentracing/opentracing-java/blob/master/README.md#accessing-the-tracer , it's best not to require GlobalTracer... it should be a contingency plan but not much more.

The current code of the SpanProtocol assumes that a receive will always follow a write to close the tracing scope once the response has been received from the server. Unfortunately that's not always the case when you consider oneway endpoints.

@pavolloffay Any ideas on how to handle this?