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

Decode values from OTEL_RESOURCE_ATTRIBUTES #1352

Closed
lgfa29 opened this issue Jul 31, 2022 · 4 comments
Closed

Decode values from OTEL_RESOURCE_ATTRIBUTES #1352

lgfa29 opened this issue Jul 31, 2022 · 4 comments
Labels
spec-compliance/v1.13.0 spec-compliance Required for OpenTelemetry spec compliance stale

Comments

@lgfa29
Copy link

lgfa29 commented Jul 31, 2022

While using the Go SDK I noticed an inconsistency in how OTEL_RESOURCE_ATTRIBUTES baggage values are encoded and decoded. This was due to a difference in the version of the W3C Baggage spec used for baggage propagation and the OTEL_RESOURCE_ATTRIBUTES decoding.

open-telemetry/opentelemetry-specification#2670 updates the W3C Baggage spec version used by the Resource spec to require percent-decoding values from OTEL_RESOURCE_ATTRIBUTES.

@ahayworth
Copy link
Contributor

I haven't parsed the differences between the versions of the W3C spec in question... but if it turns out that the current decoding logic in the baggage text-map propagator is correct then this would be a relatively easy change for us to make (thank you CGI.unescape!).

However, we should wait until the spec change is accepted. Once it is, we can definitely address this. Thank you for the heads-up, I'll add this to our spec-compliance tracking project. ❤️ 🙇

@ahayworth ahayworth added this to To do in Spec Compliance via automation Aug 1, 2022
@ahayworth ahayworth added the spec-compliance Required for OpenTelemetry spec compliance label Aug 1, 2022
@lgfa29
Copy link
Author

lgfa29 commented Aug 2, 2022

I think the main change is the requirement to percent-decode values. There are some additional considerations about privacy and security but I think they are mostly targeted at end-users.

Looking at the code you linked it does seem like that's all that is needed. Here's the relevant Go update as reference: https://github.com/open-telemetry/opentelemetry-go/pull/2963/files#diff-b00b30ddb9242080b6237eb51f35a5120cb88036d6892571a8f430d8f3152e0dR94

One interesting thing I noticed is that the W3C spec mandates that the keys must only contain visible ASCII characters (that's why they don't need percent-decoding), but a had a quick glance at the Otel SDKs and none of them seem to enforce this.

@robbkidd
Copy link
Member

Noting here that the spec update has merged to the spec, so we are green-lit for rubbing percents on things.

Copy link
Contributor

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.

@github-actions github-actions bot added the stale label Mar 11, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 12, 2024
Spec Compliance automation moved this from To do to Done Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec-compliance/v1.13.0 spec-compliance Required for OpenTelemetry spec compliance stale
Projects
Development

No branches or pull requests

3 participants