Discrepancy: Join can create a new trace in Java, but not in Go/Python

[yurishkuro]

Documentation for Join method says

// If the carrier object has no such span stored within it, a new Span is created.

This is different from Go/Python implementations where Join operation results in an error if the incoming request contains no trace context. Java versions provides no hook for instrumentation to decide if it wants a new trace started if one didn't come from upstream. On the other hand, it makes instrumentation code shorter by not forcing the end user to do:

try {
    tracer.join(...)
} catch () {
    tracer.startSpan(...)
} 

I think we should be consistent in the semantics of Join operation across languages.

@bensigelman @michaelsembwever @oibe

[bg451]

I feel a bit uneasy about Join automatically creating a new trace if it can't join one. I'd much rather let the user determine what to do if it can't join the trace. Let's say a bunch of RPCs are made in the order A→B→C→D, and C, which should never be a root span, can't join the trace for whatever reason, i'd prefer to log the error and then figure out what I want to do.

Per the try {...} catch () {...}, something like JoinOrCreate(...) can be added.

edit: tldr; I prefer the Go/Python semantics

[yurishkuro]

@bg451 I agree. I prefer Go/Python behavior, it keeps the interface to minimal functionality.

[bhs]

+1; there is real value in a method that attempts to join and falls back on a new (root) span, but that shouldn't be the lowest-level API.

[michaelsembwever]

I'm no fan of asking the client code to use try-catch.

I'd suggest either have join(..) return a NoopSpanBuilder, or change the signature on join(..) to

Optional<SpanBuilder> join(..)

Or add to SpanBuilder the method isValid() (and when false start() would return the NoopSpan).

Otherwise the code isn't determined one way or another, as it's the registered Extractor and SpanBuilder that will throw the exception when there isn't enough, or valid, traceState.

[michaelsembwever]

Coming back to this my thoughts favour returning NoopSpanBuilder.

It works well when the typical use-case is not to instrument if trace exists already.
(Because otherwise you still need to use a NoopSpanBuilder so following instrumentation can be written plainly without conditionals).

And when instrumentation wants to be started despite no existing trace then it's to

SpanBuilder builder = tracer.join(...);
if (NoopSpanBuilder.INSTANCE == builder) {
    builder = tracer.buildSpan("operation-name");
} 

[tinkerware]

The join method throwing an exception seems to misuse exceptions as control flow; for most code there's nothing unexpected about missing context for optional instrumentation.

Using Optional would have been the best choice, but I see that this project has dual Java 7/8 targets, so we would have to use a substitute for Java 7.

Returning a NoopSpanBuilder that callers have to check for identity is surprising Java API. Using the null object pattern is common enough, but requiring callers to check for it would go against the expectations of Java developers.

If the typical usage of this method is for the callers to almost always check the return value, how about SpanBuilder joinOr(T carrier, SpanBuilder.Supplier fallback), where SpanBuilder.Supplier looks like:

interface SpanBuilder {
  interface Supplier /* extends java.util.function.Supplier<SpanBuilder> on Java 8 */ {
    SpanBuilder get();
  }
  ...
}

// Assuming hypothetical global tracer API, extract context or use root span
Span span = tracer.joinOr(carrier, GlobalTracer::rootSpan).withOperationName("op");

This is somewhat inconvenient for folks using Java 7; you could provide a SpanBuilder joinOr(T carrier, SpanBuilder fallback) overload which makes it easier to return a no-op version or any other prebuilt instance:

// On Java 7, easy way to use a no-op instance 
Span span = tracer.joinOr(carrier, NoopSpanBuilder.INSTANCE).withOperationName("op");

[yurishkuro]

Note: with #32 this is no longer an issue, imo. The join() method is replaced with extract(), which returns a SpanContext, and I think it's reasonable to return null of no context found in the incoming request. The starting of the span (the subject of this ticket) is not a responsibility of the extract() method.

[bhs]

Yes, this is (happily) a moot point in the post-SpanContext, post-extract() world. extract() should return null when no SpanContext could be found... the real problem before was that join() was doing two things: extracting and starting a span. With those tasks separated at the API level, there is no longer this ambiguity about what the Tracer should do.

We should close this once #32 is in.