Switch to a C++11 API

[rnburn]

This PR puts in c++11 OpenTracing API based off of @jmacd’s LightStep tracer.

For the most part it follows the design of the OpenTracing APIs in other languages. A few exceptions are

• It removes the format parameter from Inject/Extract, instead choosing the simpler design described here where only a carrier is passed in.
• It allows span methods SetTag, Log, SetOperationName, etc to be called after a span is finished (but doesn't specify behavior) and permits Finish to be called multiple times so that instrumentation code isn’t required to do additional synchronization. (See discussion here).

I wrote added a version of LightStep’s tracer that implements the API and there’s an instrumentation of nginx that uses the API if anyone wants to see examples of what it looks like in practice.

[tedsuo]

@lookfwd sums it up pretty well: no one is using the current API. Attempts to create a perfectly efficient C++98 API struggled to retain the abstraction/decoupling that the OT concepts require while matching the efficiency that could be gained from having no abstractions. In the long run a C API would be very useful, but in the short run it turned into a case of "perfect is the enemy of good enough." The end result was that C++ tracers have not been binding to the OT API, and systems which were interested in tracing have been using these non-OT interfaces.

[jmacd]

For the record, I reviewed this code and like it.
I believe that it makes sense to maintain both a "modern" C++ library and a C++98 library, they're different enough to keep the code bases separate. The value of sharing code between these libraries is small, unless it is to achieve interoperability, and I think we can approach that topic when it arises.

[lookfwd]

I believe that it makes sense to maintain both a "modern" C++ library and a C++98 library

I would agree, assuming there's anyone wanting a C++98 lib :)

[bhs]

What happened to the binary carrier? cc @jmacd

(I focused on the Tracer API, btw – mostly Inject/Extract)

[bhs]

I am stuck in C++98-land, so I apologize if this is stupid: it seems weird to me to add a dependency like this to the core OpenTracing API. AFAICT, expected is becoming / becomes part of std in C++17, is that correct? Might it make more sense to just disallow exceptions in OT impls for the time being??

If the answer is some variant of "this is how people program in C++11", do not bother being verbose about it – I won't argue back, just wanted to ask the question.

[rnburn]

Expected is meant to support non-exception-based error handling. It's used to deal with invalid_contexts, corrupted spans, invalid carriers, etc. C++11 added std::error_code that provides a standard way to make descriptive user errors; expected gives you a way to return such errors from a function.

The expected implementation used is just a small header-only library that's bundled along with the code (like mapbox::variant). There aren't any external dependencies.

[rnburn]

I thought that the binary built-in format was a value that was reserved for later use. I didn't see a carrier interface defined for it in the other languages. But if there consensus on how to define such an interface, I can add it in a similar way to the TextMap interfaces.

For custom binary carriers that are tied to specific tracers -- those are meant to be handled by the CustomCarrier. (See for example LightStep's binary carrier).

[bhs]

I thought that the binary built-in format was a value that was reserved for later use. I didn't see a carrier interface defined for it in the other languages.

https://github.com/opentracing/opentracing-go/blob/master/propagation.go#L49 is an example, but there are similar things defined for Java, Python, etc. Also see https://github.com/opentracing/specification/blob/master/specification.md#note-required-formats-for-injection-and-extraction

Thanks!

[rnburn]

Would std::istream, std::ostream work for binary carriers? You could then do something like

std::ostringstream oss(std::ios::binary);
tracer->Inject(sc, oss);
auto blob = oss.str();

if you wanted to get a byte array of the data.

[rnburn]

I added Inject/Extract methods that take streams for the binary propagation.

[jmacd]

The stream-carrier methods look good. @bhs the reason binary carrier was left out, I suspect, is that I left it out of lightstep-tracer-cpp.

The question about which dependencies are acceptable is a good one. On the one hand if we're calling this a C++11 API we should stick to the standard C++11 library. On the other hand, the parts incorporated here, included in the C++14 and C++17 specs (i.e., variant types, expected types, and the stringref type) are being used to make the OT API idiomatic while C++ continues to evolve.

In these cases specifically, I favor the use of a feature but perhaps we can address @bhs's concerns as follows:
(a) retain any copyright notices, but copy the headers into an opentracing/feature to clean the include path; use an opentracing::feature namespace.
(b) use forward-compatible names so that when C++14 or 17 come along with these features standardized, it's a simple namespace change to adapt the code
(c) use the c++ preprocessor to ensure that a user who has their own copy of similar post-C++11 features in their code base can supply their own, assuming naming compatibility as in (b)--e.g., if mapbox wants to use this library, they'll definitely not want our fork of their own variant type.

It should be pretty easy to make these changes, the one thing I'd like to recommend is that the StringRef type be named something more like string_view for C++17 compatibility.

[rnburn]

I updated StringRef and Expected to use their c++ standard names (string_view, expected), changed the namespaces to opentracing so that they won't collide if someone uses one of the 3rd_party packages directly, and simplified the include paths.

[lookfwd]

My only thought is to somehow have all those to a v1.0 namespace and folder. I would think that when OT 2.0 etc. comes, one would still like to link and us the old API. What are your thought about supporting multiple OT versions?

[rnburn]

I do have an inline namespace that specifies the ABI version. I would think using branches might be a better way to manage the different versions than directories.

If we want to support installing multiple versions concurrently, though, maybe I should add some sort of version tag to the library name so that the installed libraries don't overwrite one another.

[lookfwd]

If we want to support

It would be nice to be some discussion around this. This API is a contract between both tracers and client applications and I would guess it's quite difficult to upgrade them all in an single operation. I would expect different major versions to co-exist. I'm not sure how important is this for people though.

[yurishkuro]

I'm not sure how important is this for people though.

API stability is one of the top goals, if not #1.

[rnburn]

I updated CMakeLists.txt to set version properties for the opentracing shared library (following these guidelines). It should allow older versions of the library to coexist with newer versions.

I couldn't find any similar feature to manage versioning of the static library, but I suppose that's less of an issue.

[tedsuo]

@jmacd @jquinn47 @yurishkuro @SaintDubious we are feeling confident about this, and would like to merge if there are no objections. We will version it as v0.1 and move to bindings for ZipKin and LightStep, followed by Envoy instrumentation.

[jmacd]

👍

[lookfwd]

@jquinn47 @SaintDubious shouldn't have problem, since as far as I know they don't link to this.
👍