Temporarily remove tracestate

[iredelmeier]

Per a discussion in the W3C Trace-Context meeting today, it feels safest to (temporarily!) remove tracestate from OpenTelemetry.

Its use in OpenTelemetry itself is currently ambiguous, e.g., should it be used to propagate tags and, if so, how do we want to encode them? Rushing this decision while there are still open, higher-level questions about distributed context in OpenTelemetry feels like a mistake, as it would likely lead to either a painful breaking change or being stuck with a bad decision. On the other hand, waiting until we've thought through it some more allows us to implement the decision as an additive decision.

Removing tracestate propagation could impact some vendors. We believe that this is unlikely, since most of the vendors actually using Trace-Context either were present for the meeting and confirmed they shouldn't be affected or have otherwise mentioned they're not yet using tracestate. Please speak up if you're aware of a vendor or other party that would be affected!

[tedsuo]

Just for clarity, is the request just remove TraceState from the API? Or are we also asking SDKs not to propagate it any more. Is TraceState in flux within the W3C proposal?

I agree that there is little obvious utility in exposing TraceState in the API, as its contents are by definition an implementation detail. There is a good case to make, which says that if developers are accessing TraceState to make decisions in their code, that is effectively the same as casting the tracer API to a specific implementation.

As with any case where users are feeling forced to do a cast, we would first want to understand the requirements and then expose an API in a vendor-neutral manner.

[mtwo]

I think that it's a discussion for (a) removing TraceState from the API and (b) determining if Tags should use TraceState (while still not exposing the TraceState API directly). I don't have a strong opinion, but we should be clear to each language's SIG about how they're expected to handle the TraceContext header.

[bogdandrutu]

I think the TraceState is independent of tags (at least based on my understanding). Happy for the moment to remove it, fyi this change is not compatible with OpenCensus, so we may need to double check that.

[yurishkuro]

tags, aka baggage, are user-controlled metadata. tracestate is vendor/implementation-controlled. Baggage does not go to tracestate, it would go to correlation-context.

[bogdandrutu]

@yurishkuro that is exactly my understanding as well.

[songy23]

Please rebase and resolve the merge conflicts.

[iredelmeier]

@songy23 done!

[bogdandrutu]

I am not sure I understand the motivation to remove it (at least the one in the description). Tags/DistributedContext are as @yurishkuro mentioned independent of this so I do not understand why this PR should be merged.

[bogdandrutu]

Also this will break OpenCensus so probably we cannot remove it. There is no other way that existing OpenCensus Tracestate can be replaced.

[bogdandrutu]

/cc @AloisReitbauer can you comment from Dynatrace perspective?

[songy23]

Per a discussion in the W3C Trace-Context meeting today, it feels safest to (temporarily!) remove tracestate from OpenTelemetry.

I would suggest to put this PR on hold until that actually happened. Agree we have to be W3C and OpenCensus compatible.

[yurishkuro]

as I understand the PR, it's not about removing tracestate from w3c-compliant encoders, but from the Spec and API.

[discostu105]

If I understand correctly, this is about removing TraceState from the API layer only. In that case I am absolutely for it. I don't think an application developer should read/write TraceState at all. It's a concern of the tracing library only.

Would the default OpenTelemetry implementation (SDK) still implement TraceState propagation?

In case of a custom tracer API implementation (OpenTracing style), the Tracer is free to choose if it wants to implement TraceContext (including TraceState) or not.

[reyang]

If this PR is about removing the TraceState related APIs, but still keeping the propagation part (in order to be compliant with W3C TraceContext), it is okay at this moment.

[bogdandrutu]

@reyang I agree, but that is not what I understand from the description and from the changes. I think we need to be w3c compatible and not propagating the tracestate is against the standard.

I do think that tracestate API can be removed until we learn better how to expose mutators or how do the users will use it, but we cannot drop it if we received. We may be able to drop the serialization/deserialization and validation but we probably still need to propagate what we received.

Happy to be convinced otherwise.

@discostu105 one option to allow users to extend the SpanContext is to not make it final so that a different SDK implementation can choose to extend that class and include the tracestate.

Question for all:

1. When we do want to do log correlation do we want to also include the tracestate in the logs?

[lizthegrey]

@bogdandrutu and I discussed today that tracestate needs to be used by the SDK to pass the sampling rate between spans... and therefore that while it's fine to remove for September 1, we will need it by October 15 in order to ensure metrics calculated from rate of observed post-sampling spans are correct.

We don't need the full tracestate APIs to be exposed to users, but we do need them to be propagated, and we do need the API to internally to the SDK be able to set one tracestate field for the sample rate.

[tedsuo]

@lizthegrey I think that's the correct assessment. TraceState is propagated, and available to the SDK, but not readable via the API for now.

@iredelmeier, does that match your original intention?

[lizthegrey]

@lizthegrey I think that's the correct assessment. TraceState is propagated, and available to the SDK, but not readable via the API for now.

@iredelmeier, does that match your original intention?

ping @iredelmeier -- this is blocking me from being able to extend open-telemetry/oteps#24 to include information on sample rates in the tracestate as part of the default SDK Sampler implementations and the Sampler SDK interface.

[SergeyKanzhelev]

The discussion of this issue ended month ago. And the branch is stale and have a lot of conflicts.

@iredelmeier I am closing this PR for now. Please re-open if needed