Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add TraceState #57
Add TraceState #57
Changes from all commits
50175e5
59abd67
88f47cf
c681e66
17a3c85
c321a59
544f3cf
7342a34
58619d1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where will this be stored? It also looks like this conflicts with
DistributedContext
as this is the equivalent of one of the class described in open-telemetry/opentelemetry-specification#140?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had talked today in the SIG about doing some kind of encoding/decoding of values. E.g. using URL encoding for the unsupported characters. Is there any kind of encoding format for this specified in the spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't in the w3c spec nor the opentelem spec. I think as an instrumentation library we should only attempt to validate the user's input and provide a stern warning (but not exception) that the input is invalid. I wouldn't want to automatically encode the input when it goes over the wire in case they aren't using an opentelem library downstream. I feel like if the user wants to include non spec conforming data and have it accessible downstream, we can recommend using URL encoding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pushing unnecessary complexity on the user. I understand if W3C don't want to enforce a specific format, but I think OpenTelemetry should standardize this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on OpenTelem standardizing on it. For what it's worth I'm not super stoked on using tracestate to propagate baggage to begin with, definitely interested in seeing how the new w3c correlation context turns out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent of
tracestate
is to encode tracing system specific information and it's up to the tracing system how this is encoded. Some vendors use base64 to send some json, some simply have information like a tenant id in plain text.At some point Opentelemetry needs to come up with what should be put in there and was there is quite some overlap between the w3c group and this project it will definitely be compliant.
The intent is NOT to make it available to the user to use it for baggage. As @bg451 said, this will be up to another spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With your above explanation that each vendor should have a single entry, it makes sense to not require a specific format. However, if there is a entry reserved for OpenTelemetry itself, then we should define the format of that entry.