Change BINARY to be resizable and stream-oriented. #223

carlosalberto · 2017-11-30T08:38:23Z

This is an effort to provide a stream-based, resizable BINARY format, inspired by #99 and its feedback.

BinaryHolder is an adapter for either ByteArrayOutputStream or ByteArrayInputStream, depending on whether the format was created for injection or extraction.

I exposed two static methods to try to make purpose clear, i.e. createInjectionHolder(), createExtractionHolder, instead of using simple constructor(s).

Any feedback is appreciated ;)

coveralls · 2017-11-30T08:43:19Z

Coverage increased (+1.06%) to 79.661% when pulling 87d1782 on carlosalberto:binary_adapter into b9feb48 on opentracing:v0.31.0.

yurishkuro · 2017-11-30T15:39:55Z

Glad to see us moving on this. I have a few high level design suggestions.

I would prefer to define an interface Binary to make it clear which methods are expected from the carrier. The Holder concrete class can be an implementation of that interface.
I'm not thrilled with the name Holder, perhaps Adapter
what might be even better is to define a single Adapters class that has a bunch of static factory methods, such as inboundTextMap(Map<String,String>), inboundBinary(byte[]). This way the helper/holder/adapter classes can be private, which is what std library does, e.g. Collections.

coveralls · 2017-11-30T19:59:22Z

Coverage increased (+0.8%) to 79.392% when pulling 1d5cb2a on carlosalberto:binary_adapter into b9feb48 on opentracing:v0.31.0.

carlosalberto · 2017-11-30T20:01:39Z

Hey @yurishkuro thanks for the feedback! So this is an updated PR with the mentioned changes (the only thing was that I left out for now the TextMap related methods for Adapters, as I'd love to initially have the very BINARY part tuned and ready ;) ) Let me know.

yurishkuro · 2017-11-30T20:41:42Z

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

@@ -0,0 +1,41 @@
+/*
+ * Copyright 2016-2017 The OpenTracing Authors


nit: as this is new code, (c) is only 2017 (same in other files)

:) Had missed that, will fix.

Oh, btw, if I use 2017 alone the build fails. Guess we need to issue a PR for fixing this part in pom.xml ;)

yurishkuro

+1 on the overall approach. Suggest soliciting other reviews, especially from people who were asking for binary format in the first place, and with at least these two questions:

agreement on the inbound/outbound terminology (if we could re-phrase in terms of inject/extract it would be better, less conceptual overhead)
the exact shape of read/write methods.

yurishkuro · 2017-11-30T20:45:55Z

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

+    /**
+     * @return whether this instance is outbound and supports writing.
+     */
+    boolean isOutbound();


are these required? We didn't have them in TextMap afaik. E.g. I don't know why a Tracer would want to call these.

Would be more of helper methods (as the Binary format either supports injection or extraction, not both). We can remove them if you think they don't bring enough value.

+1 to remove, does not add much value

Makes sense. Lets get rid of them.

yurishkuro · 2017-11-30T20:54:37Z

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

+     *
+     * @param b the data.
+     */
+    void write(byte[] b) throws IOException;


I don't have a strong opinion about the exact read/write API, but would it make more sense to model these functions after nio ReadableByteChannel and WritableByteChannel? They each have just a single method, but it's based on ByteBuffer, which I'm guessing makes them equivalent to several overloaded methods of Input/OutputStream, and certainly more flexible than write(byte[] b) (which requires an allocation if you're writing in batches of different sizes). Maybe it's not an issue for propagation codecs specifically.

Agreed. I believe ByteBuffer provides a greater level of flexibility while still being able to wrap (without copy) a byte[] with ByteBuffer.wrap

yurishkuro · 2017-11-30T20:59:36Z

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

+    /**
+     * @return A newly allocated byte array with the written data.
+     */
+    byte[] getOutboundData();


seems premature. Can always be implemented with a loop by calling read(), although less efficiently without knowing the full size of the payload.

So this is for the written data (when inject() was called), and this is supposed to be called after inject() to retrieve all those bytes. The alternatives is to let the user pass us a ByteArrayOutputStream (or similar) at Adapters.outboundBinary(). What do you think?

carlosalberto · 2017-11-30T22:07:49Z

Regarding the naming, what about Adapters.injectionBinary() instead of Adapters.outboundBinary(), and Adapters.extractionBinary() instead of Adapters.inboundBinary()?

malafeev · 2017-12-01T01:47:52Z

What's missing for me is MockTracer support. I would like to see tests with MockTracer inject/extract binary data.

gkumar7

This is a good start. Adding some tests specifically for the new format to opentracing-mock would be helpful. I feel what people need is simply a single wrapper for output and input streams which satisfies the tracer inject/extract api.

gkumar7 · 2017-12-01T04:34:33Z

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

+
+import java.io.IOException;
+
+public final class Adapters {


I do not see a real benefit of having this class. Simply calling the appropriate constructor of BinaryAdapter in a call to inject or extract should do.

The idea is to put other formats here, so we can abstract the actual implementations, so we don't have to expose BinaryAdapter and others.

gkumar7 · 2017-12-01T04:40:48Z

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

+    /**
+     * @return whether this instance is outbound and supports writing.
+     */
+    boolean isOutbound();


+1 to remove, does not add much value

gkumar7 · 2017-12-01T04:56:10Z

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

+
+        return inputStream.read(b);
+    }
+


Limiting the methods of ByteArrayOutputStream and ByteArrayInputStream to only read(byte[] b) and write(byte[] b) may limit tracing implementation libraries. Suppose this class simply had two getters:

public InputStream getInput() { if (inputStream == null) { throw new UnsupportedOperationException(); } return inputStream; } public OutputStream getOutput() { if (outputStream == null) { throw new UnsupportedOperationException(); } return outputStream; }

This would allow much greater flexibility to the tracing implementations (by not being rigid on what/how to read and when to close) and yet satisfy the api as defined by inject and extract.

I personally think exposing the actual stream objects would be overloading the API, trying to cover all cases and scenarios eventually. In any case, if this is truly needed, can be added in the future.

On a second thought: we may as well remove the write/read methods and end up exposing the streams too (after prototyping the MockTracer support for BINARY) ;)

gkumar7 · 2017-12-01T04:57:46Z

opentracing-api/src/test/java/io/opentracing/propagation/BinaryAdapterTest.java

+
+        byte[] buff = new byte[4];
+
+        assertEquals(4, binary.read(buff));


nit: let's define some constants

This is a very, very simple test, no need to put constants. Maybe we can have assertEquals(buff.length, binary.read(buff)) and that should be an improvement already ;)

carlosalberto · 2017-12-01T06:06:59Z

@malafeev Oh, sure, will cook some examples - the thing is that I need to add support for the BINARY format itself for MockTracer (which hopefully can be done soon).

coveralls · 2017-12-06T07:44:38Z

Coverage increased (+1.9%) to 80.523% when pulling 5f10075 on carlosalberto:binary_adapter into b9feb48 on opentracing:v0.31.0.

malafeev · 2017-12-06T07:51:08Z

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

+                        binary.write(ByteBuffer.wrap(stream.toByteArray()));
+
+                    } catch (IOException e) {
+                        throw new IllegalArgumentException("Corrupted data");


shouldn't be RuntimeException?

Makes sense.

malafeev · 2017-12-06T07:52:19Z

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

+     */
+    public static Binary injectBinary(OutputStream stream) {
+        if (stream == null)
+            throw new IllegalArgumentException("stream");


isn't better "stream cannot be null" message?

I would use curly braces in all if blocks even with only one statement

Oh, definitely. Missed this one.

malafeev · 2017-12-06T08:47:54Z

@carlosalberto great!

coveralls · 2017-12-06T08:48:19Z

Coverage increased (+1.9%) to 80.523% when pulling 91c999e on carlosalberto:binary_adapter into b9feb48 on opentracing:v0.31.0.

carlosalberto · 2017-12-06T08:53:57Z

This is an updated PR including:

ByteBuffer for read()/write().
injectBinary() and extractBinary() names for the adapters (instead of inbound/outbound)
(Basic) BINARY propagator for MockTracer (using the old ObjectDataStream classes).
Overloads taking both streams and channels in Adapters.

With the support for BINARY in MockTracer it should be easy to test this approach out and verify it's good enough. Let me know. Hopefully we are closer to the final PR ;) @yurishkuro @malafeev @gkumar7

malafeev · 2017-12-06T08:56:20Z

@carlosalberto LGTM
I would like to include it to RC2 to test

yurishkuro

LGTM

* Change BINARY to be resizable and stream-oriented. * Abstract the Binary adapter and have an Adapters class to return it. * Remove the isInbound/isOutbound methods from BinaryAdapter. * Make Binary use the Channel paradigm for injection/extraction * Have binary methods in Adapters named after inject/extract. * Add a BINARY propagator for MockTracer. * Throw RuntimeException in case of errors during BINARY's injection. * Put braces around if-blocks for Adapters/BinaryAdapter. * Use verbose messages for null parameters in Adapters.

@deprecated

* Code cleanup (opentracing#199) - Propagate @deprecated annotation to implementations - Remove redundant "static final" definition from interface - Use Collections.emptyMap instead of Collections.EMPTY_MAP to preserve type * Add support for multi reference and expose references context and type from MockSpan. (opentracing#213) * Publish test artifact for Globaltracer testutil (opentracing#209) * Fix MockTracer.ChildOf not fail if there is a null argument passed (opentracing#226) * Make ChildOf not fail if there is a null argument passed * Moved tracer test to where it belongs. Fixed typo. * Use correct reference in Javadoc (opentracing#230) * MockTracer use text map propag in the default constructor (opentracing#179) * Implement a simpler Span propagation without lifetime handling. (opentracing#188) - Scope replaces ActiveSpan, without sharing a common ancestor with Span. - ScopeManager replaces ActiveSpanSource. - No reference-count to handle Span's lifetime. - A simple thread-local based ScopeManager. * travis publish.sh allow releases from branches (opentracing#191) * Travis allow release from non master branches (opentracing#192) * Travis allow release from non master branches Publis script compares remote branch with current checkout. This passes travis_branch into git checkout command so it will compare the same branches. * Fix comments * Travis publish script, remove -RC on git checkout (opentracing#193) * Update the Travis script to allow publishing from v.0.0.0 branches. (opentracing#195) Thing is, we cannot publish from 0.0.0-style branches as they are excluded, based on the current global Travis configuration, thus we try to publish from branches following a v.0.0.0 style, if any. * Readme document release process for release candidates (opentracing#198) * Readme document release process for release candidates * Adjust publish.sh to work with release from master branch * Add Examples for async use cases (opentracing#197) * Add examples for async as test cases This includes execution flow similar to: * Actor ask/tell * Promises with callbacks * Work interleaved on a thread using suspend/resume. The implementations of these execution models are obviously very simplistic, but intended to emphasize the tracing aspect. * Update README files to reflect the Scope concept. (opentracing#217) * Let Tracer.activeSpan() be a shorthand for ScopeManager's active Span. (opentracing#228) * Let Tracer.activeSpan() be a shorthand for ScopeManager's active Span. * Document the null case for Tracer.activeSpan(). * Have Tracer.activeSpan() return null. * Remove @link for Tracer.activeSpan(). 'Nested' links do not happen to exist under javadoc, so Tracer.scopeManager().active().span() is now not a link. * Change BINARY to be resizable and stream-oriented. (opentracing#223) * Change BINARY to be resizable and stream-oriented. * Abstract the Binary adapter and have an Adapters class to return it. * Remove the isInbound/isOutbound methods from BinaryAdapter. * Make Binary use the Channel paradigm for injection/extraction * Have binary methods in Adapters named after inject/extract. * Add a BINARY propagator for MockTracer. * Throw RuntimeException in case of errors during BINARY's injection. * Put braces around if-blocks for Adapters/BinaryAdapter. * Use verbose messages for null parameters in Adapters. * SpanBuilder deprecate startManual (opentracing#225) * SpanBuilder deprecate startManual * Fix review comments * Remove default finish behaviour for `activate()` (opentracing#219) * Do not auto finish on scope.close * Fix review comments * Fix review comments * Add explanatory statement about not auto-finishing * Define only activate(s, bool) * Use the parameterless startActive() in a forgotten test in MockTracerTest.

Change BINARY to be resizable and stream-oriented.

87d1782

Abstract the Binary adapter and have an Adapters class to return it.

1d5cb2a

carlosalberto force-pushed the binary_adapter branch from 2182e75 to 1d5cb2a Compare November 30, 2017 19:57

yurishkuro reviewed Nov 30, 2017

View reviewed changes

carlosalberto mentioned this pull request Nov 30, 2017

Variable size ByteBuffer implementation #99

Closed

gkumar7 reviewed Dec 1, 2017

View reviewed changes

carlosalberto added 4 commits December 1, 2017 19:06

Remove the isInbound/isOutbound methods from BinaryAdapter.

ab9dfd9

Make Binary use the Channel paradigm for injection/extraction

1a95d80

Have binary methods in Adapters named after inject/extract.

6aadaec

Add a BINARY propagator for MockTracer.

5f10075

malafeev reviewed Dec 6, 2017

View reviewed changes

carlosalberto added 3 commits December 6, 2017 02:34

Throw RuntimeException in case of errors during BINARY's injection.

f7dac8e

Put braces around if-blocks for Adapters/BinaryAdapter.

9110201

Use verbose messages for null parameters in Adapters.

91c999e

yurishkuro approved these changes Dec 6, 2017

View reviewed changes

malafeev approved these changes Dec 6, 2017

View reviewed changes

Merge branch 'v0.31.0' into binary_adapter

d9eb929

tedsuo merged commit 89460bd into opentracing:v0.31.0 Dec 6, 2017

gkumar7 mentioned this pull request Dec 6, 2017

Add a new Binary Format to allow for variable SpanContext size #95

Closed

carlosalberto deleted the binary_adapter branch December 17, 2017 23:48

tedsuo mentioned this pull request Dec 20, 2017

0.31.0 Release Candidate #189

Closed

carlosalberto mentioned this pull request Jun 2, 2018

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

Merged

		@@ -0,0 +1,41 @@
		/*
		* Copyright 2016-2017 The OpenTracing Authors


		byte[] buff = new byte[4];

		assertEquals(4, binary.read(buff));

Change BINARY to be resizable and stream-oriented. #223

Change BINARY to be resizable and stream-oriented. #223

Conversation

carlosalberto commented Nov 30, 2017

coveralls commented Nov 30, 2017

yurishkuro commented Nov 30, 2017

coveralls commented Nov 30, 2017

carlosalberto commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypnoce Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Nov 30, 2017

malafeev commented Dec 1, 2017

gkumar7 left a comment • edited Loading

Choose a reason for hiding this comment

gkumar7 Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Dec 1, 2017

coveralls commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malafeev commented Dec 6, 2017

coveralls commented Dec 6, 2017

carlosalberto commented Dec 6, 2017

malafeev commented Dec 6, 2017 • edited Loading

yurishkuro left a comment

Choose a reason for hiding this comment

hypnoce Dec 1, 2017 •

edited

Loading

gkumar7 left a comment •

edited

Loading

gkumar7 Dec 1, 2017 •

edited

Loading

malafeev commented Dec 6, 2017 •

edited

Loading