Current getUpdatedTraceState may cause unnecessary allocations if SDK will also change the TraceState

[bogdandrutu]

The current getUpdatedTraceState API is inefficient if in the future the SDK will also have to modify the TraceState.

Current:

    TraceState samplingResultTraceState =
        samplingResult.updateTraceState(parentSpanContext.getTraceState());

Assume SDK will add one entry to the state:

    TraceState samplingResultTraceState =
        samplingResult.updateTraceState(parentSpanContext.getTraceState());
    TraceState  newTraceState samplingResultTraceState.toBuilder().set("foo", "bar").build()

In this case it is clear that we create a new object (in case sampler changes state) from Sampler, then 2 more objects from the SDK calls.

A possibility is to change SamplingResult to return a TraceStateUpdater(BiConsumer<TraceState (parent), TraceStateBuilder(current builder)>) and return null when no updater is available from the Sampler.

There may be other options to solve this issue, but I want to make sure we fix this before 1.0;

Also if this request is ignore at least rename getUpdatedTraceState to updateTraceState

[jkwatson]

TBH, I wish we had never even added this requirement to the specification. I'm unclear on what the use-case is for a sampler updating the TraceState, and it seems like something that could have been done after 1.0.

Do you have a real-world use-case for the SDK itself updating the TraceState? If not, I'd rather leave this as-is for now, and add another option to cover that use-case when it's clear what it actually is.

I'm not opposed to s/getUpdatedTraceState/updateTraceState/, but I thought that @Oberon00 had an opinion on that originally [i might be mis-remembering]

[bogdandrutu]

The use-case for sampler to update the TraceState is as simple as in case of a probability sampler they want to add that to the TraceState to be visible to the downstream spans. Microsoft and Honeycomb had a good example on how they used that.

I don't have a good example for SDK unless we decide to add by default the sample rate/probability to the TraceState... Maybe will not become a requirement but I usually like to design APIs in a way they are extendible and not affect performance.

I would like to hear the reasons for the name choice...

[Oberon00]

A possibility is to change SamplingResult to return a TraceStateUpdater(BiConsumer<TraceState (parent), TraceStateBuilder(current builder)>) and return null when no updater is available from the Sampler.

I don't understand the improvement here. If getUpdatedTraceState (no opinion on the name) does not need to modify, it can return the original TraceState object, right?

@jkwatson

TBH, I wish we had never even added this requirement to the specification. I'm unclear on what the use-case is for a sampler updating the TraceState, and it seems like something that could have been done after 1.0.

Samplers may want to coordinate their sampling beyond the W3C sampled flag across different processes. There is an OTEP PR for a particular use case: open-telemetry/oteps#135. The W3C specification even calls out tracestate explicitly in the context of the sampled flag, noting that vendors may want to store additional sampling info there https://www.w3.org/TR/trace-context/#sampled-flag

[Oberon00]

For the name: It seems I might have coined it in #1707 (comment), but I'm not particularly attached to it. I think I just wanted to emphasize that this does not update the object in-place (I don't think we use an errorprone annotation like @CheckReturnValue yet but we could).

[anuraaga]

Name makes sense to me too since it's not mutating.

[jkwatson]

My opinion: I think we should keep this as-is. Any changes we would make would be just as speculative as the current implementation and just as likely to end up with issues (as this one might be, in an unknown possible future). I'd rather address this API and build something better with a concrete use-case in mind, based on user feedback, rather than making tweaks based on speculation right now.

[anuraaga]

We think this is fine given the smallish use case and we'll get more feedback in real world usage and can think again later.