Skip to content

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s) #111289

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

Merged
merged 4 commits into from
Jan 14, 2025
Merged

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s) #111289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5d2e54d
Reset recorded flag when creating activity when not requested by samp…
CodeBlanch Jan 10, 2025
49fa829
Additional test coverage.
CodeBlanch Jan 10, 2025
295e8a8
Test fixes.
CodeBlanch Jan 10, 2025
6d93091
Code review.
CodeBlanch Jan 11, 2025
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
4 changes: 3 additions & 1 deletion src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,7 +1233,9 @@ internal static Activity Create(ActivitySource source, string name, ActivityKind
activity._parentSpanId = parentContext.SpanId.ToString();
}

activity.ActivityTraceFlags = parentContext.TraceFlags;
// Note: Don't inherit Recorded from parent as it is set below
// based on sampling decision
activity.ActivityTraceFlags = parentContext.TraceFlags & ~ActivityTraceFlags.Recorded;
Comment on lines -1236 to +1238
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this line exist at all? If W3C trace context introduces new flags, what is to say that inheriting the flags from the parent will be the correct thing to do for those new flags?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see this was already discussed... removing the line seems more correct to me...

Copy link
Contributor

Choose a reason for hiding this comment

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

In looking at the latest editor's draft of the spec, there is a new flag proposed: Random Trace ID Flag. My read of this flag is that it actually should be inherited from the parent, but there's no way to be certain this will be true for all new flags.

Copy link
Member

Choose a reason for hiding this comment

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

Keeping this line is not hurting for now. Any newly introduced flag will need to be decided if it is inheritable from the parent or not anyway.

activity._parentTraceFlags = (byte)parentContext.TraceFlags;
activity.HasRemoteParent = parentContext.IsRemote;
}
Expand Down
71 changes: 68 additions & 3 deletions src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ public void TestListeningToConstructedActivityEvents()
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void EnsureRecordingTest()
public void AllDataAndRecordedSamplingTest()
{
RemoteExecutor.Invoke(() => {
Activity.ForceDefaultIdFormat = true;
Expand All @@ -278,14 +278,76 @@ public void EnsureRecordingTest()

ActivitySource.AddActivityListener(listener);

Activity a = aSource.StartActivity("RecordedActivity");
// Note: Remote parent is set as NOT recorded
var parentContext = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.None, isRemote: true);

Activity a = aSource.StartActivity("RecordedActivity", ActivityKind.Internal, parentContext);
Assert.NotNull(a);

Assert.True(a.IsAllDataRequested);
Assert.True(a.Recorded);
Assert.True((a.Context.TraceFlags & ActivityTraceFlags.Recorded) != 0);
}).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void AllDataSamplingTest()
{
RemoteExecutor.Invoke(() => {
Activity.ForceDefaultIdFormat = true;
Activity.DefaultIdFormat = ActivityIdFormat.W3C;

ActivitySource aSource = new ActivitySource("EnsureRecordingTest");

ActivityListener listener = new ActivityListener
{
ShouldListenTo = (activitySource) => true,
Sample = (ref ActivityCreationOptions<ActivityContext> activityOptions) => ActivitySamplingResult.AllData
};

ActivitySource.AddActivityListener(listener);

// Note: Remote parent is set as recorded
var parentContext = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.Recorded, isRemote: true);

Activity a = aSource.StartActivity("RecordedActivity", ActivityKind.Internal, parentContext);
Assert.NotNull(a);

Assert.True(a.IsAllDataRequested);
Assert.False(a.Recorded);
Assert.False((a.Context.TraceFlags & ActivityTraceFlags.Recorded) != 0);
}).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void PropagationDataSamplingTest()
{
RemoteExecutor.Invoke(() => {
Activity.ForceDefaultIdFormat = true;
Activity.DefaultIdFormat = ActivityIdFormat.W3C;

ActivitySource aSource = new ActivitySource("EnsureRecordingTest");

ActivityListener listener = new ActivityListener
{
ShouldListenTo = (activitySource) => true,
Sample = (ref ActivityCreationOptions<ActivityContext> activityOptions) => ActivitySamplingResult.PropagationData
};

ActivitySource.AddActivityListener(listener);

// Note: Remote parent is set as recorded
var parentContext = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.Recorded, isRemote: true);

Activity a = aSource.StartActivity("RecordedActivity", ActivityKind.Internal, parentContext);
Assert.NotNull(a);

Assert.False(a.IsAllDataRequested);
Assert.False(a.Recorded);
Assert.False((a.Context.TraceFlags & ActivityTraceFlags.Recorded) != 0);
}).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void TestExpectedListenersReturnValues()
{
Expand Down Expand Up @@ -424,7 +486,10 @@ public void TestActivityCreationProperties()

Assert.Equal(ctx.TraceId, activity.TraceId);
Assert.Equal(ctx.SpanId, activity.ParentSpanId);
Assert.Equal(ctx.TraceFlags, activity.ActivityTraceFlags);
Assert.NotEqual(ctx.TraceFlags, activity.ActivityTraceFlags);
Assert.True(ctx.TraceFlags.HasFlag(ActivityTraceFlags.Recorded));
Assert.False(activity.ActivityTraceFlags.HasFlag(ActivityTraceFlags.Recorded));
Assert.False(activity.Recorded);
Assert.Equal(ctx.TraceState, activity.TraceStateString);
Assert.Equal(ActivityIdFormat.W3C, activity.IdFormat);

Expand Down
Loading