opentracing-csharp
opentracing-csharp copied to clipboard
opentracing
Carrier type for binary format
comment from @yurishkuro :
is byte array the right carrier for binary? I assume byte array has a fixed length, so especially for the injector how does the instrumentation know how long the tracing context in binary encoding will be? I don't know the native IO types in C#, but I would assume some sort of a byte buffer or i/o stream to be a better carrier than byte array.
I haven't looked into this yet so this is still open. @dawallin do you have any experience with that?
No I have not. I postponed all about the binary carrier. How are you supposed to use it? Using a stream makes sense if the client are streaming something from the first place, but I don't see that use case. Usually the client starts where everything is already serialized.
In that case I would like to remove the "BINARY"-carrier code from the library for now to not block a 0.9.0 release. We'll look into this later.
Is this ok for everyone?
I removed the binary format for now. We'll put it back once we know the correct type.
My two cents as a potential contributor: if the buffer can't be known in advance for Inject, I'd suggest using MemoryStream, which is basically a resizable byte[], and it even can return such underneath byte array through GetBuffer(). In a way it's the equivalent to Java's ByteBuffer.
Else, if a base Stream could be used instead (for simplicity).
#50 will add the binary format using Stream as a type constraint. I don't know if that's correct so if anyone has any complaints don't hesitate to comment here or at #50
Yeah Stream deviates from what Java is doing but is >>> for performance :)
That said, I shouldn't have done it in that review since it's a deviation :(
@ndrwrbgs Also, we had to remove (for now) the new implementation of Binary, as some people were worried the current approach was not the best one (currently being tracked in https://github.com/opentracing/opentracing-java/issues/253).
I'd personally leave it out for now, and wait for the Java one to be (eventually) done and then ported here (even if deviated, by that time).
@carlosalberto it sounds like most of the questions in that issue are actually address by using Stream. If we are EVER going to support binary formats at the interface level, in C# Stream is the only sensical interface I feel, it supports Size (if available), non-buffering, and anything else you'd want to do with potentially streaming data.
That said, I'm not tied to this as my own tracing efforts haven't gotten to wire formats yet :) But I do think this is one of those cases where we can potentially go faster than java because our path is well defined by the ecosystem in regards to how to handle streaming (or buffered) data.
Alright, then let's leave the binary format out for now. It's better to add it once we have a clear understanding of how it should be used. I've seen that @ndrwrbgs has already removed it from his PR. I'll also update the release plan.
@ndrwrbgs not quite - some of us are worried exposing an actual, full Stream object will be too much (hence the iterations involving simple methods for writing/reading). And we need to try out with actual code in Java before we move on and decide ;) Thanks for removing it, and hopefully we will revisit this in the (near) future!
I'd like us to get around to adding this; I've created #116 that defines the binary format as a Stream. This seems like the most appropriate analogue for Java ByteBuffer and what I've seen in Golang/JS (Binary or Buffer)
