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.

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

[Tracer] Extracting Last Parent Id If Conflicting SpanIds are Found with The W3C Headers #5721

Merged
merged 10 commits into from
Jul 1, 2024
2 changes: 1 addition & 1 deletion docker-compose.windows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ services:
- "8126:8126"
environment:
- SNAPSHOT_CI=1
- SNAPSHOT_IGNORED_ATTRS=span_id,trace_id,parent_id,duration,start,metrics.system.pid,meta.runtime-id,metrics.process_id,meta.http.client_ip,meta.network.client.ip,meta._dd.p.dm,meta._dd.p.tid
- SNAPSHOT_IGNORED_ATTRS=span_id,trace_id,parent_id,duration,start,metrics.system.pid,meta.runtime-id,metrics.process_id,meta.http.client_ip,meta.network.client.ip,meta._dd.p.dm,meta._dd.p.tid,meta._dd.parent_id

smoke-tests.windows:
build:
Expand Down
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,7 @@ services:
environment:
- ENABLED_CHECKS=trace_count_header,meta_tracer_version_header,trace_content_length
- SNAPSHOT_CI=1
- SNAPSHOT_IGNORED_ATTRS=span_id,trace_id,parent_id,duration,start,metrics.system.pid,meta.runtime-id,metrics.process_id,meta._dd.p.dm,meta._dd.p.tid
- SNAPSHOT_IGNORED_ATTRS=span_id,trace_id,parent_id,duration,start,metrics.system.pid,meta.runtime-id,metrics.process_id,meta._dd.p.dm,meta._dd.p.tid,meta._dd.parent_id

smoke-tests:
build:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ internal class SpanMessagePackFormatter : IMessagePackFormatter<TraceChunkModel>
private readonly byte[] _versionNameBytes = StringEncoding.UTF8.GetBytes(Trace.Tags.Version);

private readonly byte[] _originNameBytes = StringEncoding.UTF8.GetBytes(Trace.Tags.Origin);
private readonly byte[] _lastParentIdBytes = StringEncoding.UTF8.GetBytes("_dd.parent_id");
private readonly byte[] _lastParentIdBytes = StringEncoding.UTF8.GetBytes(Trace.Tags.LastParentId);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks!


// numeric tags
private readonly byte[] _metricsBytes = StringEncoding.UTF8.GetBytes("metrics");
Expand Down
19 changes: 18 additions & 1 deletion tracer/src/Datadog.Trace/Propagators/SpanContextPropagator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
using System.Linq;
using Datadog.Trace.ExtensionMethods;
using Datadog.Trace.Headers;
using Datadog.Trace.Propagators;
using Datadog.Trace.Util;

namespace Datadog.Trace.Propagators
{
Expand Down Expand Up @@ -165,11 +167,26 @@ internal void Inject<TCarrier, TCarrierSetter>(SpanContext context, TCarrier car
return spanContext;
}

if (localSpanContext is not null && spanContext is not null)
if (localSpanContext is not null && spanContext is not null && _extractors[i] is W3CTraceContextPropagator)
{
if (localSpanContext.RawTraceId == spanContext.RawTraceId)
link04 marked this conversation as resolved.
Show resolved Hide resolved
{
localSpanContext.AdditionalW3CTraceState += spanContext.AdditionalW3CTraceState;

if (localSpanContext.RawSpanId != spanContext.RawSpanId)
{
if (!string.IsNullOrEmpty(spanContext.LastParentId) && spanContext.LastParentId != W3CTraceContextPropagator.ZeroLastParent)
{
localSpanContext.LastParentId = spanContext.LastParentId;
}
else
{
localSpanContext.LastParentId = HexString.ToHexString(localSpanContext.SpanId);
}

localSpanContext.SpanId = spanContext.SpanId;
localSpanContext.RawSpanId = spanContext.RawSpanId;
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ internal class W3CTraceContextPropagator : IContextInjector, IContextExtractor

// zero value (16 zeroes) for when there isn't a last parent (`p`)
// this value indicates that the backend can make this span as the root span if necessary of a trace
private const string ZeroLastParent = "0000000000000000";
internal const string ZeroLastParent = "0000000000000000";

private static readonly KeyValuePair<char, char>[] InjectOriginReplacements =
{
Expand Down
10 changes: 7 additions & 3 deletions tracer/src/Datadog.Trace/SpanContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ private SpanContext(TraceId traceId, string serviceName)
/// <summary>
/// Gets the span id.
/// </summary>
public ulong SpanId { get; }
public ulong SpanId { get; internal set; }
Copy link
Member

@lucaspimentel lucaspimentel Jun 25, 2024

Choose a reason for hiding this comment

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

This feels iffy. Why are we adding a property setter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So part of the W3C Phase 3 is the future deprecation of Datadog headers, so when we find conflicting headers we prioritize W3C span id and need to update them, in the latest commit I do so like:
fdd4fd5#diff-3f905f6ce54156702b562c581f1ff7866c41fbafb6b29b79c6c2411c79c19e55R187-R188

localSpanContext.SpanId = spanContext.SpanId;
localSpanContext.RawSpanId = spanContext.RawSpanId;

Do you have any other prefer methods, I Googled the internal set and found no complaints so far.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this is SpanContext. I thought this was Span. 😅

Still, it would've been nice to mutate things as little as possible. Maybe determine the final span id first before creating the SpanContext, but that's a larger refactor.

Thanks for clarifying.


/// <summary>
/// Gets or sets the service name to propagate to child spans.
Expand Down Expand Up @@ -251,10 +251,14 @@ internal string Origin
internal string RawTraceId => _rawTraceId ??= HexString.ToHexString(TraceId128);

/// <summary>
/// Gets the span id as a hexadecimal string of length 16,
/// Gets or sets the span id as a hexadecimal string of length 16,
/// padded with zeros to the left if needed.
/// </summary>
internal string RawSpanId => _rawSpanId ??= HexString.ToHexString(SpanId);
internal string RawSpanId
{
get => _rawSpanId ??= HexString.ToHexString(SpanId);
set => _rawSpanId = value;
}

/// <summary>
/// Gets or sets additional key/value pairs from an upstream "tracestate" W3C header that we will propagate downstream.
Expand Down
6 changes: 6 additions & 0 deletions tracer/src/Datadog.Trace/Tags.cs
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,12 @@ public static partial class Tags
/// </summary>
internal const string BaseService = "_dd.base_service";

/// <summary>
/// Tag used to propagate the unsigned 64 bits last parent Id
/// lower-case 16 characters hexadecimal string
/// </summary>
internal const string LastParentId = "_dd.parent_id";

internal static class User
{
internal const string Email = "usr.email";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,8 @@ public static void AddSimpleScrubber(this VerifySettings settings, string oldVal
?.Where(kvp => !kvp.Key.StartsWith(TagPropagation.PropagatedTagPrefix, StringComparison.Ordinal))
// We must ignore both `_dd.git.repository_url` and `_dd.git.commit.sha` because we are only setting it on the first span of a trace
// no matter what. That means we have unstable snapshot results.
.Where(kvp => kvp.Key != Tags.GitRepositoryUrl && kvp.Key != Tags.GitCommitSha)
// Also ignoring `_dd.parent_id` since we test specific headers combinations which check for the value, hence why not adding it to the snapshots
.Where(kvp => kvp.Key != Tags.GitRepositoryUrl && kvp.Key != Tags.GitCommitSha && kvp.Key != Tags.LastParentId)
.Select(
kvp => kvp.Key switch
{
Expand Down Expand Up @@ -232,7 +233,8 @@ public static void AddSimpleScrubber(this VerifySettings settings, string oldVal
?.Where(kvp => !kvp.Key.StartsWith(TagPropagation.PropagatedTagPrefix, StringComparison.Ordinal))
// We must ignore both `_dd.git.repository_url` and `_dd.git.commit.sha` because we are only setting it on the first span of a trace
// no matter what. That means we have unstable snapshot results.
.Where(kvp => kvp.Key != Tags.GitRepositoryUrl && kvp.Key != Tags.GitCommitSha)
// Also ignoring `_dd.parent_id` since we test specific headers combinations which check for the value, hence why not adding it to the snapshots
.Where(kvp => kvp.Key != Tags.GitRepositoryUrl && kvp.Key != Tags.GitCommitSha && kvp.Key != Tags.LastParentId)
.Select(
kvp => kvp.Key switch
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public static void DefaultTagAssertions(SpanTagAssertion<T> s) => s
.IsOptional("version")
.IsOptional("_dd.p.dm") // "decision maker", but contains the sampling mechanism
.IsOptional("_dd.p.tid") // contains the upper 64 bits of a 128-bit trace id
.IsOptional("_dd.parent_id") // Contains the 16 length hex decoded last found parent_id found set from either exising `p` tag on tracestate or headers
.IsOptional("error.msg")
.IsOptional("error.type")
.IsOptional("error.stack")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@

using System;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.Globalization;
using Datadog.Trace.Headers;
using Datadog.Trace.Propagators;
using Datadog.Trace.Tagging;
using Datadog.Trace.Util;
using FluentAssertions;
using Moq;
using Xunit;
Expand Down Expand Up @@ -37,6 +39,8 @@ public class MultiSpanContextPropagatorTests
private static readonly SpanContextPropagator DatadogW3CPropagatorExtractFirstTrue;
private static readonly SpanContextPropagator W3CDatadogPropagatorExtractFirstFalse;
private static readonly SpanContextPropagator DatadogW3CPropagatorExtractFirstFalse;
private static readonly SpanContextPropagator DatadogB3PropagatorExtractFirstFalse;
private static readonly SpanContextPropagator B3W3CPropagatorExtractFirstFalse;

static MultiSpanContextPropagatorTests()
{
Expand Down Expand Up @@ -105,6 +109,34 @@ static MultiSpanContextPropagatorTests()
ContextPropagationHeaderStyle.W3CTraceContext,
},
false);

// Datadog-B3MultipleHeaders
DatadogB3PropagatorExtractFirstFalse = SpanContextPropagatorFactory.GetSpanContextPropagator(
new[]
{
ContextPropagationHeaderStyle.Datadog,
ContextPropagationHeaderStyle.B3MultipleHeaders,
},
new[]
{
ContextPropagationHeaderStyle.Datadog,
ContextPropagationHeaderStyle.B3MultipleHeaders,
},
false);

// B3MultipleHeaders-W3CTraceContext
B3W3CPropagatorExtractFirstFalse = SpanContextPropagatorFactory.GetSpanContextPropagator(
new[]
{
ContextPropagationHeaderStyle.B3MultipleHeaders,
ContextPropagationHeaderStyle.W3CTraceContext,
},
new[]
{
ContextPropagationHeaderStyle.B3MultipleHeaders,
ContextPropagationHeaderStyle.W3CTraceContext,
},
false);
}

[Fact]
Expand Down Expand Up @@ -722,6 +754,8 @@ public void TraceContextPrecedence_ConsistentBehaviour_WithDifferentParentId(boo
},
null);

bool expectW3cParentIds = w3CHeaderFirst || !extractFirst;

result.Should()
.NotBeNull()
.And
Expand All @@ -730,16 +764,16 @@ public void TraceContextPrecedence_ConsistentBehaviour_WithDifferentParentId(boo
{
TraceId128 = new TraceId(0x1111111111111111, 4),
TraceId = 4,
SpanId = (ulong)(w3CHeaderFirst ? 987654321 : 3540),
SpanId = (ulong)(expectW3cParentIds ? 987654321 : 3540),
RawTraceId = "11111111111111110000000000000004",
RawSpanId = w3CHeaderFirst ? "000000003ade68b1" : "0000000000000dd4",
RawSpanId = expectW3cParentIds ? "000000003ade68b1" : "0000000000000dd4",
SamplingPriority = 2,
PropagatedTags = propagatedTags,
AdditionalW3CTraceState = !extractFirst || w3CHeaderFirst ? "foo=1" : null,
AdditionalW3CTraceState = expectW3cParentIds ? "foo=1" : null,
Parent = null,
ParentId = null,
IsRemote = true,
LastParentId = w3CHeaderFirst ? "0123456789abcdef" : null, // if we have Datadog headers don't use p
LastParentId = expectW3cParentIds ? "0123456789abcdef" : null,
});
}

Expand Down Expand Up @@ -798,6 +832,44 @@ public void TraceContextPrecedence_ConsistentBehaviour_WithDifferentTraceIds(boo
});
}

[Theory]
[InlineData("dd=s:2;p:000000003ade68b1,foo=1", "1", "987654321", 987654321, null)]
[InlineData("dd=s:2;p:000000000000000a,foo=1", "2", "10", 10, null)]
[InlineData("dd=s:2;p:000000000000000a,foo=1", "1", "10", 987654321, "000000000000000a")]
[InlineData("dd=s:2,foo=1", "1", "10", 987654321, "000000000000000a")]
[InlineData("dd=s:2;p:8fffffffffffffff,foo=1", "1", "10", 987654321, "8fffffffffffffff")]
public void Datadog_W3C_TraceState_ParentExtracted(string tracestate, string traceId, string parentId, ulong expectedSpanId, string expectedParentTag)
link04 marked this conversation as resolved.
Show resolved Hide resolved
{
var uLongParentId = Convert.ToUInt64(parentId);
var headers = new NameValueHeadersCollection(new NameValueCollection());

// W3C Headers
headers.Add("traceparent", "00-00000000000000000000000000000001-000000003ade68b1-01");
headers.Add("tracestate", tracestate);
// Datadog Headers
headers.Add("x-datadog-trace-id", traceId);
headers.Add("x-datadog-parent-id", parentId);
// B3 Multi Headers
headers.Add("x-b3-traceid", $"0000000000000000000000000000000{traceId}");
headers.Add("x-b3-spanid", $"{HexString.ToHexString(uLongParentId)}");
headers.Add("x-b3-sampled", "1");

var resultDatadogW3C = DatadogW3CPropagatorExtractFirstFalse.Extract(headers);
resultDatadogW3C.Should().NotBeNull();
resultDatadogW3C?.SpanId.Should().Be(expectedSpanId);
resultDatadogW3C?.LastParentId.Should().Be(expectedParentTag);

var resultDatadogB3 = DatadogB3PropagatorExtractFirstFalse.Extract(headers);
resultDatadogB3.Should().NotBeNull();
resultDatadogB3?.SpanId.Should().Be(uLongParentId);
resultDatadogB3?.LastParentId.Should().Be(null);

var resultB3W3C = B3W3CPropagatorExtractFirstFalse.Extract(headers);
resultB3W3C.Should().NotBeNull();
resultB3W3C?.SpanId.Should().Be(expectedSpanId);
resultB3W3C?.LastParentId.Should().Be(expectedParentTag);
}

private SpanContextPropagator GetPropagatorToTest(bool extractFirst, bool w3CHeaderFirst)
=> (w3CHeaderFirst, extractFirst) switch
{
Expand Down
Loading