Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert tracestate to an unstructured raw string field #57

Merged

Conversation

tigrannajaryan
Copy link
Member

The Span.tracestate field was a structured data that stores fully decoded Tracestate.
However, there is no good use case for keeping this in structured form, there are no
known applications that work with this elements of this data, other than just passing
it around fully.

This change eliminates Tracestate message type and makse Span.tracestate a simple string
field in w3c-trace-context format.

Resolves #55

@Oberon00
Copy link
Member

Oberon00 commented Nov 15, 2019

I'm indifferent to this change itself, but if you are implying that the tracestate should be stored as a string in-memory in each language's SpanContext type, I have to object. The tracestate is the only place left for custom propagators/formats to store any information that is to be propagated (or even used by custom exporters, samplers, etc) and not already covered by the W3C fields. Also, since open-telemetry/oteps#58 was deferred, even custom API implementations have to rely on tracestate to store any custom information to be propagated in the meantime.

@tigrannajaryan
Copy link
Member Author

I'm indifferent to this change itself, but if you are implying that the tracestate should be stored as a string in-memory in each language's SpanContext type, I have to object.

This is protocol repo and discussion is about protocol only, it is not about SpanContext in client libraries, so I am not sure I understand the objection.

The purpose of this field in the protocol as of now is only to carry tracestate with each exported Span which happens via gRPC (we cannot rely on HTTP w3c-trace-context headers since one export request may contain many Spans each having a different tracestate).

@Oberon00
Copy link
Member

OK, that's what I thought, I was just making sure 😃

Actually, I wonder if the tracestate should exported at all via OTLP.

@tigrannajaryan
Copy link
Member Author

Actually, I wonder if the tracestate should exported at all via OTLP.

Yes, I believe it should otherwise we will lose this information when passing through intermediary nodes (Collector).

The Span.tracestate field was a structured data that stores fully decoded Tracestate.
However, there is no good use case for keeping this in structured form, there are no
known applications that work with this elements of this data, other than just passing
it around fully.

This change eliminates Tracestate message type and makse Span.tracestate a simple string
field in w3c-trace-context format.

Resolves open-telemetry#55
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/tracestate branch from 8e23a80 to 9f69db3 Compare November 15, 2019 17:52
@tigrannajaryan
Copy link
Member Author

@yurishkuro @bogdandrutu please have a look.

@SergeyKanzhelev
Copy link
Member

I'm OK with this change. We do the same in .NET: https://github.com/dotnet/corefx/blob/1518fdc5cc94571bf3ce1bb95e1a60f789a67f18/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L529-L558

One problem with this and this is what we discussed in .NET is the requirement to get ASCII bytes and inflate them into the unicode string. Many languages will do this "inflation" without giving any other options to access headers. But some - and especially binary protocols - will require some explicit CPU cycles.

Another consideration - it may be nice to have both - string that is just for propagation and additional name/value pairs as a separate field. So it will be easier to add properties without the need to serialize them into the string and faster access to these properties in collector.

@SergeyKanzhelev SergeyKanzhelev merged commit 64858ef into open-telemetry:master Nov 19, 2019
@tigrannajaryan tigrannajaryan deleted the feature/tigran/tracestate branch November 20, 2019 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: convert Tracestate to an unstructured raw string field in Span
5 participants