SpanBuilder should initialize the start time when start() is called

[stealthcode]

I'm creating SpanBuilder instances and initializing them with a parent span as well as basic tags. These builders are then used for all child span initialization. However when a second span is created using the same builder the start time of the 2nd span == start time of the 1st. This is unexpected. When you call .start() without calling .withStartTime(micros) then the start time is initialized to the relative nowish value. So I expected that every time .start() is called it generates a new span with tag data and parent span initialized.

I'm also unclear whether the starttime of a span originates from System.nanoTime() or from System.currentTimeMillis() * 1_000L.

[yurishkuro]

I don't think SpanBuilder is meant to be reusable.

[stealthcode]

@yurishkuro In general I don't think a builder pattern should be one-shot single purpose. That defeats their usefulness as encapsulations of context and reuse of code in the generic builder's .build() method.

When creating a child span I can think of two primary difference between using the tracer client to recreate a builder for every child span and reusing a builder instance.

1. Making a class that handles service calls (and retry logic) will need to link it's internal children spans to the parent span and possibly accept some very specific context for the sole purpose of logging in the child span's tags. It is a much cleaner simpler module boundary to simply inject an instantiated builder which contains this context.
2. Creating an instance of a SpanBuilder is an unnecessary allocation that would have to be performed every time a child span is created.

[yurishkuro]

Sorry, I do not follow your first point. Could you elaborate with a code example?

On the second point - yes, it is an extra allocation. However, that is a fairly standard pattern in Java for creating objects with optional data. The purpose of the builder is exactly that - to allow passing various optional parameters to the main startSpan method. In other OT languages there are no builders, e.g. JS and Python have an idiomatic way of passing optional parameters to functions, and in Go we use a functional option pattern. The latter is possible with Java, although I am not sure if it would still cause an allocation for the vararg array.

So my point is that, even it may not be explicitly stated in the Java OT API, but the builder was intended to be single use. In my experience it is the most common way of using the builder objects, given that they are mutable and raise all kinds of threading concerns if you try to reuse them.

[stealthcode]

I'm claiming that the API forces you to have more coupling where there are cases for low coupling. In refactoring to extract or for instance adding retries in a modular manner winds up with a lot of tracing related data coupling.

To clarify my point above...

1. Making a class that handles service calls (and retry logic) will need to link it's internal children spans to the parent span and possibly accept some very specific context for the sole purpose of logging in the child span's tags. It is a much cleaner simpler module boundary to simply inject an instantiated builder which contains this context.

Here's an example where I write two classes, one top level method that delegates to a child method and the spans they generate must be attached as a child span. This first example uses the SpanBuilder assuming the implicit single-use behavior.

public class UtilityWrapper {
    Tracer tracer = /*...*/;
    public ByteBuffer get() {
        Span outerSpan = tracer.buildSpan(OUTER_SPAN_LABEL).start();
        // prepare payload
        ByteBuffer results = new Service(tracer).get(payload, outerSpan, tag1, tag2);
        outerSpan.finish(); // simplified closing span for simplicity
        return results;
    }
}

public class Service {
    Tracer tracer;
    Service(Tracer tracer) {
        this.tracer = tracer;
    }

    public ByteBuffer get(Object payload, Span outerSpan, String tag1, String tag2) {
        int attemptCount = 0;
        while (++attemptCount <= MAX_ATTEMPTS) {
            Span innerSpan = null;
            try {
                innerSpan = tracer.buildSpan(OUTER_SPAN_LABEL)
                    .asChildOf(outerSpan)
                    .withTag(KEY1, tag1)
                    .withTag(KEY2, tag2)
                    .start();
                // do volatile work that may fail
                innerSpan.finish();
            } 
            catch(Throwable t) { // log and allow to loop 
                innerSpan.setTag(EXCEPTION_TAG, true);
            }
        }
    }
}

The next implementation assumes that the SpanBuilders can be reused and .start() assigns a new startTimestamp.

public class UtilityWrapper {
    Tracer tracer = /*...*/;
    public ByteBuffer get() {
        Span outerSpan = tracer.buildSpan(OUTER_SPAN_LABEL).start();
        // prepare payload
        Span innerSpanBuilder = tracer.buildSpan(INNER_SPAN_LABEL)
            .asChildOf(outerSpan)
            .withTag(KEY1, tag1)
            .withTag(KEY2, tag2);
        ByteBuffer results = new Service(tracer).get(payload, innerSpanBuilder);
        outerSpan.finish(); // simplified closing span for simplicity
        return results;
    }
}

public class Service {
    public ByteBuffer get(Object payload, Tracer.SpanBuilder innerSpanBuilder) {
        int attemptCount = 0;
        while (++attemptCount <= MAX_ATTEMPTS) {
            Span innerSpan = null;
            try {
                innerSpan = innerSpanBuilder.start();
                // do volatile work that may fail
                innerSpan.finish();
            } 
            catch(Throwable t) { // log and allow to loop 
                innerSpan.setTag(EXCEPTION_TAG, true);
            }
        }
    }
}

The surface area between the UtilityWrapper and Service has been reduced by eliminating a constructor arg and reducing the method arguments necessary. This makes Service more easily testable (towards the single purpose principle).

Note that both use cases are valid but the latter case is not possible right now. In theory if I had access to the underlying way that .start() generates the timestamp then I could use the method SpanBuilder.withStartTimestamp(microSeconds). Right now it seems like that method exists but is unusable since there is no external facing API for getting the timestamp in a way consistent with just calling .start() on a new builder.

[stealthcode]

raise all kinds of threading concerns if you try to reuse them.

I understand this. Are you primarily concerned with concurrent modification of the SpanContext or the method to generate the now-timestamp? If the .start() method generates unique instances for each span then each Span would have its own copy of it's data?

[yurishkuro]

Are you primarily concerned with concurrent modification of the SpanContext or the method to generate the now-timestamp?

I was referring to the builder itself being mutable and thus not thread safe. For example, what it means to call .asChildOf() more than once on the builder?

I see your point about reusing the builder, but arguably the same can be achieved by passing a () => Span lambda to Service.get().

If we require the builder to be reusable, it's a semantic change to the existing OT Java API that will break existing implementations, so must be done with care.

@bensigelman @michaelsembwever your thoughts?

[stealthcode]

For example, what it means to call .asChildOf() more than once on the builder?

Understandable. Concurrent assignment with a read or another write would just be last-one-wins even without a synchronized block, however changing multiple fields on the builder concurrently could wind up with mixed results.

One possible compromise is if the SpanBuilder had a method that returned an immutable ImmutableSpanBuilder which holds everything but the start timestamp in final fields then when .start() is called then it generates a timestamp and returns a Span.

arguably the same can be achieved by passing a () => Span lambda

Yes I agree this reduces the coupling and makes the Service more testable (i.e. not injecting the Tracer), but this is somewhat opaque as a Callable<Span>. And this still allocates a new object. I'm recommending that this be first class in OT so that there is clear support for what I think is a common Java use case.

[bhs]

IMO the start timestamp is "special" in that it's the only field the SpanBuilder presently controls which has a dynamic / constantly-changing value. So I could see a case for making SpanBuilder immutable but overloading start() with start(timestamp) (and remove the withStartTimestamp() method entirely, or at least deprecate it).

[gkumar7]

Similar issue here: #69

Starting at the creation of a SpanBuilder seems semantically incorrect as duration should by default capture Span lifetime and not include build time.

[bhs]

That's an interesting point, @gkumar7... the "builder" in this case is really just a way to support many different combinations of params to start() without actually creating a combinatorial expansion of all possibilities as overloaded methods. Maybe it would feel better if it was called SpanStarter instead of SpanBuilder but had all of the same methods? That's essentially what it is. YMMV?

[yurishkuro]

That's precisely what I was saying above.

SpanStarter: +1

[gkumar7]

SpanStarter +1, good idea.

I like how Jaeger defines startTimeMicroseconds here: https://github.com/uber/jaeger-client-java/blob/master/jaeger-core/src/main/java/com/uber/jaeger/Tracer.java#L287-L293

instead of defining it when SpanBuilder is created, which is basically what I was alluding to above.

[cwe1ss]

-1 for SpanStarter. tracer.buildSpan is a fluent interface which means that its function should be obvious even if people don't know the class name. So I don't think that changing the name helps with the general issue. Also, changing it would be a huuuge breaking change.

+1 for starting the clock on the call to "start".

[michaelsembwever]

@bensigelman @michaelsembwever your thoughts?

spanBuilder.start() should not be called multiple times.
Pre-conditions should be put into the start() and other build methods to ensure such methods are not called multiple times.

The concept of a re-usable builder does provide useful in places, as @stealthcode demonstrates but can be solved by adding a copy-constructor to the SpanBuilder.

-1 for SpanStarter.
+1 for pre-conditions preventing api mis-use.
+0 for copy-constructor.

[bhs]

While I believe that SpanStarter is probably more "correct" in some pedantic way, the fluent API point that @cwe1ss makes alongside the fact that "it's-a-builder-pattern-goddamit" makes me inclined to just leave the name where it is.

I like @michaelsembwever's copy-constructor idea (or a clone() method, or similar).