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

TraceState/Baggage fields currently should only accept string-string pairs #1539

Closed
MrAlias opened this issue Feb 16, 2021 · 5 comments · Fixed by #1967
Closed

TraceState/Baggage fields currently should only accept string-string pairs #1539

MrAlias opened this issue Feb 16, 2021 · 5 comments · Fixed by #1967
Assignees
Labels
area:trace Part of OpenTelemetry tracing pkg:API Related to an API package
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Feb 16, 2021

Currently they use the label.KeyValue. This is out of compliance with the spec.

@MrAlias MrAlias added pkg:API Related to an API package area:trace Part of OpenTelemetry tracing release:required-for-ga labels Feb 16, 2021
@MrAlias MrAlias added this to the RC1 milestone Feb 16, 2021
@MrAlias MrAlias self-assigned this Feb 16, 2021
@Aneurysm9 Aneurysm9 added the good first issue Good for newcomers label Mar 24, 2021
@humivo
Copy link
Member

humivo commented Mar 26, 2021

Hello I would like to take on this issue

@Aneurysm9
Copy link
Member

@MrAlias I don't see a linked PR, so I'm going to assume you're fine with someone taking this off your hands!

@MrAlias MrAlias removed their assignment Mar 26, 2021
@MrAlias MrAlias removed the good first issue Good for newcomers label Apr 1, 2021
@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 1, 2021

Notes from SIG meeting (2021-04-01):

  • It is not ideal to change the interface to a string and not unify on a single attribute type.
  • Our attributes currently already have conversions to a string. If we want to keep this conversion we would need to have this conversion specified at the specification level.

@MrAlias MrAlias self-assigned this Apr 8, 2021
@MrAlias MrAlias removed their assignment Apr 29, 2021
@MrAlias MrAlias self-assigned this May 17, 2021
@MrAlias
Copy link
Contributor Author

MrAlias commented May 18, 2021

Talking with Bogdan it was made clear we want the TraceState to not include type. There is no way to interpret a value when deserializing as a particular type.

If we include a type it could lead to user expectation that we will correctly deserialize the type of their choosing. Meaning, for example, they create some enum type with a base type of an int and serialize that value it will become a string representation of an int, but when they deserialize it and expect it to be interpreted as the original type the string will not be interpreted as such and cause an error. The explicit expectation that the user handle the conversion becomes clear when we only support string values.

This may also be the same for baggage, but with the recent unification of attributes in metrics, there may be a situation where we want baggage to be transformed into attributes. Meaning that types may need to be communicated in the baggage. We are still exploring if this is a potential for the future and will plan based on findings.

@MrAlias
Copy link
Contributor Author

MrAlias commented May 20, 2021

Regarding the baggage rework, it was pointed out today in the SIG meeting that if we restructure the Baggage to handle metadata and revert to just using strings we can always add on another layer that looks for type information (if it becomes available) and abstract that back to our KeyValue type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing pkg:API Related to an API package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants