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

Add ActivitySource.CreateActivity methods #48374

Merged
merged 4 commits into from
Feb 23, 2021
Merged

Add ActivitySource.CreateActivity methods #48374

Show file tree
Hide file tree
Changes from 1 commit
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) { 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) { throw null; }
Copy link
Member

Choose a reason for hiding this comment

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

Weren't we going to handle #43853 by adding an IdFormat parameter here?

Copy link
Member Author

@tarekgh tarekgh Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No. After discussing this in the design review, @pakrym felt the other proposal is not needed and that can be handled by this CreateActivity proposal. After creating the activity, they can set the IdFormat on such created Activity before starting it.

Copy link
Member

Choose a reason for hiding this comment

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

Unforetunately that doesn't work. CreateActivity() calls the sampling callback, sampling callback has access to the ID, generating that ID has already fixed the id format. Using SetIdFormat() after CreateActivity() should probably throw an exception if it doesn't already.

Copy link
Member Author

@tarekgh tarekgh Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

        public Activity SetIdFormat(ActivityIdFormat format)
        {
            if (_id != null || _spanId != null)
            {
                NotifyError(new InvalidOperationException(SR.SetFormatOnStartedActivity));
            }
            else
            {
                IdFormat = format;
            }
            return this;
        }

If the Activity not started, _id and _spanId would still be nulls. otherwise Activity.Start() would fail too. no?

Copy link
Member Author

@tarekgh tarekgh Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@noahfalk I have addressed all comments. The remaining point is this one which related to IdFormat. I don't think we should fix the Ids before starting the Activity. Please let me know if you think otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

The caller is passing default context, there is no parent (Activity.Current), and the listeners didn't try to access the trace Id from the provided ActivityCreationOptions.

This case seems very real. Think of a worker app uploading a blob. There is no parent and all spans are exported so TraceId is not accessed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This case seems very real. Think of a worker app uploading a blob. There is no parent and all spans are exported so TraceId is not accessed.

Yes, agree. But using CreateActivity with the current design will make it work. right? I mean users can use CreateActivity and SetIdFormat if they need to for this scenario.

Copy link
Member Author

@tarekgh tarekgh Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@pakrym I had offline discussion with @noahfalk and we propose adding extra Boolean parameter to the CreateActivity called forceW3CId. The reason we didn't have the parameter of ActivityIdFormat type because we want to promote W3C in general and not allow users to try the hierarchal explicitly. Please let's know if you have any concern. I'll send it anyway to the fxdc and will CC you there.

        public Activity? CreateActivity(string name, 
                                        ActivityKind kind);

        public Activity? CreateActivity(
                             string name,
                             ActivityKind kind, 
                             ActivityContext parentContext,
                             IEnumerable<KeyValuePair<string, object?>>? tags = null, 
                             IEnumerable<ActivityLink>? links = null,
                             bool forceW3CId = false); // added parameter

        public Activity? CreateActivity(
                             string name, 
                             ActivityKind kind,
                             string parentContext,
                             bool forceW3CId = false,
                             IEnumerable<KeyValuePair<string, object?>>? tags = null, 
                             IEnumerable<ActivityLink>? links = null,
                             bool forceW3CId = false);     // added parameter

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. If we want to promote W3C can we default the parameter to true? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

If we want to promote W3C can we default the parameter to true? 😄

I discussed that with @noahfalk and he preferred not to default the value to W3C as he want to have consistent behavior with StartActivity when passing the exact same parameters.

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
103 changes: 57 additions & 46 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,56 +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,
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)
DateTimeOffset startTime, ActivityTagsCollection? samplerTags, ActivitySamplingResult request, bool startIt)
{
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();

if (links != null)
{
using (IEnumerator<ActivityLink> enumerator = links.GetEnumerator())
Expand Down Expand Up @@ -1091,16 +1050,68 @@ 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._previousActiveActivity = Current;
if (activity._parentId == null && activity._traceId == null && 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();

if (activity.StartTimeUtc == default)
{
activity.StartTimeUtc = GetUtcNow();
}

SetCurrent(activity);

source.NotifyActivityStart(activity);
}

return activity;
}
Expand Down
62 changes: 52 additions & 10 deletions src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,54 @@ public bool HasListeners()
/// <param name="name">The operation name of the Activity</param>
/// <param name="kind">The <see cref="ActivityKind"/></param>
/// <returns>The created <see cref="Activity"/> object or null if there is no any event listener.</returns>
/// <remarks>
/// If the Activity object is created, it will not start automatically. Callers need to call <see cref="Activity.Start()"/> to start it.
/// </remarks>
public Activity? CreateActivity(string name, ActivityKind kind)
=> CreateActivity(name, kind, default, null, null, null, default, startIt: false);

/// <summary>
/// Creates a new <see cref="Activity"/> object if there is any listener to the Activity, returns null otherwise.
/// If the Activity object is created, it will not automatically start. Callers will need to call <see cref="Activity.Start()"/> to start it.
/// </summary>
/// <param name="name">The operation name of the Activity.</param>
/// <param name="kind">The <see cref="ActivityKind"/></param>
/// <param name="parentContext">The parent <see cref="ActivityContext"/> object to initialize the created Activity object with.</param>
/// <param name="tags">The optional tags list to initialize the created Activity object with.</param>
/// <param name="links">The optional <see cref="ActivityLink"/> list to initialize the created Activity object with.</param>
/// <returns>The created <see cref="Activity"/> object or null if there is no any listener.</returns>
/// <remarks>
/// If the Activity object is created, it will not start automatically. Callers need to call <see cref="Activity.Start()"/> to start it.
/// </remarks>
public Activity? CreateActivity(string name, ActivityKind kind, ActivityContext parentContext, IEnumerable<KeyValuePair<string, object?>>? tags = null, IEnumerable<ActivityLink>? links = null)
=> CreateActivity(name, kind, parentContext, null, tags, links, default, startIt: false);

/// <summary>
/// Creates a new <see cref="Activity"/> object if there is any listener to the Activity, returns null otherwise.
/// </summary>
/// <param name="name">The operation name of the Activity.</param>
/// <param name="kind">The <see cref="ActivityKind"/></param>
/// <param name="parentId">The parent Id to initialize the created Activity object with.</param>
/// <param name="tags">The optional tags list to initialize the created Activity object with.</param>
/// <param name="links">The optional <see cref="ActivityLink"/> list to initialize the created Activity object with.</param>
/// <returns>The created <see cref="Activity"/> object or null if there is no any listener.</returns>
/// <remarks>
/// If the Activity object is created, it will not start automatically. Callers need to call <see cref="Activity.Start()"/> to start it.
/// </remarks>
public Activity? CreateActivity(string name, ActivityKind kind, string parentId, IEnumerable<KeyValuePair<string, object?>>? tags = null, IEnumerable<ActivityLink>? links = null)
=> CreateActivity(name, kind, default, parentId, tags, links, default, startIt: false);

/// <summary>
/// Creates and starts a new <see cref="Activity"/> object if there is any listener to the Activity, returns null otherwise.
/// </summary>
/// <param name="name">The operation name of the Activity</param>
/// <param name="kind">The <see cref="ActivityKind"/></param>
/// <returns>The created <see cref="Activity"/> object or null if there is no any event listener.</returns>
public Activity? StartActivity([CallerMemberName] string name = "", ActivityKind kind = ActivityKind.Internal)
=> StartActivity(name, kind, default, null, null, null, default);
=> CreateActivity(name, kind, default, null, null, null, default);

/// <summary>
/// Creates a new <see cref="Activity"/> object if there is any listener to the Activity events, returns null otherwise.
/// Creates and starts a new <see cref="Activity"/> object if there is any listener to the Activity events, returns null otherwise.
/// </summary>
/// <param name="name">The operation name of the Activity.</param>
/// <param name="kind">The <see cref="ActivityKind"/></param>
Expand All @@ -91,10 +134,10 @@ public bool HasListeners()
/// <param name="startTime">The optional start timestamp to set on the created Activity object.</param>
/// <returns>The created <see cref="Activity"/> object or null if there is no any listener.</returns>
public Activity? StartActivity(string name, ActivityKind kind, ActivityContext parentContext, IEnumerable<KeyValuePair<string, object?>>? tags = null, IEnumerable<ActivityLink>? links = null, DateTimeOffset startTime = default)
=> StartActivity(name, kind, parentContext, null, tags, links, startTime);
=> CreateActivity(name, kind, parentContext, null, tags, links, startTime);

/// <summary>
/// Creates a new <see cref="Activity"/> object if there is any listener to the Activity events, returns null otherwise.
/// Creates and starts a new <see cref="Activity"/> object if there is any listener to the Activity events, returns null otherwise.
/// </summary>
/// <param name="name">The operation name of the Activity.</param>
/// <param name="kind">The <see cref="ActivityKind"/></param>
Expand All @@ -104,10 +147,10 @@ public bool HasListeners()
/// <param name="startTime">The optional start timestamp to set on the created Activity object.</param>
/// <returns>The created <see cref="Activity"/> object or null if there is no any listener.</returns>
public Activity? StartActivity(string name, ActivityKind kind, string parentId, IEnumerable<KeyValuePair<string, object?>>? tags = null, IEnumerable<ActivityLink>? links = null, DateTimeOffset startTime = default)
=> StartActivity(name, kind, default, parentId, tags, links, startTime);
=> CreateActivity(name, kind, default, parentId, tags, links, startTime);

/// <summary>
/// Creates a new <see cref="Activity"/> object if there is any listener to the Activity events, returns null otherwise.
/// Creates and starts a new <see cref="Activity"/> object if there is any listener to the Activity events, returns null otherwise.
/// </summary>
/// <param name="kind">The <see cref="ActivityKind"/></param>
/// <param name="parentContext">The parent <see cref="ActivityContext"/> object to initialize the created Activity object with.</param>
Expand All @@ -117,9 +160,9 @@ public bool HasListeners()
/// <param name="name">The operation name of the Activity.</param>
/// <returns>The created <see cref="Activity"/> object or null if there is no any listener.</returns>
public Activity? StartActivity(ActivityKind kind, ActivityContext parentContext = default, IEnumerable<KeyValuePair<string, object?>>? tags = null, IEnumerable<ActivityLink>? links = null, DateTimeOffset startTime = default, [CallerMemberName] string name = "")
=> StartActivity(name, kind, parentContext, null, tags, links, startTime);
=> CreateActivity(name, kind, parentContext, null, tags, links, startTime);

private Activity? StartActivity(string name, ActivityKind kind, ActivityContext context, string? parentId, IEnumerable<KeyValuePair<string, object?>>? tags, IEnumerable<ActivityLink>? links, DateTimeOffset startTime)
private Activity? CreateActivity(string name, ActivityKind kind, ActivityContext context, string? parentId, IEnumerable<KeyValuePair<string, object?>>? tags, IEnumerable<ActivityLink>? links, DateTimeOffset startTime, bool startIt = true)
{
// _listeners can get assigned to null in Dispose.
SynchronizedList<ActivityListener>? listeners = _listeners;
Expand Down Expand Up @@ -218,8 +261,7 @@ public bool HasListeners()

if (samplingResult != ActivitySamplingResult.None)
{
activity = Activity.CreateAndStart(this, name, kind, parentId, context, tags, links, startTime, samplerTags, samplingResult);
listeners.EnumWithAction((listener, obj) => listener.ActivityStarted?.Invoke((Activity) obj), activity);
activity = Activity.Create(this, name, kind, parentId, context, tags, links, startTime, samplerTags, samplingResult, startIt);
}

return activity;
Expand Down
Loading