Binary format proposal. #252

carlosalberto · 2018-01-19T07:56:18Z

Uses ByteBuffer to read/write data synchronously.
write() is expected to read all passed data.
read() may need many calls in case the buffer is not big enough.
Adapter for InputStream/OutputStream.
MockTracer BINARY propagator.

* Uses ByteBuffer to read/write data *synchronously*. * write() is expected to read all passed data. * read() may need many calls in case the buffer is not big enough. * Adapter for InputStream/OutputStream. * MockTracer BINARY propagator.

coveralls · 2018-01-19T07:58:39Z

Coverage increased (+0.7%) to 82.238% when pulling 8a4372c on carlosalberto:binary_format_proposal into 242ba95 on opentracing:v0.32.0.

carlosalberto · 2018-01-19T08:39:32Z

See #253

malafeev · 2018-01-19T08:45:53Z

opentracing-mock/src/main/java/io/opentracing/mock/MockTracer.java

+                        throw new RuntimeException("Corrupted state");
+                    } finally {
+                        if (objStream != null) {
+                            try { objStream.close(); } catch (Exception e2) {}


is it OK do not print e2?

malafeev · 2018-01-19T08:46:04Z

opentracing-mock/src/main/java/io/opentracing/mock/MockTracer.java

+                    } catch (IOException e) {
+                    } finally {
+                        if (objStream != null) {
+                            try { objStream.close(); } catch (Exception e2) {}


is it OK do not print e2?

I think it's not important here - this is closing an object used within this method, and it's more of a cleanup task.

malafeev · 2018-01-19T08:47:30Z

opentracing-api/src/main/java/io/opentracing/propagation/BinaryAdapter.java

+        //    the end of the stream or not.
+        byte[] b = new byte[buffer.remaining()];
+        int available = inputStream.read(b);
+        if (available < 0)


please add curly braces for if body

Will update 👍

yurishkuro · 2018-01-19T15:29:42Z

opentracing-api/src/main/java/io/opentracing/propagation/Binary.java

+public interface Binary {
+    /**
+     * Writes a sequence of bytes to this carrier from the given buffer.
+     * The internal buffer is expected to grow as more data is written.


what does this line refer to? Whose internal buffer? Seems we can remove it without loss.

Internal storage, basically (which would mean that many writes are possible). So based on your comment, it sounds like it definitely needs improvement (either by mentioning that multiple writes are allowed, or removing this line altogether).

yurishkuro · 2018-01-19T15:34:33Z

opentracing-mock/src/main/java/io/opentracing/mock/MockTracer.java

+                        }
+                    }
+                } else {
+                    throw new IllegalArgumentException("Unknown carrier");


can do this at the top to reduce nesting

Oh, was following the style the TEXT_MAP propagator has :) Anyhow, I can definitely change it to be top level as well.

tylerbenson · 2018-01-23T16:37:33Z

@carlosalberto I haven't had time to review the whole discussion yet, but does this fully incorporate @raphw's feedback from before?

carlosalberto · 2018-01-23T16:53:50Z

@tylerbenson Some of it, but we are still not taking into account two things that were requested:

Not telling in advance the output size (Tracer.inject())
Not over-the-wire support, but instead, in-memory buffering is expected (just like HttpHeaders and TextMap, where you write everything to some in-memory buffer and after that you pass it to the transmission and let the protocol handle it).

I'd expect, in turn:

Some (or a few) proof of concept implementations.
Early/experimental Tracers support and their feedback on this.

raphw · 2018-01-24T08:43:48Z

I can only repeate my critique of this API:

Byte buffers are meant to interact with data either on or off heap. This makes byte buffers useful if a server can allow the Tracer to directly serialize or deserialize data from for example a TCP buffer without the JVM needing to also allocate these byte on the Java heap what is expensive in terms of GC. This idea is not reflected by this API proposal and only puts additional weight onto the tracer as most implementations will look like in this example where buffers are controlled by the tracer. A NIO-friendly API must allow for providing ByteBuffers from outside of the tracer.
Byte buffers are typically used in the context of asynchrous I/O. An important factor with NIO is that an external buffer such a native TCP storage might not be able to hold all the bytes that belong to the baggage information at once. If a server, from the outside, provides a buffer to the tracer, it cannot normally guarantee that all data is already available. It would rather ask the tracer to do a partial deserialization/serialization to clear the buffer and then suspend the current channel to allow the buffer to fill. In the meantime, it would move on to the next channel from the very same thread to avoid blocking the thread. This is the core idea of NIO and selectors. With OpenTracing being a synchronous API that does not allow for such partial deserialization, the server would need to copy the data into a continous heap array and provide this array once it completed and wrapped as a byte buffer. The current API is therefore equivalent to one that is operating on byte arrays.
There is no binary contract implied. Binary protocols that allow embedding random bytes typically add a marker byte to indicate the start and end of a custom segment. This way, a server can accept baggage data from a client without even having a tracer installed by just cropping the segment. By allowing to write to byte buffers explicitly, the user of a tracer has no chance to escape any payload bytes that equal the marker byte, thus breaking the protocol. The only way to work around this would be to ask the tracer to write to an intermediate buffer and escape the data from there. This is breaking the core assumption of NIO.

None of these problems occurs by offering a stream-based approach:

Streams can read and write one byte at a time what avoids additional allocation of byte arrays. (Streams can be directed to write or read to or from a direct byte buffer without the mentioned need for intermediaries.)
Streams can easily be decorated, allowing for escaping of mentioned marker bytes.
But streams are synchronous. But since this is also true for the baggage injection and extraction APIs, this is not a problem here.

But even with streams: if the server uses a different tracer implementation than the client, none of this will work to begin with. There is no serialization format defined and if I write the data as BSON but the server expects protobuf, this would result in garbage data being reported. Even worse, this could maybe turned into an attack vector if I find a way to exploit the servers deserialization approach, maybe by forcing it to overconsume data from the buffer what corrupts the remainder of the stream what can have unwanted effects.

tylerbenson · 2018-01-29T01:55:39Z

@raphw What would you then suggest? Maybe I'm missing something, but writing instrumentation to support propagation over a generic binary interface without breaking the protocol when only one side is instrumented seems to be a significant challenge. Perhaps it would help if we had some specific use cases. What protocols/frameworks are expected to use this binary format? (perhaps protobuf, thrift, grpc?) Maybe it would be better to define those propagation interfaces specifically like we do for http, instead of lumping everything into a binary format? Which formats/protocols are more common when working with NIO? What frameworks should be explored that would highlight these problems?

(Forgive me for my ignorance. I've written a fair bit of instrumentation, but relatively little for systems without generic headers/metadata/parameters that can be used for propagation details.)

tedsuo · 2018-01-29T19:46:57Z

@raphw @tylerbenson I'd love to see the requirements for binary format expressed as a set of tests/examples. This worked very well for designing context propagation, where the requirements were similarly nuanced. If we keep discussing in english, we'll probably never resolve this. :)

raphw · 2018-02-02T20:42:53Z

Binary Protocols typically include such optional sections either by:

Disclosing the size of the segment with some meta data, e.g. by a fixed-length name and a fixed-length size indication followed by the amount of bytes disclosed. This way, if the server recieves this segment but does not know a dispatcher for such a named area, it just discards the bytes.
Starting the segment by a name and by indicating the end of the section by a marker byte. All bytes in the payload that would indicate the same byte as this marker byte are then escaped. E.g. any null byte is replaced by two null bytes within the payload. If a null byte is not followed by another null byte, it describes the end of the segment. Again, if no dispatcher is found, the payload can be discarded.

The first option requires to know the binary size in advance, the second option requires a way to manipulate any written byte before it is written to a buffer. My original suggestion using streams would allow for that.

carlosalberto · 2018-04-27T14:16:53Z

Hery everybody - sorry for the delay. Recently I took up on testing the current BINARY proposal to do tracing for Cassandra on the server side (through a plugin), and the experimental code consuming this specific BINARY support exists in https://github.com/carlosalberto/java-cassandra-server

Please feel free to test it out. Cassandra itself takes a ByteBuffer as payload for outgoing query requests, and receives a ByteBuffer from the server side as well (described in the mentioned repository containing the Cassandra plugin).

I have the impression most of the times, as the SpanContext tends to be small, the payload will be buffered and transferred in a single step.

That being said, I do realize that @raphw usage may not be supported with the current approach, and that may need an additional format (maybe something like BINARY_STREAMING).

Let me know ;)

@tedsuo @tylerbenson @yurishkuro

carlosalberto · 2018-05-10T16:24:30Z

Trying to re-take the discussion: I took a little look at how Netty works, and ended up gathering some overall issues that have been floating during the design of this format. I'm listing them here to help things get moving.

Netty and similar asynchronous frameworks for doing low-level bytes transmission (through socket/streaming connections) could be helped by exposing an InputStream/OutputStream API - however, synchronously injecting the SpanContext would work just fine, specially taking into account the context tends to be small.
In most APIs I've seen, any binary payload is specified as a specific member, not as part of a bytes stream. For example: in Cassandra that is exposed as a (single) ByteBuffer, and in Thrift you have a binary type. As you need to buffer the payload in memory before passing/setting it, a stream API wouldn't help much here.
An InputStream/OutputStream API could also let the user know how much space will be used, byte-wise (as a prior method call to do the actual write()).
An InputStream/OutputStream API would expose too many methods that would be useless for injection/extraction - mostly in the InputStream (close, mark, reset, skip, etc). This would go against a clean API to some degree.

One thing I wonder is how much time will be spent writing/doing instrumentation for low-level stuff (such as the one exposed by Netty), and how other times it will be done for http or frameworks abstracting the transmission (for these ones we don't need the stream-based API).

Thoughts? Waiting for your feedback @raphw

raphw · 2018-05-11T19:01:30Z

the more I think of it, I believe that it would actually be the easiest to extract a TextMap and push it into the stream depending on the underlying binary protocol. Until there is an actual use case, maybe it makes sense to table the binary extraction?

carlosalberto · 2018-05-15T11:43:31Z

@raphw Thanks for the feedback. Well, I'd say using OT for instrumenting Cassandra on the server side is already an actual case (albeit a simple one ;) ).

@yurishkuro Any opinion on this?

yurishkuro · 2018-05-15T14:56:55Z

I agree that this is not a pressing matter (strictly speaking "binary" can be just a text_map serialized in a way specific to the protocol being instrumented, like Cassandra), but would be nice to resolve.

carlosalberto · 2018-10-19T16:34:43Z

Closing this PR in favor of #276 (which was just merged).

Binary format proposal.

8a4372c

* Uses ByteBuffer to read/write data *synchronously*. * write() is expected to read all passed data. * read() may need many calls in case the buffer is not big enough. * Adapter for InputStream/OutputStream. * MockTracer BINARY propagator.

carlosalberto mentioned this pull request Jan 19, 2018

Binary support design #253

Open

malafeev reviewed Jan 19, 2018

View reviewed changes

yurishkuro reviewed Jan 19, 2018

View reviewed changes

carlosalberto mentioned this pull request Jun 2, 2018

Simple layer on top of ByteBuffer for BINARY format. #276

Merged

carlosalberto closed this Oct 19, 2018

carlosalberto mentioned this pull request Oct 23, 2018

0.32.0 Release #314

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binary format proposal. #252

Binary format proposal. #252

carlosalberto commented Jan 19, 2018

coveralls commented Jan 19, 2018

carlosalberto commented Jan 19, 2018

malafeev Jan 19, 2018 •

edited

Loading

malafeev Jan 19, 2018 •

edited

Loading

carlosalberto Jan 19, 2018

malafeev Jan 19, 2018

carlosalberto Jan 19, 2018

yurishkuro Jan 19, 2018

carlosalberto Jan 19, 2018

yurishkuro Jan 19, 2018

carlosalberto Jan 19, 2018

tylerbenson commented Jan 23, 2018

carlosalberto commented Jan 23, 2018

raphw commented Jan 24, 2018 •

edited

Loading

tylerbenson commented Jan 29, 2018 •

edited

Loading

tedsuo commented Jan 29, 2018

raphw commented Feb 2, 2018

carlosalberto commented Apr 27, 2018

carlosalberto commented May 10, 2018

raphw commented May 11, 2018

carlosalberto commented May 15, 2018

yurishkuro commented May 15, 2018

carlosalberto commented Oct 19, 2018

Binary format proposal. #252

Binary format proposal. #252

Conversation

carlosalberto commented Jan 19, 2018

coveralls commented Jan 19, 2018

carlosalberto commented Jan 19, 2018

malafeev Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

malafeev Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

carlosalberto Jan 19, 2018

Choose a reason for hiding this comment

malafeev Jan 19, 2018

Choose a reason for hiding this comment

carlosalberto Jan 19, 2018

Choose a reason for hiding this comment

yurishkuro Jan 19, 2018

Choose a reason for hiding this comment

carlosalberto Jan 19, 2018

Choose a reason for hiding this comment

yurishkuro Jan 19, 2018

Choose a reason for hiding this comment

carlosalberto Jan 19, 2018

Choose a reason for hiding this comment

tylerbenson commented Jan 23, 2018

carlosalberto commented Jan 23, 2018

raphw commented Jan 24, 2018 • edited Loading

tylerbenson commented Jan 29, 2018 • edited Loading

tedsuo commented Jan 29, 2018

raphw commented Feb 2, 2018

carlosalberto commented Apr 27, 2018

carlosalberto commented May 10, 2018

raphw commented May 11, 2018

carlosalberto commented May 15, 2018

yurishkuro commented May 15, 2018

carlosalberto commented Oct 19, 2018

malafeev Jan 19, 2018 •

edited

Loading

malafeev Jan 19, 2018 •

edited

Loading

raphw commented Jan 24, 2018 •

edited

Loading

tylerbenson commented Jan 29, 2018 •

edited

Loading