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 #2963

Merged
merged 10 commits into from Oct 19, 2022

Conversation

lgfa29
Copy link
Contributor

@lgfa29 lgfa29 commented Jun 20, 2022

The W3C spec specifies that values must be percent-encoded so when
reading the environment variable OTEL_RESOURCE_ATTRIBUTES the SDK
should decode them.

This is done by the baggage package, but its behaviour in case of
errors is slightly different from the current implementation of the SDK,
more specifically in cases where a key is missing a value. The SDK
returns a partial resource while the bagage package returns nil.

This may be considered a breaking change, so this commit fixes the
current implementation instead of using baggage.Parse.

The W3C spec specifies that values must be percent-encoded so when
reading the environment variable `OTEL_RESOURCE_ATTRIBUTES` the SDK
should decode them.

This is done by the `baggage` package, but its behaviour in case of
errors is slightly different from the current implementation of the SDK,
more specifically in cases where a key is missing a value. The SDK
returns a partial resource while the `bagage` package returns nil.

This may be considered a breaking change, so this commit fixes the
current implementation instead of using `baggage.Parse`.
Comment on lines 94 to 97
if err != nil {
// Retain original value if decoding fails.
v = field[1]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec actually defines that the invalid character should be replaced

When decoding the value, percent-encoded octet sequences that do not match the UTF-8 encoding scheme MUST be replaced with the replacement character (U+FFFD).

This would take quite a bit of extra work, so I thought it would be better to do it in the baggage package and then migrate the SDK to use it?

@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #2963 (f17714b) into main (430f558) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2963   +/-   ##
=====================================
  Coverage   77.8%   77.9%           
=====================================
  Files        164     164           
  Lines      11282   11289    +7     
=====================================
+ Hits        8784    8795   +11     
+ Misses      2301    2297    -4     
  Partials     197     197           
Impacted Files Coverage Δ
sdk/resource/env.go 94.0% <100.0%> (+0.9%) ⬆️
sdk/trace/batch_span_processor.go 81.9% <0.0%> (+1.7%) ⬆️

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is an appropriate change to make, at least not before there is consensus across the project. The specification states that ...

key value pairs [...] are expected to be represented in a format matching to the W3C Baggage

However, it links to a version of the baggage specification that does not decode values like the current version does.

Looking at the python and java parsing of this value, they do not decode this value. For compatibility with these other languages I would prefer to have consensus at the specification level before we make this change.

@lgfa29
Copy link
Contributor Author

lgfa29 commented Jun 23, 2022

I do not think this is an appropriate change to make, at least not before there is consensus across the project. The specification states that ...

key value pairs [...] are expected to be represented in a format matching to the W3C Baggage

However, it links to a version of the baggage specification that does not decode values like the current version does.

Looking at the python and java parsing of this value, they do not decode this value. For compatibility with these other languages I would prefer to have consensus at the specification level before we make this change.

That's a good point, and worth a broader discussion.

I would just point out that the reason I ran into this is because the baggage package in the Go SDK does encode values, so there's currently a mismatch in behaviour when generating the OTEL_RESOURCE_ATTRIBUTES and when reading them back.

For a little more context, this is what I'm doing but reading the values back this is what I get:
image

So if this change is not accepted, then perhaps the bagagge package should be changed so it doesn't encode values?

@MrAlias
Copy link
Contributor

MrAlias commented Jun 23, 2022

So if this change is not accepted, then perhaps the bagagge package should be changed so it doesn't encode values?

The editors draft that is linked from the specification for baggage propagation is a different version than the one linked for the SDK environment variables. We are currently compliant with the OTel specification based on these implementation.

That doesn't resolve your real issue though. I believe a PR to the specification to update the parsing link to match the editors draft (or even the latest published draft) would be a good idea. That way it will address the issue project wide.

@lgfa29
Copy link
Contributor Author

lgfa29 commented Jul 1, 2022

I wasn't sure how a spec update would work since they are both considered stable, so I open open-telemetry/opentelemetry-specification#2639 to start a discussion at the spec level.

I think I will also mark this PR as a draft until we have an update on the spec.

@lgfa29 lgfa29 marked this pull request as draft July 1, 2022 13:45
@lgfa29 lgfa29 marked this pull request as ready for review August 9, 2022 12:40
@lgfa29
Copy link
Contributor Author

lgfa29 commented Aug 9, 2022

Marking this as ready for review again since open-telemetry/opentelemetry-specification#2670 is merged and the spec is now updated 🙂

I also updated the CHANGELOG entry from Fixed to Changed while solving the merge conflict since this was not a bug as I initially thought.

@lgfa29
Copy link
Contributor Author

lgfa29 commented Aug 21, 2022

Hi everyone 👋

I just want to check if there's anything missing to get this PR merged?

Thanks!

@MrAlias
Copy link
Contributor

MrAlias commented Oct 18, 2022

@lgfa29 can you make sure to resolve all the open comments that are completed and addressed?

@lgfa29
Copy link
Contributor Author

lgfa29 commented Oct 19, 2022

@lgfa29 can you make sure to resolve all the open comments that are completed and addressed?

Ah sorry, both conversations had already been addressed. I marked them as resolved now 👍

CHANGELOG.md Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit 05aca23 into open-telemetry:main Oct 19, 2022
20 checks passed
@MrAlias MrAlias mentioned this pull request Oct 19, 2022
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.

None yet

8 participants