[API] Baggage - Spec alignment changes

[nabutabu]

Towards #5689

Changes

Adds a more verbose test suite that ensures spec alignment and then changes the BaggagePropogator to follow these tests

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)

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
2954	1	2953	10

View the full list of 1 ❄️ flaky test(s)

OpenTelemetry.Api.FuzzTests.BaggagePropagatorFuzzTests::InjectExtractRoundTripPreservesSafeBaggage

Flake rate in main: 16.67% (Passed 10 times, Failed 2 times)

Stack Traces | 0.0221s run time

----- Inner Stack Trace -----

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

[nabutabu]

What I found while writing and editing the tests:

• on extract
  • keys are not currently being treated as literals according to spec and issue [BaggagePropagator] Do not encode and decode keys #5479. Also this discussion is important: baggage: Accept non-ASCII keys opentelemetry-go#4946
    • do not encode and decode keys at all
    • just drop invalid ones (anything containing CTLs, whitespace, DQUOTE, comma, semicolon, and backslash )
    • only happening for '+' sign, see results of ValidateValidTcharInKeyIsAcceptedOnExtract
  • values
    • incorrect decoding with '%20' was not converted to ' '
    • incorrect decoding '+' was incorrectly converted to ' '
• on inject,
  • value encoding is incorrect (eg. ' ' -> '+' when it should be ' ' -> '%20')
    • specifically for '+' ValidateSpecialCharactersInjectionForValue
    • all characters that should pass through inject with no change: ValueValidBaggageOctetCharPassesThroughUnchanged

Before moving forward:

• keep reviewing tests to ensure spec compliance
• find what changes are needed for BaggagePropogator
  • bring up these changes in SIG, breaking change

[nabutabu]

@martincostello Could I get a quick initial review on these tests before I start working on the changes for the propogator? Thanks!

[martincostello]

Seems reasonable from a skim-read.

[nabutabu]

Raw Benchmark Data

Before my changes

| Method  | ItemCount | UseSpecialChars | HeaderStyle | Mean        | Error      | StdDev     | Median      | Gen0   | Gen1   | Allocated |
|-------- |---------- |---------------- |------------ |------------:|-----------:|-----------:|------------:|-------:|-------:|----------:|
| Extract | 1         | False           | Properties  |   120.85 ns |   2.441 ns |   6.301 ns |   120.32 ns | 0.0410 |      - |     344 B |
| Inject  | 1         | False           | Properties  |    76.96 ns |   1.564 ns |   3.300 ns |    76.70 ns | 0.0181 |      - |     152 B |
| Extract | 1         | False           | W3C         |   139.40 ns |   2.817 ns |   6.909 ns |   137.98 ns | 0.0410 |      - |     344 B |
| Inject  | 1         | False           | W3C         |    70.21 ns |   1.449 ns |   3.270 ns |    70.23 ns | 0.0181 |      - |     152 B |
| Extract | 1         | True            | Properties  |   578.58 ns |  24.091 ns |  71.033 ns |   602.58 ns | 0.0925 |      - |     776 B |
| Inject  | 1         | True            | Properties  |   238.99 ns |   5.649 ns |  16.116 ns |   236.78 ns | 0.0515 |      - |     432 B |
| Extract | 1         | True            | W3C         |   363.38 ns |   7.201 ns |  14.547 ns |   362.51 ns | 0.0925 |      - |     776 B |
| Inject  | 1         | True            | W3C         |   191.29 ns |   3.831 ns |   4.845 ns |   191.69 ns | 0.0515 |      - |     432 B |
| Extract | 5         | False           | Properties  |   413.19 ns |   7.479 ns |   6.245 ns |   413.25 ns | 0.1049 | 0.0005 |     880 B |
| Inject  | 5         | False           | Properties  |   236.02 ns |   4.759 ns |   6.825 ns |   236.45 ns | 0.0582 |      - |     488 B |
| Extract | 5         | False           | W3C         |   513.41 ns |  10.072 ns |  22.528 ns |   508.12 ns | 0.1049 |      - |     880 B |
| Inject  | 5         | False           | W3C         |   243.32 ns |   4.909 ns |  10.776 ns |   241.97 ns | 0.0582 |      - |     488 B |
| Extract | 5         | True            | Properties  | 1,382.50 ns |  27.499 ns |  54.281 ns | 1,376.08 ns | 0.3624 | 0.0019 |    3040 B |
| Inject  | 5         | True            | Properties  |   820.35 ns |  16.120 ns |  37.998 ns |   819.69 ns | 0.2298 |      - |    1928 B |
| Extract | 5         | True            | W3C         | 1,445.38 ns |  28.821 ns |  67.368 ns | 1,442.03 ns | 0.3624 | 0.0019 |    3040 B |
| Inject  | 5         | True            | W3C         |   814.16 ns |  16.300 ns |  39.367 ns |   810.93 ns | 0.2298 |      - |    1928 B |
| Extract | 20        | False           | Properties  | 1,501.17 ns |  29.935 ns |  64.439 ns | 1,494.76 ns | 0.4272 | 0.0076 |    3576 B |
| Inject  | 20        | False           | Properties  |   795.07 ns |  14.345 ns |  21.471 ns |   792.91 ns | 0.2384 | 0.0010 |    2000 B |
| Extract | 20        | False           | W3C         | 1,905.92 ns |  33.061 ns |  29.307 ns | 1,910.59 ns | 0.4272 | 0.0076 |    3576 B |
| Inject  | 20        | False           | W3C         |   802.50 ns |  16.015 ns |  29.684 ns |   804.33 ns | 0.2384 | 0.0010 |    2000 B |
| Extract | 20        | True            | Properties  | 5,123.01 ns | 100.644 ns | 165.361 ns | 5,155.75 ns | 1.4648 | 0.0305 |   12296 B |
| Inject  | 20        | True            | Properties  | 2,949.73 ns |  58.668 ns | 153.524 ns | 2,916.64 ns | 0.8163 | 0.0038 |    6832 B |
| Extract | 20        | True            | W3C         | 5,612.90 ns | 109.314 ns | 228.180 ns | 5,572.05 ns | 1.4648 | 0.0305 |   12296 B |
| Inject  | 20        | True            | W3C         | 2,889.92 ns |  57.324 ns | 138.443 ns | 2,892.45 ns | 0.8163 | 0.0038 |    6832 B |

After my changes

| Method  | ItemCount | UseSpecialChars | HeaderStyle | Mean        | Error     | StdDev     | Gen0   | Gen1   | Allocated |
|-------- |---------- |---------------- |------------ |------------:|----------:|-----------:|-------:|-------:|----------:|
| Extract | 1         | False           | Properties  |    86.80 ns |  1.528 ns |   2.716 ns | 0.0411 |      - |     344 B |
| Inject  | 1         | False           | Properties  |    69.21 ns |  1.419 ns |   2.767 ns | 0.0229 |      - |     192 B |
| Extract | 1         | False           | W3C         |    36.90 ns |  0.761 ns |   1.207 ns | 0.0067 |      - |      56 B |
| Inject  | 1         | False           | W3C         |    69.73 ns |  1.413 ns |   2.619 ns | 0.0229 |      - |     192 B |
| Extract | 1         | True            | Properties  |   198.06 ns |  3.940 ns |   9.132 ns | 0.0801 |      - |     672 B |
| Inject  | 1         | True            | Properties  |   224.86 ns |  4.485 ns |   9.460 ns | 0.0832 |      - |     696 B |
| Extract | 1         | True            | W3C         |    34.72 ns |  0.723 ns |   1.444 ns | 0.0067 |      - |      56 B |
| Inject  | 1         | True            | W3C         |   216.02 ns |  4.326 ns |   9.220 ns | 0.0832 |      - |     696 B |
| Extract | 5         | False           | Properties  |   299.65 ns |  5.966 ns |  13.096 ns | 0.1049 | 0.0005 |     880 B |
| Inject  | 5         | False           | Properties  |   234.77 ns |  4.698 ns |  10.411 ns | 0.0820 |      - |     688 B |
| Extract | 5         | False           | W3C         |    67.33 ns |  1.381 ns |   2.230 ns | 0.0067 |      - |      56 B |
| Inject  | 5         | False           | W3C         |   227.87 ns |  4.554 ns |   6.532 ns | 0.0823 |      - |     688 B |
| Extract | 5         | True            | Properties  |   820.86 ns | 16.333 ns |  36.193 ns | 0.3004 | 0.0019 |    2520 B |
| Inject  | 5         | True            | Properties  |   932.39 ns | 18.606 ns |  33.551 ns | 0.3834 | 0.0010 |    3208 B |
| Extract | 5         | True            | W3C         |    68.69 ns |  1.408 ns |   2.191 ns | 0.0067 |      - |      56 B |
| Inject  | 5         | True            | W3C         |   963.07 ns | 19.278 ns |  35.732 ns | 0.3834 |      - |    3208 B |
| Extract | 20        | False           | Properties  | 1,034.73 ns | 20.502 ns |  37.488 ns | 0.4272 | 0.0076 |    3576 B |
| Inject  | 20        | False           | Properties  |   842.20 ns | 16.734 ns |  29.308 ns | 0.3347 | 0.0010 |    2800 B |
| Extract | 20        | False           | W3C         |   205.17 ns |  4.105 ns |   5.192 ns | 0.0067 |      - |      56 B |
| Inject  | 20        | False           | W3C         |   794.50 ns | 15.605 ns |  28.535 ns | 0.3347 | 0.0010 |    2800 B |
| Extract | 20        | True            | Properties  | 3,523.02 ns | 70.014 ns | 131.504 ns | 1.2093 | 0.0305 |   10136 B |
| Inject  | 20        | True            | Properties  | 3,519.56 ns | 69.175 ns | 139.738 ns | 1.4153 | 0.0076 |   11856 B |
| Extract | 20        | True            | W3C         |   219.32 ns |  4.306 ns |   5.894 ns | 0.0067 |      - |      56 B |
| Inject  | 20        | True            | W3C         | 3,276.47 ns | 63.636 ns | 125.612 ns | 1.4153 | 0.0076 |   11856 B |

Notable Changes

Before

| Extract | 20        | True            | W3C         | 5,612.90 ns | 109.314 ns | 228.180 ns | 5,572.05 ns | 1.4648 | 0.0305 |   12296 B |
| Inject  | 20        | True            | W3C         | 2,889.92 ns |  57.324 ns | 138.443 ns | 2,892.45 ns | 0.8163 | 0.0038 |    6832 B |

After

| Extract | 20        | True            | W3C         |   219.32 ns |  4.306 ns |   5.894 ns | 0.0067 |      - |      56 B |
| Inject  | 20        | True            | W3C         | 3,276.47 ns | 63.636 ns | 125.612 ns | 1.4153 | 0.0076 |   11856 B |

[nabutabu]

@Kielek as mentioned in #7009 Baggage needs more tests and requirements that need to be fixed. This PR is a collection of those missing changes that the spec mentions.
Build currently fails because OWS and properties tests fail but those should be addressed by #7009 and will pass once that is merged to main and main is merged to this

This is a bigger LOC change so let me also give a more verbose description of the changes. Lot of this will quote my previous comment:

Keys

• Extract:
  • Keys are no longer being encoded but are checked for validity. Spec does not allow:

  Delimiters are chosen from the set of US-ASCII visual characters not allowed in a token (DQUOTE and "(),/:;<=>?@[]{}").

• Inject:
  • Key is now using Uri.EscapeDataString which means delimiters are now being escaped. Look at test KeyWithDelimiterCharIsPercentEncodedOnInject

Values

• Extract:
  • No changes
• Inject
  • Is encoded if any characters outside the baggage-octet are seen. Checks are performed using a ReadOnlySpan (similar to keys during Extract and IsValidKey) to ensure performance

  baggage-octet = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
  ; US-ASCII characters excluding CTLs,
  ; whitespace, DQUOTE, comma, semicolon,
  ; and backslash

Let me know if you'd like more information.

[nabutabu]

Working on Fuzz tests which are failing due to increased restrictions on allowed characters