Decode the OTEL_RESOURCE_ATTRIBUTES environment variable according to the spec #6461

hannahhaering · 2025-09-02T16:36:25Z

Changes

Added percent-decoding of the OTEL_RESOURCE_ATTRIBUTES variable according to the spec, adhering to the W3C Baggage format. As in the Go implementation, I retained the original value in case decoding fails.

Merge requirement checklist

CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs

martincostello · 2025-09-02T17:01:20Z

src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs

+    private static string DecodeValue(string baggageEncoded)
+    {
+        var bytes = new List<byte>();
+        for (int i = 0; i < baggageEncoded.Length; i++)
+        {
+            if (baggageEncoded[i] == '%' && i + 2 < baggageEncoded.Length && IsHex(baggageEncoded[i + 1]) && IsHex(baggageEncoded[i + 2]))
+            {
+                string hex = baggageEncoded.Substring(i + 1, 2);
+                bytes.Add(Convert.ToByte(hex, 16));
+
+                i += 2;
+            }
+            else if (baggageEncoded[i] == '%')
+            {
+                return baggageEncoded; // Bad percent triplet -> return original value
+            }
+            else
+            {
+                if (!IsBaggageOctet(baggageEncoded[i]))
+                {
+                    return baggageEncoded; // non-encoded character not baggage octet encoded -> return original value
+                }
+
+                bytes.Add((byte)baggageEncoded[i]);
+            }
+        }
+
+        return new UTF8Encoding(false, false).GetString(bytes.ToArray());
+    }
+
+    private static bool IsHex(char c) =>
+        (c >= '0' && c <= '9') ||
+        (c >= 'a' && c <= 'f') ||
+        (c >= 'A' && c <= 'F');
+
+    private static bool IsBaggageOctet(char c) =>
+    c == 0x21 ||
+    (c >= 0x23 && c <= 0x2B) ||
+    (c >= 0x2D && c <= 0x3A) ||
+    (c >= 0x3C && c <= 0x5B) ||
+    (c >= 0x5D && c <= 0x7E);
 }


Can we use WebUtility.UrlDecode() here, rather than hand-roll our own?

We can, but there is one difference. The UrlDecode method would replace + in the value string with space in the decoded string. This behavior is not part of the specification which uses only percent encoding (unlike application/x-www-form-urlencoded data, where + should be decoded into space). The current specification links to this.
However, client libraries in other languages seem to mostly use UrlDecode. If you prefer to follow their (incorrect 😄) approach I can change it.

I did a quick search, and it seems that we already use WebUtility.UrlEncode() here to encode baggage (added in #2012), so I think on balance it's best to match that and also have less code to maintain.

@martincostello, I would consider this as a bug. See: #5689

Maybe we should go with the simpler option first (i.e. the slightly non-compliant WebUtility.UrlDecode() for consistency), then in a follow-up we can add a compliant implementation (maybe using the optimised/tested code from this method itself and amending as appropriate) as shared code, then update all the appropriate places at once to use it and fix the spec-drift all together?

I'm fine with both approaches. I don't feel qualified to decide this 😄 Could you tell me which decoding to use? @Kielek @martincostello

@hannahhaering, any option that you can start with bugfixing baggage propagator with proper de/encoding? Based on this we can adjust this PR.

I would like to avoid introducing intentional bug.

I can try to do this. I can start doing this in a few days (maybe earlier).

I will mark this PR as draft in the meantime.

I implemented a new helper class for percent encoding. I used this to encode and decode the baggage in the propagator and in the resource detector.

I also added some unit tests. I added a test for non-ascii character decoding, this makes a the linter script fail. Any way around this?

@Kielek I updated the tests and the linter doesn't complain anymore. I think this PR is ready. Do you have any other remarks/comments?

test/OpenTelemetry.Tests/Resources/OtelEnvResourceDetectorTests.cs

src/OpenTelemetry/CHANGELOG.md

Co-authored-by: Martin Costello <martin@martincostello.com>

src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs

paulomorgado · 2025-09-03T08:12:46Z

src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs

    {
        var attributes = new List<KeyValuePair<string, object>>();

        string[] rawAttributes = resourceAttributes.Split(AttributeListSplitter);


These string[] and string allocations could be removed with span operations.

I considered using spans but decided against it since this method is only used for config parsing. Supporting mixed .NET targets (some without full span support) would require two versions, adding unnecessary complexity and reducing readability. I don’t think the trade-off is worth it here.

Are you saying this is the only place in the whole codebase that splits strings and shouldn't?

Polyfills can be easily created for targets that miss some features. I've done that more than once.

paulomorgado · 2025-09-03T08:14:02Z

src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs

+            var key = rawKeyValuePair.Substring(0, indexOfFirstEquals).Trim();
+            var value = rawKeyValuePair.Substring(indexOfFirstEquals + 1).Trim();


string allocations could be removed with span operations.

Suggested change

var key = rawKeyValuePair.Substring(0, indexOfFirstEquals).Trim();

var value = rawKeyValuePair.Substring(indexOfFirstEquals + 1).Trim();

var key = rawKeyValuePair.AsSpan(0, indexOfFirstEquals).Trim().ToString();

var value = rawKeyValuePair.AsSpan(indexOfFirstEquals + 1).Trim().ToString();

test/OpenTelemetry.Tests/Resources/OtelEnvResourceDetectorTests.cs

Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>

codecov · 2025-09-14T21:51:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.60%. Comparing base (2eb4373) to head (2636bd0).
⚠️ Report is 124 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6461      +/-   ##
==========================================
- Coverage   86.69%   86.60%   -0.09%     
==========================================
  Files         258      258              
  Lines       11910    11885      -25     
==========================================
- Hits        10325    10293      -32     
- Misses       1585     1592       +7

Flag	Coverage Δ
unittests-Project-Experimental	`86.34% <100.00%> (+0.01%)`	⬆️
unittests-Project-Stable	`86.27% <100.00%> (-0.24%)`	⬇️
unittests-Solution	`86.54% <100.00%> (-0.08%)`	⬇️
unittests-UnstableCoreLibraries-Experimental	`85.87% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...metry.Api/Context/Propagation/BaggagePropagator.cs	`75.67% <100.00%> (-9.33%)`	⬇️
...OpenTelemetry/Resources/OtelEnvResourceDetector.cs	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

github-actions · 2025-09-22T03:28:03Z