Skip to content

[api-baggage] revert space encoding change #5687

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

Merged
merged 5 commits into from
Jun 12, 2024
Merged

[api-baggage] revert space encoding change #5687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b14e6b3
revert space encoding change
lachmatt Jun 11, 2024
e474fa7
Update src/OpenTelemetry.Api/CHANGELOG.md
lachmatt Jun 11, 2024
6b04b9b
changelog formatting fix
lachmatt Jun 11, 2024
d8c2798
Merge branch 'main' into revert-baggage-space-encoding
CodeBlanch Jun 11, 2024
a0d4af5
Merge branch 'main' into revert-baggage-space-encoding
CodeBlanch Jun 12, 2024
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
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Api/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

* **Breaking change:** Revert space character encoding change from `+` to `%20`
Copy link
Member

@vishweshbankwar vishweshbankwar Jun 11, 2024

Choose a reason for hiding this comment

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

Add this PR # as well.

Copy link
Contributor Author

@lachmatt lachmatt Jun 11, 2024

Choose a reason for hiding this comment

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

Added in e474fa7

Copy link
Member

@alanwest alanwest Jun 11, 2024

Choose a reason for hiding this comment

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

Please add more information about why we're reverting this change to the changelog or PR description. Something to the effect of "... because the fix did not properly address the inconsistency with the baggage spec. We will address this issue more completely in a later release"

Copy link
Contributor Author

@lachmatt lachmatt Jun 12, 2024

Choose a reason for hiding this comment

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

PR description updated.

for baggage item values from [#5303](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5303)
([#5687](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5687))

## 1.9.0-rc.1

Released 2024-Jun-07
Expand Down
7 changes: 4 additions & 3 deletions src/OpenTelemetry.Api/Context/Propagation/BaggagePropagator.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.Net;
using System.Text;
using OpenTelemetry.Internal;

Expand Down Expand Up @@ -93,7 +94,7 @@ public override void Inject<T>(PropagationContext context, T carrier, Action<T,
continue;
}

baggage.Append(Uri.EscapeDataString(item.Key)).Append('=').Append(Uri.EscapeDataString(item.Value)).Append(',');
baggage.Append(WebUtility.UrlEncode(item.Key)).Append('=').Append(WebUtility.UrlEncode(item.Value)).Append(',');
}
while (e.MoveNext() && ++itemCount < MaxBaggageItems && baggage.Length < MaxBaggageLength);
baggage.Remove(baggage.Length - 1, 1);
Expand Down Expand Up @@ -140,8 +141,8 @@ internal static bool TryExtractBaggage(string[] baggageCollection, out Dictionar
continue;
}

var key = Uri.UnescapeDataString(parts[0]);
var value = Uri.UnescapeDataString(parts[1]);
var key = WebUtility.UrlDecode(parts[0]);
var value = WebUtility.UrlDecode(parts[1]);

if (string.IsNullOrEmpty(key) || string.IsNullOrEmpty(value))
{
Expand Down
17 changes: 4 additions & 13 deletions test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,7 @@ public void ValidateSpecialCharsBaggageExtraction()
Assert.Equal("key%28%293", escapedKey);
Assert.Equal("value%28%29%21%26%3B%3A", escapedValue);

var initialBaggage =
$"key%201=value%201,{encodedKey}={encodedValue},{escapedKey}={escapedValue},key4=%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F0123456789%3A%3B%3C%3D%3E%3F%40ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~";

var initialBaggage = $"key+1=value+1,{encodedKey}={encodedValue},{escapedKey}={escapedValue}";
var carrier = new List<KeyValuePair<string, string>>
{
new KeyValuePair<string, string>(BaggagePropagator.BaggageHeaderName, initialBaggage),
Expand All @@ -144,11 +142,11 @@ public void ValidateSpecialCharsBaggageExtraction()
Assert.False(propagationContext == default);
Assert.True(propagationContext.ActivityContext == default);

Assert.Equal(4, propagationContext.Baggage.Count);
Assert.Equal(3, propagationContext.Baggage.Count);

var actualBaggage = propagationContext.Baggage.GetBaggage();

Assert.Equal(4, actualBaggage.Count);
Assert.Equal(3, actualBaggage.Count);

Assert.True(actualBaggage.ContainsKey("key 1"));
Assert.Equal("value 1", actualBaggage["key 1"]);
Expand All @@ -158,10 +156,6 @@ public void ValidateSpecialCharsBaggageExtraction()

Assert.True(actualBaggage.ContainsKey("key()3"));
Assert.Equal("value()!&;:", actualBaggage["key()3"]);

// x20-x7E range
Assert.True(actualBaggage.ContainsKey("key4"));
Assert.Equal(" !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", actualBaggage["key4"]);
}

[Fact]
Expand Down Expand Up @@ -201,14 +195,11 @@ public void ValidateSpecialCharsBaggageInjection()
{
{ "key 1", "value 1" },
{ "key2", "!x_x,x-x&x(x\");:" },

// x20-x7E range
{ "key3", " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~" },
}));

this.baggage.Inject(propagationContext, carrier, Setter);

Assert.Single(carrier);
Assert.Equal("key%201=value%201,key2=%21x_x%2Cx-x%26x%28x%22%29%3B%3A,key3=%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F0123456789%3A%3B%3C%3D%3E%3F%40ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~", carrier[BaggagePropagator.BaggageHeaderName]);
Assert.Equal("key+1=value+1,key2=!x_x%2Cx-x%26x(x%22)%3B%3A", carrier[BaggagePropagator.BaggageHeaderName]);
}
}