Support propagation directly to/from strings.

[rnburn]

Adds functions to support propagation directly to/from strings so as to allow tracers to use more efficient implementations.

Includes default implementations for the binary string Inject/Extract functions that use iostream's so that tracers aren't required to provide implementations for them.

Addresses efficiency concerns that came up when instrumenting envoy for OpenTracing.

[yurishkuro]

so as to allow tracers to use more efficient implementations

Not sure I understand. (a) Binary format is not supported by many implementations. (b) What is the meaning of using a binary codec to write to an ostringstream - won't you end up with garbage?

[rnburn]

(a) Isn't binary one of the required propagation formats for OpenTracing? It's supported in the other API's as well. Envy has a custom single-header propagation mode that uses a base64 encoding of the binary span context representation.

(b) stringstream's can be used with arbitrary binary data. They don't have to correspond to ASCII or any other printable format. (See https://stackoverflow.com/q/47125387/4447365)

[yurishkuro]

(a) Isn't binary one of the required propagation formats for OpenTracing? It's supported in the other API's as well.

Yes, it is "required" yet often not implemented because there are hardly any transport protocols where binary is actually required. Pretty much all instrumentation for HTTP protocol uses HTTP_HEADERS format, sometimes TEXT_MAP format is used for protocols that guarantee that keys and values do not get mangled.

Envy has a custom single-header propagation mode that uses a base64 encoding of the binary span context representation.

Hm, that makes it incompatible with anything that's not going through Envoy. E.g. if you have a remote client (mobile or maybe an API client) talking to the backend over HTTP, the normal way for that client to pass the context is using the tracer's HTTP_HEADERS format.

I'm not suggesting you stop doing this, it's just an interesting development I wasn't aware of. The OT way is pretty much "tracer rules them all", there is no standard on how the context is represented on the wire, not even which headers are used. The TraceContext Spec is almost complete opposite, it standardizes both the headers and the values. And Envoy's approach is in the middle, standard header, but 100% custom value.

[yurishkuro]

question directly related to the PR though - why extend the tracer API with string& writer instead of putting that code in Envoy?

[rnburn]

I added an option in the OT PR so that users can configure their tracer to propagate using it's native format if they want, but the custom format is still there since Envoy has been using that for a while.

Their propagation method was discussed quite a bit here for context. With Zipkin, they actually propagate twice, both in their custom format and Zipkin's native B3 format.

[rnburn]

question directly related to the PR though - why extend the tracer API with string& writer instead of putting that code in Envoy?

The point of the string overloads is so that tracer's might implement those functions directly to propagate more efficiently. The code I provided is just a default fallback to iostream Inject/Extract if tracer implementations don't want to bother with also writing Inject/Extract for strings.

[yurishkuro]

The point of the string overloads is so that tracer's might implement those functions directly to propagate more efficiently.

But you are transposing Envoy's non-standard (from OT perspective) approach into OT API. There is no standard format in OT for "string blob". If the other side of the connection is an OT tracer from the same tracing system but in different language, it won't be able to deal with the string blob.

[yurishkuro]

thanks for the link, btw - I saw the beginning of that discussion, but not how it ended (should've subscribed).

[rnburn]

Yes, that is a drawback of how they're propagating. I think at Lyft they do this form of custom propagation in their other languages as well; so provided the binary formats in the different languages match, they can still propagate.

Nothing about their single header format is leaking into this PR, though. It's only about allowing for the already supported binary format to be more efficient.

[rnburn]

For reference: this is how the proposed Envoy OT PR propagates. It supports both Envoy's custom format and the tracers native http format.

[yurishkuro]

I'm just suggesting that you can do the exact same thing directly in Envoy - use the binary/ostream API but pass an ostringstream. No OT API in other languages define codecs for a string blob. Efficiency here == utility ==> does not need to be part of the API (because it's confusing and unexplainable based on the OT spec).

I am not even sure why you need the string in the first place - base64 encoding is fundamentally defined over []byte not strings (could be a quirk of the specific encode you're using).

[rnburn]

Right, but std::string is the closest thing you have to a []byte array in C++11. A separate std::byte type was only added in C++17. Using std::string like this to package binary data is fairly standard practice.

Envoy's concern with std::ostringstream was that it requires you to make unnecessary copies of the data. With std::string, a tracer can potentially propagate more efficiently by calculating how much space it needs and allocating memory only once.

[rnburn]

For comparison, Google's C++ Protobuf library also provides functions that serialize directly to/from std::string as well as iostreams.

[tedsuo]

This looks to me like an efficient implementation for binary propagation in C++. Can we focus on whether this is the correct way to implement binary in C++, and leave aside discussion of the utility of the binary transport?

[yurishkuro]

It's not about the utility of the binary format, merely about the API design. We already have an f(X) in the API, but we're trying to add g(Y) which is trivially expressible in terms of f. So we're increasing the API surface without any new functional requirement, merely as a convenience, a convenience that can just as easily be implemented as a static utility function elsewhere.

[rnburn]

@yurishkuro That's not quite true. The Inject/Extract functions are virtual, so tracer libraries can provide their own more efficient implementations if they want.

Since they can be expressed in terms of the iostream Inject/Extract functions, the OT library does provide a default implementation so that libraries aren't required to implement them.

But the whole point of the PR is to allow for more efficient implementations if you're using strings for binary propagation.

[yurishkuro]

so tracer libraries can provide their own more efficient implementations if they want.

Rule of Three - do we have 3 library providers who are interested in overriding this method in order to use more efficient implementation?

[isaachier]

I tend to agree. I have seen too many C++ programmers worried about streams vs. strings and strings vs. string views/refs when that is the least of their performance concerns. My fear is that the C++ implementation will become too C-like if performance is given priority over elegance/simplicity of use. If there is a significant performance issue and/or it is a deal-breaker for those considering OpenTracing, I would reconsider.

[rnburn]

I'll close this since envoy was ok merging in a version using iostreams.

But I do think that unnecessary memory allocations are absolutely something to be concerned about in the API.