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?

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 (80858c5) to head (b55fea3).
⚠️ Report is 31 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6461      +/-   ##
==========================================
- Coverage   86.61%   86.60%   -0.02%     
==========================================
  Files         258      258              
  Lines       11878    11853      -25     
==========================================
- Hits        10288    10265      -23     
+ Misses       1590     1588       -2

Flag	Coverage Δ
unittests-Project-Experimental	`86.53% <100.00%> (+0.16%)`	⬆️
unittests-Project-Stable	`86.30% <100.00%> (-0.26%)`	⬇️
unittests-Solution	`86.55% <100.00%> (+0.21%)`	⬆️
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 2 files with indirect coverage changes

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

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

github-actions · 2025-09-30T03:22:08Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

hannahhaering added 2 commits September 1, 2025 18:52

parse OTEL_RESOURCE_ATTRIBUTES according to spec

b05c573

added comments

9e35708

github-actions bot added the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Sep 2, 2025

added changelog entry

aef230e

hannahhaering marked this pull request as ready for review September 2, 2025 16:46

hannahhaering requested a review from a team as a code owner September 2, 2025 16:46

martincostello reviewed Sep 2, 2025

View reviewed changes

hannahhaering and others added 2 commits September 2, 2025 22:33

simplified test code

e244ed1

Update src/OpenTelemetry/CHANGELOG.md

b59b52c

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

Kielek reviewed Sep 3, 2025

View reviewed changes

src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs Outdated Show resolved Hide resolved

paulomorgado reviewed Sep 3, 2025

View reviewed changes

martincostello reviewed Sep 3, 2025

View reviewed changes

test/OpenTelemetry.Tests/Resources/OtelEnvResourceDetectorTests.cs Show resolved Hide resolved

hannahhaering and others added 2 commits September 3, 2025 10:31

Update src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs

985617b

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

removed directive in test

9191ac2

hannahhaering marked this pull request as draft September 10, 2025 01:09

hannahhaering added 2 commits September 14, 2025 17:45

implemented centralized baggage encoding; added unit tests

febb6c2

Merge remote-tracking branch 'origin' into parse-resource-envVar

5263bb3

github-actions bot added the pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package label Sep 14, 2025

disable warnings in unit tests

b55fea3

hannahhaering marked this pull request as ready for review September 15, 2025 02:34

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 22, 2025

Kielek removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 22, 2025

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 30, 2025

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

Decode the OTEL_RESOURCE_ATTRIBUTES environment variable according to the spec #6461

Are you sure you want to change the base?

Decode the OTEL_RESOURCE_ATTRIBUTES environment variable according to the spec #6461

Uh oh!

Conversation

hannahhaering commented Sep 2, 2025

Changes

Merge requirement checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Sep 22, 2025

Uh oh!

github-actions bot commented Sep 30, 2025

Uh oh!

Uh oh!

codecov bot commented Sep 14, 2025 •

edited

Loading