Skip to content
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

Fix W3CTraceContextPropagator for non-standard array types #6654

Merged
merged 1 commit into from
Feb 21, 2025
+161 −18
Merged

Fix W3CTraceContextPropagator for non-standard array types #6654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 20 additions & 18 deletions tracer/src/Datadog.Trace/Propagators/W3CTraceContextPropagator.cs
View file
Open in desktop
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;
Copy link
Member

Choose a reason for hiding this comment

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

It took me a while to figure out the underlying bug. In short, this last line should have been

return hasValue;

instead of

return false;

otherwise there's never any way for this method to return true!

// 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
Open in desktop
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();
}
}
}
}
Loading