Skip to content

Commit

Permalink
Fix W3CTraceContextPropagator for non-standard array types (#6654)
Browse files Browse the repository at this point in the history 
## Summary of changes

The `W3CTraceContextPropagator` would check whether there was a single
`traceparent` header value to determine whether or not it should extract
W3C headers further. For non-standard array types it would _always_
return `false` meaning that headers wouldn't be extracted. This pull
request fixes that so that non-standard array types will return `true`
to allow extraction to happen.

## Reason for change

If we have valid headers we should extract them.

## Implementation details

Swapped to `GetEnumerator` as it seems more readable than the `foreach`
route.
If we have one value return `true` instead of `false`.
Swapped the `TryGetSingle` and `TryGetSingleRare` to `internal` to add
direct unit tests to them.

## Test coverage

- Added tests for `TryGetSingle` and `TryGetSingleRare` testing
fundamental behavior.
- Added tests for `Extract` to test possible behavior we could see.

## Other details
<!-- Fixes #{issue} -->

Fixes #6596

<!-- ⚠️ Note: where possible, please obtain 2 approvals prior to
merging. Unless CODEOWNERS specifies otherwise, for external teams it is
typically best to have one review from a team member, and one review
from apm-dotnet. Trivial changes do not require 2 reviews. -->
  • Loading branch information
@bouwkast
bouwkast authored Feb 21, 2025
1 parent 2860cab commit b8967f9
Show file tree
Hide file tree
Showing 2 changed files with 161 additions and 18 deletions.
38 changes: 20 additions & 18 deletions tracer/src/Datadog.Trace/Propagators/W3CTraceContextPropagator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,8 @@ public bool TryExtract<TCarrier, TCarrierGetter>(
return true;
}

private static bool TryGetSingle(IEnumerable<string?> values, out string value)
// internal for regression testing
internal static bool TryGetSingle(IEnumerable<string?> values, out string value)
{
// fast path for string[], List<string>, and others
if (values is IReadOnlyList<string?> list)
Expand All @@ -683,28 +684,29 @@ private static bool TryGetSingle(IEnumerable<string?> values, out string value)
return TryGetSingleRare(values, out value);
}

private static bool TryGetSingleRare(IEnumerable<string?> values, out string value)
// internal for regression testing
internal static bool TryGetSingleRare(IEnumerable<string?> values, out string value)
{
value = string.Empty;
var hasValue = false;
using var enumerator = values.GetEnumerator();

foreach (var s in values)
if (!enumerator.MoveNext())
{
if (!hasValue)
{
// save first item
value = s ?? string.Empty;
hasValue = true;
}
else
{
// we already saved the first item and there is a second one
return false;
}
// there were no items
value = string.Empty;
return false;
}

// there were no items
return false;
// store first value
value = enumerator.Current ?? string.Empty;

// is there a second value?
if (enumerator.MoveNext())
{
value = string.Empty;
return false; // more than one value
}

return true;
}

private static string TrimAndJoinStrings(IEnumerable<string?> values)
Expand Down
141 changes: 141 additions & 0 deletions tracer/test/Datadog.Trace.Tests/Propagators/W3CTraceContextPropagatorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

using System;
using System.Collections.Generic;
using System.Linq;
using Datadog.Trace.ExtensionMethods;
using Datadog.Trace.Headers;
using Datadog.Trace.Propagators;
Expand All @@ -17,6 +18,7 @@ namespace Datadog.Trace.Tests.Propagators
{
public class W3CTraceContextPropagatorTests
{
private const string DummyTraceParent = "00-000000000000000000000000075bcd15-000000003ade68b1-01";
private const string ZeroLastParentId = "0000000000000000";

private static readonly TraceTagCollection PropagatedTagsCollection = new(
Expand Down Expand Up @@ -47,6 +49,85 @@ static W3CTraceContextPropagatorTests()
};
}

/// <summary>
/// The <see cref="W3CTraceContextPropagator"/> uses <see cref="W3CTraceContextPropagator.TryGetSingle(IEnumerable{string?}, out string)"/>
/// and a <see cref="W3CTraceContextPropagator.TryGetSingleRare(IEnumerable{string?}, out string)"/> for extraction of the traceparent headers.
/// However, this was failing and always return false for non-standard array types (e.g. a queue or hashset).
/// This data tests those methods directly (as it is signifcantly easier to see the error).
/// </summary>
/// <returns>The test data</returns>
public static IEnumerable<object[]> TryGetSingleTestCases()
{
yield return new object[] { new HashSet<string> { "foo" } }; // Triggers TryGetSingleRare
yield return new object[] { new List<string> { "foo" } }; // Triggers fast path
yield return new object[] { new string[] { "foo" } }; // Triggers fast path
yield return new object[] { new Queue<string>(new[] { "foo" }) }; // Triggers TryGetSingleRare
yield return new object[] { Enumerable.Range(0, 1).Select(_ => "foo") }; // Triggers TryGetSingleRare
yield return new object[] { GetYieldValues("foo") }; // Triggers TryGetSingleRare
}

/// <summary>
/// The <see cref="W3CTraceContextPropagator"/> uses <see cref="W3CTraceContextPropagator.TryGetSingle(IEnumerable{string?}, out string)"/>
/// and a <see cref="W3CTraceContextPropagator.TryGetSingleRare(IEnumerable{string?}, out string)"/> for extraction of the traceparent headers.
/// However, this was failing and always return false for non-standard array types (e.g. a queue or hashset).
/// </summary>
/// <returns>The test data</returns>
public static IEnumerable<object[]> TryExtractTestCases()
{
yield return new object[]
{
new[] { DummyTraceParent }, // Fast path (string[])
true // VALID
};

yield return new object[]
{
new List<string> { DummyTraceParent }, // Fast path (List<string>)
true // VALID
};

yield return new object[]
{
new HashSet<string> { DummyTraceParent }, // TryGetSingleRare path (HashSet<string>)
true // VALID
};

yield return new object[]
{
new Queue<string>(new[] { DummyTraceParent }), // TryGetSingleRare path (Queue<string>)
true // VALID
};

yield return new object[]
{
GetYieldValues(DummyTraceParent), // TryGetSingleRare path (yield return)
true // VALID
};

// edge cases that are invalid, but have likely already been covered by other tests
yield return new object[] { Array.Empty<string>(), false }; // No headers
yield return new object[] { new[] { string.Empty }, false }; // Empty string header
yield return new object[] { new[] { " " }, false }; // Whitespace-only header
yield return new object[] { new[] { "invalid" }, false }; // Invalid traceparent format
yield return new object[]
{
new[]
{
DummyTraceParent,
DummyTraceParent
},
false // Multiple headers should fail
};
}

public static IEnumerable<string> GetYieldValues(params string[] values)
{
foreach (var value in values)
{
yield return value;
}
}

[Theory]
[InlineData(0, 123456789, 987654321, null, "00-000000000000000000000000075bcd15-000000003ade68b1-01")]
[InlineData(0, 123456789, 987654321, SamplingPriorityValues.UserReject, "00-000000000000000000000000075bcd15-000000003ade68b1-00")]
Expand Down Expand Up @@ -1008,5 +1089,65 @@ public void Extract_Sampled0_MissingSamplingPriority_UsesSamplingPriorityOf0()
},
opts => opts.ExcludingMissingMembers());
}

[Fact]
public void TryGetSingleRare_ShouldReturnFalse_ForEmptyValues()
{
W3CTraceContextPropagator.TryGetSingleRare(Array.Empty<string>(), out _).Should().BeFalse();
}

[Fact]
public void TryGetSingleRare_ShouldReturnTrue_ForSingleValue()
{
W3CTraceContextPropagator.TryGetSingleRare(new[] { "foo" }, out var value).Should().BeTrue();
value.Should().Be("foo");
}

[Fact]
public void TryGetSingleRare_ShouldReturnFalse_ForMultipleValues()
{
W3CTraceContextPropagator.TryGetSingleRare(new[] { "foo", "bar" }, out _).Should().BeFalse();
}

[Fact]
public void TryGetSingle_ShouoldReturnFalse_ForEmptyValeus()
{
W3CTraceContextPropagator.TryGetSingle(Array.Empty<string>(), out _).Should().BeFalse();
}

[Theory]
[MemberData(nameof(TryGetSingleTestCases))]
public void TryGetSingle_ShouldReturnTrue_ForSingleValues(IEnumerable<string> values)
{
W3CTraceContextPropagator.TryGetSingle(values, out var value).Should().BeTrue();
value.Should().Be("foo");
}

[Theory]
[MemberData(nameof(TryExtractTestCases))]
public void TryExtract_ShouldReturnExpectedResult(IEnumerable<string> traceParentHeaders, bool expectedSuccess)
{
var headers = new Mock<IHeadersCollection>(MockBehavior.Strict);
headers.Setup(h => h.GetValues("traceparent"))
.Returns(traceParentHeaders);
headers.Setup(h => h.GetValues("tracestate"))
.Returns(new[] { "dd=s:2;o:rum;t.dm:-4;t.usr.id:12345" });

var carrier = headers.Object;
var carrierGetter = new Mock<ICarrierGetter<IHeadersCollection>>();
carrierGetter.Setup(c => c.Get(carrier, "traceparent"))
.Returns(traceParentHeaders);

var result = W3CPropagator.Extract(headers.Object, (carrier, name) => carrier.GetValues(name));

if (expectedSuccess)
{
result.SpanContext.Should().NotBeNull();
}
else
{
result.SpanContext.Should().BeNull();
}
}
}
}

0 comments on commit b8967f9

Please sign in to comment.