new SamplingResult spec makes it more expensive/difficult for Samplers that don't want to modify TraceState

[jkwatson]

What are you trying to achieve?

Recently merged PR #988 added a field to the SamplingResult API for the modified TraceState. Most samplers (and, indeed, all the built-in samplers) do not modify the TraceState, and hence could previously return one of several constant SamplingResults in almost all cases. Now, every response needs to include the passed in parent TraceState if it doesn't want to modify it. I would like to make it easy to do the most common thing, but possible to do the more complex thing (i.e. modifying the TraceState).

I propose that the SamplingResult be extended to include a boolean field, defaulting to false that signals to the SDK whether a new, updated TraceState is to be used for the Span. So, by default, Samplers will not modify the TraceState.

[Oberon00]

I propose that the SamplingResult be extended to include a boolean field, defaulting to false

While that may be a possible implementation, I think the spec should be written in a way that avoids going into so much detail. E.g. some languages may prefer something like

type TraceStateUpdate = NewTraceState(TraceState) | KeepParent

as the type for the tracestate in the result instead of returning one. Or in C# you might pass the tracestate as an ref parameter so it can be reassigned but if it is not reassigned it implicitly stays the same.

Agree with the gist of the issue though that it should be made hard to accidentally clear the tracestate and easy to use the parent unmodified.

[arminru]

cc @Kanwaldeep @paulosman

[jkwatson]

I am very much in favor of the spec being changed to specify the capability of allowing Samplers to modify TraceState, and leave the precise technical details up to the various language implementations.

[andrewhsu]

from issue triage mtg today, setting as p3 since small backwards-compat change if it comes in before ga since it is just adding a field