Decode values from OTEL_RESOURCE_ATTRIBUTES

[lgfa29]

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.

[lgfa29]

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]

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

@@          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%)	⬆️

[MrAlias]

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]

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:

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

[MrAlias]

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]

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]

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]

Hi everyone 👋

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

Thanks!

[MrAlias]

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

[lgfa29]

@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 👍