opentracing-cpp re-redux (virtual inheritance)

[jquinn47]

Picking up from #6 and based off our google hang out chat, it sounded like we wanted to experiment with a virtual inheritance model:

• It would be simpler to understand
• It would allow for multiplexing tracers

The only problem @SaintDubious and I saw with this approach originally was that having the Tracer be responsible for being the glue between Span/Contexts and Carriers had some unfavorable physical side effects when it came to library organization.

Now @jmacd's idea is a good one: we should be able to shift this burden into the carriers themselves, avoiding adding all library organization problems introduced by the previous implementations layout.

This PR is a rough draft outline of what that might look like. The pain point at the moment is carrier code. Say we have RedTracer and RedContext. Application code wouldn't look surprising:

Span * s = tracer->start("op");
XyzWriter writer;
tracer->inject(&writer, *s);

Injects could be specialized on the implementation type:

class XyzWriter : public Writer {
    virtual void inject(Context* context){
         if(RedContext* rs = dynamic_cast<RedContext*>(context){
               // RedContext specific code...
         }
         else {
              // If we aren't expecting the type, fallback on a contexts ability to externalize itself
              vector<TexPair> textmap;
              context->externalize(&textmap);
             // store textmap into the carrier message...
         }
    } 
};

Extracts are a bit more confusing. If a carrier needs to support multiple tracer implementations, it would have to do some extra checks to make sure the type it received over the wire is the same type as its being asked to extract into.

XyzReader: public Reader{
     virtual int extract(Context * context){
          if(RedContext * rs = dynamic_cast<RedContext*>(context)){
               // attempt to extract red span from the wire. If that fails, now what?
               rs->setCoolRedSpanDetailsDirectly(...);
          }
          else if(receivedTextMap){ 
                context->reset(textmap);
          }
          else if(receivedBinary){
                context->reset(binaryBlob);
          }
          else {
               //failure
          }
      }
};

There is a very ambiguous contract how Readers/Writers interact here and the upgrade path gets messy. If we add a BlueContext, all applications need to be upgraded to understand both, then over time, decommissioning RedContext. For that to work, all the carrier messages need to be able to inject a single context multiple times, in different formats, and extracts need to be on the look out for the same. All of this is doable, but gross.

As we discussed in the hangout, this is a well known upgrade/migration technique. I don't see anything too crazy about it. The only downside is that every carrier implementation has to get it right now, instead of a single tracer library.

As one final footnote, @SaintDubious and I were discussing how Bloomberg would do this. We would likely define a single BloombergSpanContext type, and have all of our carriers perform a static_cast<BloombergSpanContext*>. I do not think we'd take advantage of the carrier fallback feature set, but using CRTP when we have a simple alternative that benefits other organizations seems like overkill.

[yurishkuro]

class XyzWriter : public Writer {
    virtual void inject(Context* context){
         if(RedContext* rs = dynamic_cast<RedContext*>(context){
               // RedContext specific code...
         }

It seems to me there is a misunderstanding of what "custom carriers" are meant to achieve. Carriers are used by instrumentation code. The primary objective of OT spec is to make instrumentation code portable. The above example is the absolute opposite of that, it makes instrumentation code dependent on the specific tracing implementation.

To me, the custom carriers are not supposed to know the implementation details of the tracer. They are "custom" because they are specific to the current framework being instrumented with OT, e.g. an RPC frameworks that either has a specialized wire protocol, or more likely just a different abstraction of the "request" object that needs an adapter in order to satisfy the standard carrier interfaces that all tracers are supposed to support (text map, http headers, binary).

[bhs]

A few misc thoughts (not a detailed review since I know this is just a sketch)

[bhs]

I'd prefer that we leave deallocation out of the Tracer interface... seems like a preoptimization and a possible point of confusion

[bhs]

why not just Span::getContext?

[bhs]

why do implementations need to create their own versions of SpanOptions? I would have expected it to be a glorified struct, implemented directly in the opentracing lib.

[bhs]

as discussed here and there, this has intentionally been left out of the OT spec... happy to discuss again if you want :)

[bhs]

re the various log methods... maybe we could do something like the zero-allocation approach in opentracing-go? in any case, it's important for log to support multiple key:value fields per log call.

[tedsuo]

Hi @jquinn47, wondering if I can close this PR. Please feel free to open a new issue when it's time for another take at a C++98 API.

[lookfwd]

sure, We can close this I think

…

On Mon, Sep 4, 2017 at 3:53 PM Ted Young ***@***.***> wrote: Hi @jquinn47 <https://github.com/jquinn47>, wondering if I can close this PR. Please feel free to open a new issue when it's time for another take at a C++98 API. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAwLbyuiEVMSGgMLyk43Zs3xfHzSu7c7ks5sfFVAgaJpZM4LtEXT> .