Skip to content

Commit

Permalink
Do not reset Activity.Id for non-W3C formats (#5739)
Browse files Browse the repository at this point in the history
## Summary of changes

This makes it so that we validate that the `Activity` object is in W3C
format (by checking that the `Activity` has a SpanID and TraceID after
it stars) if we are updating the SpanId and TraceId.

Note that for this to happen the `Activity` needs to be a non-W3C format
_and_ it's start time must be less than the Datadog Active Span start
time. This start time issue can happen as an `Activity` can modify its
start time _after_ it is started and we don't update the created Datadog
span with that new information.

## Reason for change

T fixes an edge case where an `Activity` object could be in a non-W3C
format and hit this code resulting in its `Id` being erroneously set to
`null` causing a `NullReferenceException` for us (workaround in #5690),
but could have also thrown if anyone accessed that `Id` and attempted to
make a call on it.

## Implementation details

Check to ensure that the `Activity` is in W3C format - this is done by
just checking that it already has a Span Id and Trace Id.

## Test coverage

Added tests to `Samples.NetActivitySdk`, the
`RunManuallyUpdatedStartTime()` would fail and throw an exception.

## Other details

Should consider handling instances where users update the start time of
the `Activity` and update the Datadog `Span` accordingly.

<!-- ⚠️ 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 authored Jul 2, 2024
1 parent 6d3106c commit b8c6db0
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,9 @@ public static void ActivityStarted<T>(string sourceName, T activity, OpenTelemet
{
// We ensure the activity follows the same TraceId as the span
// And marks the ParentId the current spanId

if (activity.Parent is null || activity.Parent.StartTimeUtc < activeSpan.StartTime.UtcDateTime)
if ((activity.Parent is null || activity.Parent.StartTimeUtc < activeSpan.StartTime.UtcDateTime)
&& activitySpanId is not null
&& activityTraceId is not null)
{
// TraceId (always 32 chars long even when using 64-bit ids)
w3cActivity.TraceId = activeSpan.Context.RawTraceId;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ private static void IgnoreActivity<T>(T activity, Span? span)
{
if (span is not null && activity is IW3CActivity w3cActivity)
{
if (activity.Parent is null || activity.Parent.StartTimeUtc < span.StartTime.UtcDateTime)
if ((activity.Parent is null || activity.Parent.StartTimeUtc < span.StartTime.UtcDateTime)
&& w3cActivity.SpanId is not null
&& w3cActivity.TraceId is not null)
{
// If we ignore the activity and there's an existing active span
// We modify the activity spanId with the one in the span
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public async Task SubmitsTraces()
using (var agent = EnvironmentHelper.GetMockAgent())
using (await RunSampleAndWaitForExit(agent))
{
const int expectedSpanCount = 50;
const int expectedSpanCount = 53;
var spans = agent.WaitForSpans(expectedSpanCount);

using var s = new AssertionScope();
Expand Down
54 changes: 54 additions & 0 deletions tracer/test/snapshots/NetActivitySdkTests.verified.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1062,5 +1062,59 @@
span.kind: server,
version: 1.0.0
}
},
{
TraceId: Id_60,
SpanId: Id_61,
Name: internal,
Resource: TimeParent,
Service: Samples.NetActivitySdk,
Type: custom,
Tags: {
env: integration_tests,
language: dotnet,
otel.status_code: STATUS_CODE_UNSET,
runtime-id: Guid_2,
span.kind: internal,
version: 1.0.0
},
Metrics: {
process_id: 0,
_dd.top_level: 1.0,
_dd.tracer_kr: 1.0,
_sampling_priority_v1: 1.0
}
},
{
TraceId: Id_60,
SpanId: Id_62,
Name: internal,
Resource: TimeTrigger,
Service: Samples.NetActivitySdk,
Type: custom,
ParentId: Id_61,
Tags: {
env: integration_tests,
language: dotnet,
otel.status_code: STATUS_CODE_UNSET,
span.kind: internal,
version: 1.0.0
}
},
{
TraceId: Id_60,
SpanId: Id_63,
Name: internal,
Resource: TimeChild,
Service: Samples.NetActivitySdk,
Type: custom,
ParentId: Id_62,
Tags: {
env: integration_tests,
language: dotnet,
otel.status_code: STATUS_CODE_UNSET,
span.kind: internal,
version: 1.0.0
}
}
]
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ public static async Task Main(string[] args)
RunNonW3CId(); // 2 spans (total 46)
RunActivityReservedAttributes(); // 1 span (47 total)

RunManuallyUpdatedStartTime(); // 3 spans (50 total)

await Task.Delay(1000);
}

Expand All @@ -70,6 +72,7 @@ private static void PrintSpanStoppedInformation(Activity activity)
Console.WriteLine($"Activity.StatusDescription: {activity.StatusDescription}");
Console.WriteLine($"Activity.TraceStateString: {activity.TraceStateString}");
Console.WriteLine($"Activity.Source.Name: {activity.Source.Name}");
Console.WriteLine($"Activity.STartTime: {activity.StartTimeUtc}");
Console.WriteLine("Tags:");
foreach(var tag in activity.TagObjects)
{
Expand Down Expand Up @@ -171,6 +174,40 @@ private static void RunNonW3CId()
}
}

private static void RunManuallyUpdatedStartTime()
{
using (var parent = new Activity("TimeParent"))
{
parent.SetIdFormat(ActivityIdFormat.Hierarchical);
parent.SetStartTime(new DateTime(1980, 1, 1, 0, 0, 0, DateTimeKind.Utc));
parent.Start();
using (var manuallySetStartTime = new Activity("TimeTrigger"))
{
manuallySetStartTime.SetIdFormat(ActivityIdFormat.Hierarchical);
manuallySetStartTime.Start();
// ISSUE: Activity's can have their start time modified after they are started
// We use the Activity.Parent.StartTime as a basis to check whether to
// nest an Activity as a child node.
// This code then clears/updates various Span/Trace ID values on the Activity
// and resets the Activity.Id value.
// The issue here is that for Hierarchical IDs we were setting a Span/Trace ID
// when we shouldn't have been and then clearing out the Activity.Id.
// The Activity.Id would then be null and would cause issues for us and the customer's
// application if they did anything with the ID.
manuallySetStartTime.SetStartTime(new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc));
using (var child = new Activity("TimeChild"))
{
child.SetIdFormat(ActivityIdFormat.Hierarchical);
child.Start();

// Without the fix, this child.Id.Substring(1) call would throw an NRE
// as we cleared out the ID wrongly.
Console.WriteLine(child.Id.Substring(1));
}
}
}
}

private static void RunActivityUpdate()
{
using var errorSpan = _source.StartActivity("ErrorSpan");
Expand Down

0 comments on commit b8c6db0

Please sign in to comment.