Skip to content

Decode the OTEL_RESOURCE_ATTRIBUTES environment variable according to the spec #6461

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.

Sign up for GitHub

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Decode the OTEL_RESOURCE_ATTRIBUTES environment variable according to the spec #6461

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b05c573
parse OTEL_RESOURCE_ATTRIBUTES according to spec
hannahhaering Sep 1, 2025
9e35708
added comments
hannahhaering Sep 2, 2025
aef230e
added changelog entry
hannahhaering Sep 2, 2025
e244ed1
simplified test code
hannahhaering Sep 3, 2025
b59b52c
Update src/OpenTelemetry/CHANGELOG.md
hannahhaering Sep 3, 2025
985617b
Update src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs
hannahhaering Sep 3, 2025
9191ac2
removed directive in test
hannahhaering Sep 3, 2025
febb6c2
implemented centralized baggage encoding; added unit tests
hannahhaering Sep 14, 2025
5263bb3
Merge remote-tracking branch 'origin' into parse-resource-envVar
hannahhaering Sep 14, 2025
b55fea3
disable warnings in unit tests
hannahhaering Sep 15, 2025
a9fbfc2
Merge branch 'main' into parse-resource-envVar
hannahhaering Oct 1, 2025
9624ed9
use Unicode escapes in InlineData to avoid non-ASCII chars
hannahhaering Oct 1, 2025
4bb160d
replace remaining non-ASCII chars in InlineData with Unicode escapes
hannahhaering Oct 1, 2025
7c30944
fix non-ASCII literals in PercentEncodingHelperTests
hannahhaering Oct 1, 2025
7656241
retry workflow
hannahhaering Oct 1, 2025
2636bd0
Merge branch 'main' into parse-resource-envVar
hannahhaering Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ Notes](../../RELEASENOTES.md).

## Unreleased

* Added decoding of the `OTEL_RESOURCE_ATTRIBUTES` variable according to the specification,
adhering to the [W3C Baggage](https://github.com/w3c/baggage/blob/main/baggage/HTTP_HEADER_FORMAT.md)
format.
([#6461](https://github.com/open-telemetry/opentelemetry-dotnet/pull/6461))

* Added `FormatMessage` configuration option to self-diagnostics feature. When
set to `true` (default is false), log messages will be formatted by replacing
placeholders with actual parameter values for improved readability.
Expand Down
66 changes: 63 additions & 3 deletions src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

using System.Text;
using Microsoft.Extensions.Configuration;

namespace OpenTelemetry.Resources;
Expand Down Expand Up @@ -38,15 +39,74 @@ private static List<KeyValuePair<string, object>> ParseResourceAttributes(string
string[] rawAttributes = resourceAttributes.Split(AttributeListSplitter);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

foreach (string rawKeyValuePair in rawAttributes)
{
string[] keyValuePair = rawKeyValuePair.Split(AttributeKeyValueSplitter);
if (keyValuePair.Length != 2)
#if NETSTANDARD2_1 || NET8_0_OR_GREATER
var indexOfFirstEquals = rawKeyValuePair.IndexOf(AttributeKeyValueSplitter.ToString(), StringComparison.Ordinal);
#else
var indexOfFirstEquals = rawKeyValuePair.IndexOf(AttributeKeyValueSplitter);
#endif
if (indexOfFirstEquals == -1)
{
continue;
}

attributes.Add(new KeyValuePair<string, object>(keyValuePair[0].Trim(), keyValuePair[1].Trim()));
var key = rawKeyValuePair.Substring(0, indexOfFirstEquals).Trim();
var value = rawKeyValuePair.Substring(indexOfFirstEquals + 1).Trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

string allocations could be removed with span operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above.

Copy link
Contributor

Choose a reason for hiding this comment

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

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();


if (!IsValidKeyValuePair(key, value))
{
continue;
}

var decodedValue = DecodeValue(value);

attributes.Add(new KeyValuePair<string, object>(key, decodedValue));
}

return attributes;
}

private static bool IsValidKeyValuePair(string key, string value) =>
!string.IsNullOrEmpty(key) && !string.IsNullOrEmpty(value) && key.All(c => c <= 127);

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);
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

50 changes: 32 additions & 18 deletions test/OpenTelemetry.Tests/Resources/OtelEnvResourceDetectorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,35 +37,49 @@ public void OtelEnvResource_NullEnvVar()
Assert.Equal(Resource.Empty, resource);
}

[Fact]
public void OtelEnvResource_WithEnvVar_1()
[Theory]
[InlineData("key1=val1,key2=val2", new string[] { "key1", "key2" }, new string[] { "val1", "val2" })]
[InlineData("key1,key2=val2", new string[] { "key2" }, new string[] { "val2" })]
[InlineData("key=Am%C3%A9lie", new string[] { "key" }, new string[] { "Amélie" })] // Valid percent-encoded value
[InlineData("key1=val1,key2=val2==3", new string[] { "key1", "key2" }, new string[] { "val1", "val2==3" })] // Valid value with equal sign
[InlineData("key1=,key2=val2", new string[] { "key2" }, new string[] { "val2" })] // Empty value for key1
[InlineData("=val1,key2=val2", new string[] { "key2" }, new string[] { "val2" })] // Empty key for key1
[InlineData("Amélie=val", new string[] { }, new string[] { })] // Invalid key
[InlineData("key=invalid%encoding", new string[] { "key" }, new string[] { "invalid%encoding" })] // Invalid value
[InlineData("key=v1+v2", new string[] { "key" }, new string[] { "v1+v2" })]
[InlineData("key=a%E0%80Am%C3%A9lie", new string[] { "key" }, new string[] { "a��Amélie" })]
public void OtelEnvResource_EnvVar_Validation(string envVarValue, string[] expectedKeys, string[] expectedValues)
{
// Arrange
var envVarValue = "Key1=Val1,Key2=Val2";
Environment.SetEnvironmentVariable(OtelEnvResourceDetector.EnvVarKey, envVarValue);
var resource = new OtelEnvResourceDetector(
new ConfigurationBuilder().AddEnvironmentVariables().Build())
.Detect();
#if NET
Assert.NotNull(expectedKeys);
Assert.NotNull(expectedValues);
#else
if (expectedKeys == null)
{
throw new ArgumentNullException(nameof(expectedKeys));
}

// Assert
Assert.NotEqual(Resource.Empty, resource);
Assert.Contains(new KeyValuePair<string, object>("Key1", "Val1"), resource.Attributes);
}
if (expectedValues == null)
{
throw new ArgumentNullException(nameof(expectedValues));
}
#endif

[Fact]
public void OtelEnvResource_WithEnvVar_2()
{
// Arrange
var envVarValue = "Key1,Key2=Val2";
Environment.SetEnvironmentVariable(OtelEnvResourceDetector.EnvVarKey, envVarValue);
var resource = new OtelEnvResourceDetector(
new ConfigurationBuilder().AddEnvironmentVariables().Build())
.Detect();

// Assert
Assert.NotEqual(Resource.Empty, resource);
Assert.Single(resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("Key2", "Val2"), resource.Attributes);
Assert.Equal(expectedKeys.Length, expectedValues.Length);
Assert.Equal(expectedKeys.Length, resource.Attributes.Count());
for (int i = 0; i < expectedKeys.Length; i++)
{
Assert.Equal(
expectedKeys.Zip(expectedValues, (k, v) => new KeyValuePair<string, object>(k, v)), resource.Attributes);
}
}

[Fact]
Expand Down