SpanBuilder vs StartSpanOptions vs StartSpan-parameters

[cwe1ss]

As of #2, I'd like to discuss how we should pass parameters (tags, references, timestamp) to ITracer.StartSpan. There are multiple options for this:

1 - Optional parameters

/* ITracer interface */

ISpan StartSpan(
  string operationName, /* only this is required, the rest is optional */
  DateTimeOffset? startTimestamp = null,
  IEnumerable<SpanReference> references = null,
  IDictionary<string, object> tags = null
);

/* Second method for single reference (because this is the most-used case) */
// this allows for easier consumer code and for tracer perf optimizations
// (in case they want to store single references without a list)
ISpan StartSpan(
  string operationName,
  DateTimeOffset? startTimestamp = null,
  SpanReference reference = null,
  IDictionary<string, object> tags = null
);

/* usage */

tracer.StartSpan("get_account"); // no optional parameters

tracer.StartSpan("get_account", startTimestamp: DateTime.UtcNow); // custom timestamp

// just one reference - using the second method
tracer.StartSpan("get_account", reference: SpanReference.ChildOf(someSpanContext)); 

// tags from a variable and reference list
var myTags = new Dictionary<string, object> { { "key", "value" }, { "key2", "value2"} };
tracer.StartSpan("get_account",
  references: new List<SpanReference> { ref1, ref2 },
  tags: myTags
);

Advantages:

• It doesn't create unnecessary objects (StartSpanOptions, SpanBuilder)
• Probably the best performance (because it requires less garbage collection)
• Extending the interface with additional optional parameters
  • doesn't break consumers
  • breaks trace implementations with a compile-time error (which is a good thing IMO because they now know that they have to implement it)

Disadvantages:

• Adding tags always requires creating the dictionary instance
• It's not possible to add typed extensions for known tags/references
• the method signature might get long if we add more parameters in the future

---

2 - StartSpanOptions

/* ITracer interface */
ISpan StartSpan(string operationName, StartSpanOptions options);

/* StartSpanOptions class */
class StartSpanOptions {
  IList<SpanReference> References { get; set; } = new List<SpanReference>();
  IDictionary<string, object> Tags { get; set; } = new Dictionary<string, object>();
  DateTimeOffset? StartTimestamp { get; set; }

  // helper methods for nicer API
  StartSpanOptions AddReference(SpanReference reference);
  StartSpanOptions AsChildOf(ISpanContext parent); // calls AddReference
  StartSpanOptions FollowsFrom(ISpanContext parent); // calls AddReference
  StartSpanOptions SetTag(string key, string value);
  StartSpanOptions SetStartTimestamp(DateTimeOffset startTimestamp);
}

/* usage */

tracer.StartSpan("get_account"); // no options

// inline without helper methods [1]
tracer.StartSpan("get_account", new StartSpanOptions {
  StartTimestamp = DateTime.UtcNow,
  Tags = new Dictionary<string, object> { { "key", "value" }, { "key2", "value2" } }
});

// using helper methods
tracer.StartSpan("get_account", new StartSpanOptions()
  .SetStartTimestamp(DateTime.UtcNow)
  .SetTag("key", "value")
  .AddReference(SpanReference.ChildOf(otherSpanContext))
);

// separate object
var options = new StartSpanOptions { StartTimestamp = DateTime.UtcNow };
options.SetTag("key", "value");
tracer.StartSpan("get_account", options);

Advantages:

• Slightly more fluent API
• Does not create additional objects if there are no options
• Extending the interface with additional optional parameters doesn't break consumers

Disadvantages:

• Creates unnecessary objects if tags/references are not used/overwritten.
  • StartSpanOptions should be initialized with empty lists, dictionaries otherwise users might run into NullReferenceExceptions when using Tags, References directly.
  • To improve this, the variables could be made private and null-initialized. The only way to add references/tags would be through methods which initialize the variable if it is null.
  • However, this way, setting tags like in example [1] is no longer possible
• It's not as fluent as a builder pattern because you manually have to create the StartSpanOptions instance
• Extending the StartSpanOptions class with additional parameters doesn't break trace implementations
  • If trace implementations update their dependencies, they don't get any compile-time notice about the new parameters
  • (This might also be seen as an advantage depending on your point of view)

---

3 - SpanBuilder

/* ITracer interface */
ISpanBuilder BuildSpan(string operationName);

/* ISpanBuilder interface */
interface ISpanBuilder {
  ISpanBuilder AsChildOf(ISpanContext parent);
  ISpanBuilder AddReference(SpanReference reference);
  ISpanBuilder WithTag(string key, object value);
  ISpanBuilder WithStartTimestamp(DateTimeOffset startTimestamp);
  ISpan Start();
}

/* usage */

tracer.BuildSpan("get_account").Start(); // no options

// using helper methods
tracer.BuildSpan("get_account")
  .WithStartTimestamp(DateTime.UtcNow)
  .WithTag("key", "value")
  .AddReference(SpanReference.ChildOf(otherSpanContext))
  .Start();

Advantages:

• Very fluent API
• Extending the interface with additional optional parameters
  • doesn't break consumers
  • breaks trace implementations with a compile-time error (which is a good thing IMO because they now know that they have to implement it)

Disadvantages:

• User must call Start() even if she doesn't need options
• Every trace implementations must decide how to store all variables in the builder implementation and how to pass them to the Span constructor. They might end up with something very similar to the first two options.
• Always requires additional object instances, even if there are no options required
  • At least the builder-instance itself and the operationName string (which will be passed to the Span)

---

Language comparison:

• Java: "3 - SpanBuilder"
• Go: "2 - StartSpanOptions" (Although the usage looks more similar to "1 - Optional Parameters")
• Objective C: "1 - Optional Parameters" but with many overloads for different kinds since they don't have optional parameters)
• JavaScript: "2 - StartSpanOptions"
• Python: "1 - Optional Parameters"
• C++: "2 - StartSpanOptions"

[dawallin]

Good overview of all patterns.
I think there are to many parameters for "1 - optional parameters" and therefor like either "2 - StartSpanOptions" or "3 - SpanBuilder" best.
I think StartSpanOptions with Tracer.StartSpan(options) is conceptually easier and makes the easiest API to grasp if you new to the API, it is direct and clear.
With that said, once you understand the API, the fluent feeling of SpanBuilder makes it very attractive, but I really dislike that every implementation has make the same collecting of options.
I vote to choose the StartSpanOption pattern. As shown in #2 it is then possible for a util or extension project to add a fluent builder as extensionmethods to the Tracer.

[bhs]

@cwe1ss thanks for the overview / research!

@dawallin a meta-note: when we end up with more than a handful of C# Tracer implementations, it's totally fair game to create helper packages/artifacts that do menial/trivial work (like implementing a Builder). For that reason, I would reemphasize my opinion that API decisions here should be primarily motivated by what's easiest and clearest for the caller.

For C#, I slightly prefer the Builder version since it makes it possible for a noop Tracer implementation to really be a true noop. (As opposed to the StartSpanOptions, where we are going to end up implementing tag storage/etc within OT proper) I also think it makes the common-case "create a child span" syntax a little cleaner.

[cwe1ss]

If we go with a Builder, I guess using an interface like in Java would be better. This would allow trace implementations to optimize/do whatever they want without us having to release new versions. They could also decide which parameters should be passed to the Span constructor and which can be passed afterwards.
It keeps the focus of this library at an "abstractions only" level. It should contain as little implementation code as possible.

My main concern with the builder pattern is that it requires more allocations. Is this something we should take into account?