Add support for the LightStep binary format in SpanContext propagation. #40
Conversation
This change adds support for the LightStep binary format (a protocol buffer-based encoding) of OT SpanContext in the LightStep tracer. LSBinaryCodec contains all functions that performing encoding and decoding of this format, which is a set of bytes that are then base64 encoded.
Pod/Classes/LSBinaryCodec.m
Outdated
| } | ||
| } | ||
|
|
||
| + (void)writeFixedEncodedUInt64:(UInt64)number buffer:(NSMutableData *)buffer { |
There was a problem hiding this comment.
it would be easier to grok/review if we move the truly generic stuff (like this) into its own compilation unit
There was a problem hiding this comment.
Done - these are now moved to LSPBUtil
Pod/Classes/LSBinaryCodec.m
Outdated
| return keyInfo; | ||
| } | ||
|
|
||
| // Reads a varint off the proto bytes, advances the offset to the next byte after the varint. |
There was a problem hiding this comment.
assuming you are basing each of these helper functions on C/C++ code that lives elsewhere, can you link to those function definitions in each function comment?
There was a problem hiding this comment.
not really - they're based on my reading of the Protobuf encoding "spec"
| return false; | ||
| } | ||
|
|
||
| while (head < tail) { |
There was a problem hiding this comment.
seems like corrupt data could lead to an infinite loop if mapKeyInfo.length == 0... I'm not reading all of this defensively, but I think we should audit this code to check that corrupt data doesn't lead to actual crashes or infinite loops (and attempt to defend against them: e.g., in the mapKeyInfo.length == 0 case, we would fill out errorPtr with something and return).
There was a problem hiding this comment.
Yes, corrupt data could lead to an infinite loop here. There's also a bug where we weren't advancing the offset correctly after reading a baggage string.
| return outer; | ||
| } | ||
|
|
||
| + (BOOL)decodeMessage:(NSData *)protoEnc |
There was a problem hiding this comment.
thinking out loud: would it make more sense to only include the encode path for now? realistically the iOS app has no need to decode a context/baggage ~ever, and the less code we have here the fewer errors there might be.
There was a problem hiding this comment.
It's actually slightly easier to test if I can decode too - what "should" a given message look like? Kind of a pain to generate the test cases...
There was a problem hiding this comment.
This also gives us a road to server-side swift integration, which is coming: http://perfect.org/
There was a problem hiding this comment.
It should not be very difficult to separate the encoder and decoder into separate libraries, though. It's either a C-preprocessor, BUILD-system problem, or pragma thing.
#if LIGHTSTEP_CARRIER_ENCODER
...
#endif
There was a problem hiding this comment.
Any advice about what to do about code that calls the decode path? What's the right way to switch on that? Would it also be to use #if or something else?
There was a problem hiding this comment.
I don't think we need to separate them. I was just thinking about minimizing the surface area for this code entirely... i.e., it's not about the binary size or whether they get linked in, but just whether we need to maintain it.
I did have one thought overnight about this (ah, insomnia): we could skip baggage for v1 of this code (fine to keep it around in a git branch or whatever). Once all parties feel confident about the "flat" varint stuff, we could then introduce baggage and unittest the hell out of it, including intentionally corrupt data and so forth.
|
I've also created a no-baggage, encoding only version of this PR: I left the utilities for writing a string map in place there for now. |
|
This issue is ancient, and I imagine the repo itself is rarely used. |
This change adds support for the LightStep binary format (a protocol
buffer-based encoding) of OT SpanContext in the LightStep tracer.
LSBinaryCodec contains all functions that performing encoding and
decoding of this format, which is a set of bytes that are then base64
encoded.