Add a new Binary Format to allow for variable SpanContext size

[gkumar7]

A Binary format is defined in opentracing-api which uses a ByteBuffer as a carrier. ByteBuffer needs to be allocated with a predetermined size for the SpanContext to be injected. This in turn requires a size calculation of baggage.

Possible approaches:

1. Place an upper limit on the size of the baggage.
2. Define a new carrier class (similar to TextMap) and format (similar to BINARY). Essentially, this would provide a wrapper for byte[].

// Additional Format specification (Format.java)
public final static Format<SimpleBinary> SIMPLE_BINARY = new Builtin<SimpleBinary>();

public class SimpleBinary {
    private byte[] payload;

    public byte[] getPayload() {
        return payload;
    }

    public void setPayload(byte[] payload) {
        this.payload = payload;
    }
}

BinaryInjectorImpl and BinaryExtractorImpl would be library implementation-specific (below is an example)

final class BinaryInjectorImpl implements Injector<SimpleBinary> {

    private final AbstractTracer tracer;
    private boolean baggageEnabled = AbstractTracer.BAGGAGE_ENABLED;

    BinaryInjectorImpl(AbstractTracer tracer) {
        this.tracer = tracer;
    }

    @Override
    public void inject(SpanContext spanContext, SimpleBinary carrier) {
        byte[] contextBytes = convertSpanContextToBytes(spanContext);
        carrier.setPayload(contextBytes);
    }

    void setBaggageEnabled(boolean baggageEnabled) {
        this.baggageEnabled = baggageEnabled;
    }
}

This allows the library to not have to calculate the size of the SpanContext as this size will depend on how the context is converted to bytes in convertSpanContextToBytes (ie using avro, etc).

[yurishkuro]

personally, my preference is for streaming-friendly carriers, such as the standard io.Writer we used in Go:

type Writer interface {
	Write(p []byte) (n int, err error)
}

The key difference here is that the Write method can be called by the tracer many times. This allows the tracer to be implemented without having to pre-allocate []byte for the whole payload.

[gkumar7]

Yes, I also prefer streaming-friendly buffers. The way the Format is specified currently:

public final static Format<ByteBuffer> BINARY = new Builtin<ByteBuffer>();

uses a ByteBuffer which is not a streaming-friendly carrier. A ByteArrayOutputStream could be used, but I do not believe inject is designed in a such a way to allow for multiple calls. My proposal above, although also not streaming-friendly, abstracts size calculation from the user.

The beauty of the go implementation is bytes.Buffer is a variable size buffer. On the other hand, Java's ByteBuffer is not.

[bhs]

IIRC, there was a desire to make the inject() caller's life easier before and after the call to inject()... the streaming options create complications there. If we went for a wrapper, I suppose it would also be possible to wrap a ByteBuffer (and thus allow the impl to decide on its exact size) rather than to just wrap a byte[].

Playing devil's advocate, there might be something to be said for giving the caller a clear signal (i.e., the ByteBuffer's capacity) about how much space the implementation gets to use, baggage or not. Inasmuch as the injected data ends up getting sent over the network, that's an assurance a caller might prefer to have.

[gkumar7]

@bensigelman, the size of the SpanContext may be an unwanted restriction and I feel should be up to the library implementation. In any case, the Binary format with ByteBuffer carrier will still remain. The SimpleBinary is a different format to help remove the restriction and abstract away size calculation.

[bhs]

@gkumar7 understood.

Just so I understand, is this issue in response to an actual production need, or a theoretical concern?

What do you think about having SimpleBinary wrapping a ByteBuffer rather than byte[], btw? And it'd be good to come up with a better name, though that's the last part to worry about.

[gkumar7]

Yes, we would like to have this in production. For now, we are using an upper bound for the SpanContext. If the implementation would like to place a limit using a ByteBuffer, they may use the Binary format, unless there is another benefit of using ByteBuffer for SimpleBinary.

I agree SimpleBinary is not a very good name, any suggestions? :).

[bhs]

What I'd really want to do is call this new format Binary, but of course that's a compatibility issue. When this was designed initially, I don't think this implication was well-understood (or at least not by me).

In lieu of that, maybe we call it VariableBinary or BinaryHolder (in the sense that the carrier "holds" the underlying byte array or ByteBuffer).

Speaking only for myself, I would accept a PR that added this. Though I would want others with more Java expertise to weigh in about pitfalls and/or alternatives, too.

[gkumar7]

Those are much better names, I am willing to submit a pull request for this. @adriancole, @michaelsembwever, @wu-sheng any comments?

[bhs]

I think a PR for a BinaryHolder carrier is warranted. The usability issue you raise here is legitimate (and unintended IMO), so no sense hiding from it.

[wu-sheng]

I prefer BinaryHolder, and by adding this, I am not sure whether we should keep ByteBuffer or not (Except Forward Compatibility). I thought these two formats have the similar purpose about inject/extract Context/Babbage into a byte[].

[bhs]

The existing binary format would only exist for compatibility reasons. At the next major version I would argue that we swap it out entirely.

[wu-sheng]

At the next major version I would argue that we swap it out entirely.

+1 on this.

[gkumar7]

Any update on this and the corresponding pull request? Thanks!

[ivantopo]

We stumbled upon this same need but the solution we came up with was a bit different, sharing it in case it might be of interest: instead of adding a new format we changed the signature of inject in our tracer a little so that it returns an instance of the carrier type and added an overload that doesn't receive a carrier object, just returns it. This is how inject looks like at the moment for us (Scala code):

  def inject[C](spanContext: SpanContext, format: Format[C], carrier: C): C
  def inject[C](spanContext: SpanContext, format: Format[C]): C

This plays nicely with cases in which the caller actually wants to append to a already allocated ByteBuffer and additionally gives the option to not care at all and let the Tracer decide internally how much to allocate and use the returned ByteBuffer.

Additionally to that, this second signature also plays nice with HTTP toolkits with immutable models (such as Spray and Akka HTTP) since now the caller can just take the returned TextMap and create a new copy of the HttpRequest with the additional headers. All in all, it seems to me that this solution breaks less things, makes the API a bit more flexible and leaves all the "trouble" of caring about how big the ByteBuffer has to be to the tracer implementation, if there is any interest in contrasting these solutions before deciding what gets into OT I'll be glad to participate!

[gkumar7]

Closing as #223 is merged.