Opentelemetry-go: Remove Span.SetAttribute

[rakyll]

Span.SetAttributes(...core.KeyValue) already does this if you pass a single argument. In Go, we don't provide too many options for the same functionality and variadic args are used exactly to avoid necessity to provide two methods.

[jmacd]

In OpenTelemetry, we have stated a desire to avoid performance penalties especially when there is a no-op implementation in use. SetAttributes(singleton) requires a memory allocation, whereas SetAttribute(singleton) does not. We believe that instrumentation libraries are special, from this point of view, and that it is worthwhile to provide options that avoid memory allocation where it is convenient.

The specification language that justifies this decision is "It is also important that minimal implementation incurs as little performance penalty as possible".

[paivagustavo]

I've made a simple benchmark comparing using setAttributes(singleton) vs setAttribute(singleton) when one is using the SDK to start and end a span (can be seen at #325).

Basically, It sets 4 attributes (one by one) using each function. Here is the benchmark running on my machine:

pkg: go.opentelemetry.io/otel/sdk/trace
BenchmarkSpan_SetAttributes/SetAttribute/AlwaysSample-6               434814          4179 ns/op        1552 B/op         29 allocs/op
BenchmarkSpan_SetAttributes/SetAttribute/NeverSample-6                958104          1114 ns/op         336 B/op          4 allocs/op
BenchmarkSpan_SetAttributes/SetAttributes/AlwaysSample-6              260006          5051 ns/op        1744 B/op         33 allocs/op
BenchmarkSpan_SetAttributes/SetAttributes/NeverSample-6              1000000          1478 ns/op         528 B/op          8 allocs/op

Each call adds an additional alloc and approximately 75~225ns.

IMO, that is a small penalty that I think is worth taking off to have a clear and smaller API. I'm in favor of removing this singular sets all over the API, as @rakyll and @freeformz has already spoken on this and other issues. Although, I'm okay the SDK using this kinda of optimization in its internals.

ps: After we have a decision here, we should probably extend this for the rest of the API. For example, the Trace API have SetAttribute(core.KeyValue) and SetAttributes(...core.KeyValue) methods but only have AddLink(Link) method, which is not consistent.

[jmacd]

100-200ns is what you see in a microbenchmark.

What you'll see in real applications is additional garbage collection cost, which has a non-local, non-linear impact. I return to "It is also important that minimal implementation incurs as little performance penalty as possible". Even a no-op implementation bears this cost.

I agree that AddLink doesn't have a corresponding plural version, but I also suspect a call to support adding multiple links will have ~0 users. Adding a single attribute should be a common operation.

[paivagustavo]

Agreed that the impact is higher than what the benchmark shows and I understand your position with the library guidelines.

But there is not doubt that this "duplicated" method pollutes the Span interface and the user may be confused why and when to use each variation, which is a common standard for Go. Also, there is nothing preventing the user to use the SetAttributes when calling with a single attribute or the opposite.

At the very minimum we should better document these methods and their tradeoffs.

About the AddLink, the specs doesn't define a method AddLink for spans, only for its creation, I've filled an issue (#329) for removing it.

[jmacd]

I will not object any further to removing SetAttribute. If there is a serious performance complaint, perhaps it can be re-introduced.

(For me, somewhat of a grammar nerd, using a plural "attributes" when there is only one is a bit off-putting. That's a separate matter.)

[paivagustavo]

I think we both made our points about this, would be great to have input from others as well and get to a consensus.

[iredelmeier]

IIRC we reached consensus about removing this in last Thursday's SIG meeting.

To @jmacd's point, we can always add the singular version back in later!

[0sc]

Is this up for grabs? I'll like to try my hands at it