Add setTag/withTag overloads that accept io.opentracing.tag types

[cwe1ss]

To use the fluent syntax with Span/SpanBuilder and the tag types, one currently has to write span.setTag(Tags.HTTP_STATUS.getKey(), 200). This has two issues:

• It loses the type restrictions, offered by the tag types. People could call e.g. span.setTag(Tags.HTTP_STATUS.getKey(), "foo")
• Having to add .getKey() is an unnecessary inconvenience.

Proposed solution

Add overloads on Span and SpanBuilder that accept Tag types so that people can write span.setTag(Tags.HTTP_STATUS, 200).

We've been discussing this in opentracing/opentracing-csharp#72 and compared a few options - I'll copy my latest comment for your convenience:

I created a similar perf test with BenchmarkDotNet.

I've been testing the following signatures with MockTracer (calling SetObjectTag()) and NoopTracer (doing a no-op):

ISpan SetTag_Int(string key, int value);

ISpan SetTag_AbstractGeneric<TTagValue>(AbstractTag<TTagValue> tag, TTagValue value);

ISpan SetTag_AbstractTyped(AbstractTag<int> tag, int value);

ISpan SetTag_IntTag(IntTag tag, int value);

Result:

BenchmarkDotNet=v0.10.13, OS=Windows 10 Redstone 3 [1709, Fall Creators Update] (10.0.16299.248)
Intel Core i5-4300U CPU 1.90GHz (Haswell), 1 CPU, 4 logical cores and 2 physical cores
Frequency=2435765 Hz, Resolution=410.5486 ns, Timer=TSC
.NET Core SDK=2.1.4
 [Host]     : .NET Core 2.0.5 (CoreCLR 4.6.26020.03, CoreFX 4.6.26018.01), 64bit RyuJIT
 DefaultJob : .NET Core 2.0.5 (CoreCLR 4.6.26020.03, CoreFX 4.6.26018.01), 64bit RyuJIT

Method	Mean	Error	StdDev
MockSpan_IntTag	77.927 ns	1.5573 ns	1.5992 ns
MockSpan_AbstractTag_Generic	90.484 ns	1.5414 ns	1.4418 ns
MockSpan_AbstractTag_Typed	77.102 ns	1.5754 ns	1.6178 ns
MockSpan_IntTag_Key	80.962 ns	1.5437 ns	1.4440 ns
MockSpan_Const	76.666 ns	1.5525 ns	1.5943 ns
MockSpan_Const_Overload	77.437 ns	1.5908 ns	1.4880 ns
NoopSpan_IntTag	4.040 ns	0.1024 ns	0.0958 ns
NoopSpan_AbstractTag_Generic	13.419 ns	0.2553 ns	0.2263 ns
NoopSpan_AbstractTag_Typed	4.625 ns	0.1388 ns	0.2573 ns
NoopSpan_IntTag_Key	5.052 ns	0.1431 ns	0.1757 ns
NoopSpan_Const	3.021 ns	0.0989 ns	0.0925 ns
NoopSpan_Const_Overload	3.068 ns	0.0985 ns	0.1054 ns

Using one SetTag<T>(AbstractTag<T> tag, T value) method is considerably slower so I would rule that out. It's a little bit more future-proof but it doesn't properly support the existing IntOrStringTag and adding new types is a breaking change anyway, as we also have to add a regular SetTag(string key, NewType value).

It's surprising to me that using AbstractTag<int> is sometimes faster than using IntTag directly (I ran it a few times). I would still rule that out though as this method still doesn't properly support IntOrStringTag.

I would therefore prefer using the actual Tag-types directly! This would ensure, all existing Tag types are supported and it also looks better in IntelliSense IMO. Adding a new Tag type is a breaking change but as I've said before, this is breaking anyway.

To sum it up, I would prefer the following additions to our interfaces:

// ISpan
ISpan SetTag(BooleanTag tag, bool value);
ISpan SetTag(IntOrStringTag tag, string value);
ISpan SetTag(IntTag tag, int value);
ISpan SetTag(StringTag tag, string value);

// ISpanBuilder
ISpanBuilder WithTag(BooleanTag tag, bool value);
ISpanBuilder WithTag(IntOrStringTag tag, string value);
ISpanBuilder WithTag(IntTag tag, int value);
ISpanBuilder WithTag(StringTag tag, string value);

---

Java might have different performance characteristics of course!

Adding overloads should be non-breaking for instrumentation code but it obviously is a breaking change for tracers.

Note that we could add this in a non-breaking way in C# by making them "extension methods" (see comment) but this gives tracers less flexibility and it would make the NoopTracer a little less efficient (because the extension method will still be executed and the no-op will happen later). Not sure if there's something similar in Java?

[carlosalberto]

Marking it as an enhancement for the time being ;)

[tylerbenson]

@tedsuo @carlosalberto can we consider getting this included into 0.32.0?