Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Binary support design #253

Open
carlosalberto opened this issue Jan 19, 2018 · 4 comments
Open

Binary support design #253

carlosalberto opened this issue Jan 19, 2018 · 4 comments
Assignees

Comments

@carlosalberto
Copy link
Collaborator

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:

  • A size-variable output for Tracer.inject().
  • A simple and clear implementation.

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

  • The current approach for inject/extract takes for granted that all data will be stored in memory first (in a buffer, that is), and only then passed to the transfer protocol. This is how currently TEXT_MAP and HTTP_HEADERS work. Is this something enough for BINARY? Or do we need to support a "streaming" version of this API?

  • The need for calculating the size of the output (for inject) was also requested (mostly related to the 'streaming' request in the previous point).

  • Another request was to expose, for the new Binary format, the actual InputStream and OutputStream (instead of write and read). The overall idea so far for this is that this would be an overkill, design wise, as it would expose operations that are not required at all for this case.

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
Copy link
Collaborator Author

carlosalberto commented May 18, 2018

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
Copy link
Collaborator Author

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
Copy link

@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
Copy link
Collaborator Author

@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants