opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 344 forks source link

Binary support design #253

Open carlosalberto opened 6 years ago

carlosalberto commented 6 years ago

A pair of previous PR have shown both the need to have a better Binary support than the one we have now, as well as the need to have a proper design process to achieve it (which should include proof of concept and actual sample implementations in some Tracers).

The requirements have been so far:

Over a PR that happened prior to the 0.31 release, we came upon a specific design that is simple, but may still be incomplete - see #252 . This design so far can be summarized in the PR itself, but there are a few issues in the air.

Issues/questions

Please feel free to join an provide feedback (specially if you can manage to provide specific examples or a proof of concept). Hopefully we can roll this change as part of 0.32 ;)

carlosalberto commented 6 years ago

Based on the status of #252 I'd like to get this moving on a more pragmatic way for now, considering the needs at hand. As part of that, I think it would be nice to have a simple-but-working approach now, instead of simply passing/getting around a ByteBuffer. Later on, a more complex, (full) streaming one can be designed and implemented as/if needed.

Looking in retrospective, there was an initial effort to have something on top of ByteBuffer: #99 Maybe we could have something like that? One advantage is that it would be a very simple take, and we could even decide to let the user know in advance how many bytes are required for the output (which can be of help when calling ByteBuffer.allocate())

I also remember @yurishkuro was not very fond of this approach - in that case, we could stick to the approach that exists in #252 which is a simple write/read one.

Opinions on this? @tedsuo @yurishkuro CCing @tylerbenson (in case you are interested ;) )

carlosalberto commented 6 years ago

Hey everybody,

I went and worked on a simple approach for the BINARY format, as a simple alternative to the current proposed write()/read() approach.

276 defines a single wrapper on top of ByteBuffer to let the users get a hint of the required buffer length for Tracer.inject(), which has been previously stated as one of the reasons to have a new Binary format. This is a rather simple but working approach, and could let free space for a future streaming implementation if/when needed (maybe called BINARY_STREAMING), either as a write()/read() approach, or one based on IO streams.

This way we could close/fix this issue - with the requirements and scenarios we have at this time.

Thoughts? @yurishkuro @tedsuo @tylerbenson

tylerbenson commented 6 years ago

@carlosalberto Unfortunately, I haven't had any experience trying to implement propagation over a binary protocol, so I don't really feel like a good authority on this. Perhaps @raphw is a better person to weigh in?

carlosalberto commented 6 years ago

@tylerbenson Oh, no worries. Definitely we need a simple, working approach for now (needed, for example, for instrumenting Cassandra on the server side). @raphw needs a more advanced, streaming-based approach that we could add later on, definitely, once we hit that need ;)

Hope this makes sense.