Suggestion re: span start/end option naming

[iredelmeier]

Thoughts on renaming:

SpanOption -> StartSpanOption to disambiguate from EndOption
SpanOptions -> SpanConfig (or StartSpanConfig depending on the above) and EndOptions -> EndConfig to make them more greppable, expand the number of nouns we can use (e.g., so that opts/options could be used for a slice of Options and c/cfg/config for the config version)

[freeformz]

is there a reason documented for these Option structs? I'm not a fan, but maybe there is a reason for it?

[iredelmeier]

@freeformz do you mean the SpanOptions and EndOptions structs? If so, here is the relevant part of the spec.

One frequent-ish example comes up with start and end timestamp configuration, which can be quite useful for things like interop with other tracing tools (including OpenTracing and OpenCensus), testing, etc.

[jmacd]

Do you dislike the options being a struct as opposed to some other style?

[freeformz]

Go has a means of passing arguments. If a value is required, then declare it on the constructor. For everything optional there are few ways to handle them, but I prefer the functional option pattern.

That method leaves us with a single type with one or more constructors and likely a few functions.

The patterns I've seen here leaves us with at least 3 types, in addition to one or more constructors and a few functions.

For me, it's kind of crazy making re-implementing parameters over and over and over again. Not to mention having to use them regularly.

This is of course my opinion (it's a variant of one of the anti-patterns I pointed out 2 years go) and it's one of several things that frustrates me when diving into OT-Go's code base, so maybe take this Q (and my critique) with a grain of salt.

[jmacd]

Thank you @freeformz. I like the functional options pattern, but I wouldn't say I always prefer it. There's a point about the appearance of at least one additional memory allocation when passing a variable-length argument list (maybe? maybe not? can the Go compiler can optimize the pattern?). I'm am willing to follow your guidance -- this appears to be a style-vs-performance question.

[freeformz]

I don't have numbers and I'm not sure it's easy to get them, but my feeling is that having to declare a struct of options, even for those that don't need to be set, likely allocates more in the common case (the common case being where there are no or few additional "options"). With that said some (or all of those allocation are likely on the stack).

FWIW: If we care that much about counting allocations we should be writing benchmarks to prove / disprove / have some numbers to care about. Also interfaces, cough cough cough (But I won't rant about that again atm).

[freeformz]

"not sure it's easy to get them" == We would have to write a portion of OT-GO both ways and run the same benchmarks against each implementation.

[jmacd]

Passing a struct definitely happens on the stack, which is effectively zero cost, whereas unless the Go compiler is able to (I'm not sure it is), the functional option pattern requires one allocation for the slice of options and one allocation for each functional closure.

For example: https://gist.github.com/jmacd/5b9b64711030c1cb88a7521df840f240

There are two options being passed, so we're comparing 3 vs 0 memory allocations per call.

I'm not here to argue that we should always choose the higher-performance option. I tend to go with functional options for cases where APIs are used infrequently. There will be users who want to create spans at a high frequency, and those users may want to know that their observability SDK is not creating additional interference for the application it is observing. Those users won't need a benchmark to know that additional memory allocations are unwelcome.

[ferhatelmas]

@jmacd In this issue, we're only renaming, right?

Also, in #325, shouldn't SetAttributes take multiple pairs to have a better comparison. For example, in the benchmark, SetAttribute is called 4 times so SetAttributes should be called once with 4 pairs.

/cc @paivagustavo