Skip to content

Add ActivitySource.CreateActivity methods #48374

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
Feb 23, 2021
Merged

Add ActivitySource.CreateActivity methods #48374

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
e91b69a
Add ActivitySource.CreateActivity methods
tarekgh Feb 17, 2021
92c5a64
Address the feedback
tarekgh Feb 17, 2021
29fde6f
Add IdFormat parameter to ActivitySource.Create and fix the whole IdF…
tarekgh Feb 20, 2021
9471f9a
Address @noahfalk feedback
tarekgh Feb 22, 2021
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
3 changes: 3 additions & 0 deletions ...es/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ public sealed class ActivitySource : IDisposable
public string Name { get { throw null; } }
public string? Version { get { throw null; } }
public bool HasListeners() { throw null; }
public System.Diagnostics.Activity? CreateActivity(string name, System.Diagnostics.ActivityKind kind) { throw null; }
public System.Diagnostics.Activity? CreateActivity(string name, System.Diagnostics.ActivityKind kind, System.Diagnostics.ActivityContext parentContext, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>>? tags = null, System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink>? links = null, System.Diagnostics.ActivityIdFormat idFormat = System.Diagnostics.ActivityIdFormat.Unknown) { throw null; }
public System.Diagnostics.Activity? CreateActivity(string name, System.Diagnostics.ActivityKind kind, string parentId, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>>? tags = null, System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink>? links = null, System.Diagnostics.ActivityIdFormat idFormat = System.Diagnostics.ActivityIdFormat.Unknown) { throw null; }
public System.Diagnostics.Activity? StartActivity([System.Runtime.CompilerServices.CallerMemberName] string name = "", System.Diagnostics.ActivityKind kind = ActivityKind.Internal) { throw null; }
public System.Diagnostics.Activity? StartActivity(string name, System.Diagnostics.ActivityKind kind, System.Diagnostics.ActivityContext parentContext, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>>? tags = null, System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink>? links = null, System.DateTimeOffset startTime = default) { throw null; }
public System.Diagnostics.Activity? StartActivity(string name, System.Diagnostics.ActivityKind kind, string parentId, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>>? tags = null, System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink>? links = null, System.DateTimeOffset startTime = default) { throw null; }
Expand Down
77 changes: 30 additions & 47 deletions 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 @@ -1007,55 +1007,15 @@ public void SetCustomProperty(string propertyName, object? propertyValue)
return ret;
}

internal static Activity CreateAndStart(ActivitySource source, string name, ActivityKind kind, string? parentId, ActivityContext parentContext,
IEnumerable<KeyValuePair<string, object?>>? tags, IEnumerable<ActivityLink>? links,
DateTimeOffset startTime, ActivityTagsCollection? samplerTags, ActivitySamplingResult request)
internal static Activity Create(ActivitySource source, string name, ActivityKind kind, string? parentId, ActivityContext parentContext,
IEnumerable<KeyValuePair<string, object?>>? tags, IEnumerable<ActivityLink>? links, DateTimeOffset startTime,
ActivityTagsCollection? samplerTags, ActivitySamplingResult request, bool startIt, ActivityIdFormat idFormat)
{
Activity activity = new Activity(name);

activity.Source = source;
activity.Kind = kind;

activity._previousActiveActivity = Current;
if (parentId != null)
{
activity._parentId = parentId;
}
else if (parentContext != default)
{
activity._traceId = parentContext.TraceId.ToString();

if (parentContext.SpanId != default)
{
activity._parentSpanId = parentContext.SpanId.ToString();
}

activity.ActivityTraceFlags = parentContext.TraceFlags;
activity._traceState = parentContext.TraceState;
}
else
{
if (activity._previousActiveActivity != null)
{
// The parent change should not form a loop. We are actually guaranteed this because
// 1. Un-started activities can't be 'Current' (thus can't be 'parent'), we throw if you try.
// 2. All started activities have a finite parent change (by inductive reasoning).
activity.Parent = activity._previousActiveActivity;
}
}

activity.IdFormat =
ForceDefaultIdFormat ? DefaultIdFormat :
activity.Parent != null ? activity.Parent.IdFormat :
activity._parentSpanId != null ? ActivityIdFormat.W3C :
activity._parentId == null ? DefaultIdFormat :
IsW3CId(activity._parentId) ? ActivityIdFormat.W3C :
ActivityIdFormat.Hierarchical;

if (activity.IdFormat == ActivityIdFormat.W3C)
activity.GenerateW3CId();
else
activity._id = activity.GenerateHierarchicalId();
activity.IdFormat = idFormat;

if (links != null)
{
Expand Down Expand Up @@ -1091,16 +1051,39 @@ internal static Activity CreateAndStart(ActivitySource source, string name, Acti
}
}

activity.StartTimeUtc = startTime == default ? GetUtcNow() : startTime.UtcDateTime;

activity.IsAllDataRequested = request == ActivitySamplingResult.AllData || request == ActivitySamplingResult.AllDataAndRecorded;

if (request == ActivitySamplingResult.AllDataAndRecorded)
{
activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded;
}

SetCurrent(activity);
if (parentId != null)
{
activity._parentId = parentId;
}
else if (parentContext != default)
{
activity._traceId = parentContext.TraceId.ToString();

if (parentContext.SpanId != default)
{
activity._parentSpanId = parentContext.SpanId.ToString();
}

activity.ActivityTraceFlags = parentContext.TraceFlags;
activity._traceState = parentContext.TraceState;
}

if (startTime != default)
{
activity.StartTimeUtc = startTime.UtcDateTime;
}

if (startIt)
Copy link
Member

Choose a reason for hiding this comment

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

Does anything prevent us from doing?

Suggested change
if (startIt)
if (startIt)
{
Start();
}

It feels odd to be replicating all this start code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is possible. The only different case would be when someone calling StartActivity and passing ActivityContext with a default span Id. we used to keep Activity.Parent=null but if we call Activity.Start() it would set Activity.Parent to the Activity.Current. I am not sure though if this will be ok (or should be the right behavior). what you think?

Copy link
Member

Choose a reason for hiding this comment

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

In theory spanId = 0 is an invalid parent context. We should probably take a peak at the spec and chat with the OT folks to see if they have any input on what error handling behavior is desirable. I don't recall us ever defining the behavior very precisely but what I see the code currently do is give the bad id (in string or ActivityContext form) to the sampler as-is. Later in CreateAndStart we potentially ignore the id (if it was in string form) and create a new id based on IdFormat. This runs afoul of our sampler TraceId == Activity.TraceId invariant the same as above.

I don't care too much about the factoring on its own, but I am glad thinking about it sussed out issues like this one : )

{
activity.Start();
}

return activity;
}
Expand Down
49 changes: 43 additions & 6 deletions ...ies/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityCreationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,29 +23,58 @@ public readonly struct ActivityCreationOptions<T>
/// <param name="kind"><see cref="ActivityKind"/> to create the Activity object with.</param>
/// <param name="tags">Key-value pairs list for the tags to create the Activity object with.<see cref="ActivityContext"/></param>
/// <param name="links"><see cref="ActivityLink"/> list to create the Activity object with.</param>
internal ActivityCreationOptions(ActivitySource source, string name, T parent, ActivityKind kind, IEnumerable<KeyValuePair<string, object?>>? tags, IEnumerable<ActivityLink>? links)
/// <param name="idFormat">The default Id format to use.</param>
internal ActivityCreationOptions(ActivitySource source, string name, T parent, ActivityKind kind, IEnumerable<KeyValuePair<string, object?>>? tags, IEnumerable<ActivityLink>? links, ActivityIdFormat idFormat)
{
Source = source;
Name = name;
Kind = kind;
Parent = parent;
Tags = tags;
Links = links;
IdFormat = idFormat;

if (IdFormat == ActivityIdFormat.Unknown && Activity.ForceDefaultIdFormat)
{
IdFormat = Activity.DefaultIdFormat;
}

_samplerTags = null;

if (parent is ActivityContext ac)
if (parent is ActivityContext ac && ac != default)
{
_context = ac;
if (IdFormat == ActivityIdFormat.Unknown)
{
IdFormat = ActivityIdFormat.W3C;
}
}
else if (parent is string p && p != null)
{
// We don't care about the return value. we care if _context is initialized accordingly.
ActivityContext.TryParse(p, null, out _context);
if (IdFormat != ActivityIdFormat.Hierarchical)
{
if (ActivityContext.TryParse(p, null, out _context))
{
IdFormat = ActivityIdFormat.W3C;
}

if (IdFormat == ActivityIdFormat.Unknown)
{
IdFormat =ActivityIdFormat.Hierarchical;
}
}
else
{
_context = default;
}
}
else
{
_context = default;
if (IdFormat == ActivityIdFormat.Unknown)
{
IdFormat = Activity.Current != null ? Activity.Current.IdFormat : Activity.DefaultIdFormat;
}
}
}

Expand Down Expand Up @@ -103,16 +132,24 @@ public ActivityTraceId TraceId
#endif
get
{
if (Parent is ActivityContext && _context == default)
if (Parent is ActivityContext && IdFormat == ActivityIdFormat.W3C && _context == default)
{
Func<ActivityTraceId>? traceIdGenerator = Activity.TraceIdGenerator;
ActivityTraceId id = traceIdGenerator == null ? ActivityTraceId.CreateRandom() : traceIdGenerator();

// Because the struct is readonly, we cannot directly assign _context. We have to workaround it by calling Unsafe.AsRef
Unsafe.AsRef(in _context) = new ActivityContext(ActivityTraceId.CreateRandom(), default, ActivityTraceFlags.None);
Unsafe.AsRef(in _context) = new ActivityContext(id, default, ActivityTraceFlags.None);
}

return _context.TraceId;
}
}

/// <summary>
/// Retrieve Id format of to use for the Activity we may create.
/// </summary>
internal ActivityIdFormat IdFormat { get; }

// Helper to access the sampling tags. The SamplingTags Getter can allocate when not necessary.
internal ActivityTagsCollection? GetSamplingTags() => _samplerTags;

Expand Down
Loading