Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public string? Id
public string? TraceStateString { get { throw null; } set { } }
public System.Diagnostics.Activity AddBaggage(string key, string? value) { throw null; }
public System.Diagnostics.Activity AddEvent(System.Diagnostics.ActivityEvent e) { throw null; }
public System.Diagnostics.Activity AddException(System.Exception exception, System.Diagnostics.TagList tags = default, System.DateTimeOffset timestamp = default) { throw null; }
public System.Diagnostics.Activity AddLink(System.Diagnostics.ActivityLink link) { throw null; }
public System.Diagnostics.Activity AddTag(string key, string? value) { throw null; }
public System.Diagnostics.Activity AddTag(string key, object? value) { throw null; }
Expand Down Expand Up @@ -276,11 +277,13 @@ public readonly struct ActivityCreationOptions<T>
public string? TraceState { get { throw null; } init { throw null; } }
}
public delegate System.Diagnostics.ActivitySamplingResult SampleActivity<T>(ref System.Diagnostics.ActivityCreationOptions<T> options);
public delegate void ExceptionRecorder(System.Diagnostics.Activity activity, System.Exception exception, ref System.Diagnostics.TagList tags);
public sealed class ActivityListener : IDisposable
{
public ActivityListener() { throw null; }
public System.Action<System.Diagnostics.Activity>? ActivityStarted { get { throw null; } set { throw null; } }
public System.Action<System.Diagnostics.Activity>? ActivityStopped { get { throw null; } set { throw null; } }
public System.Diagnostics.ExceptionRecorder? ExceptionRecorder { get { throw null; } set { throw null; } }
public System.Func<System.Diagnostics.ActivitySource, bool>? ShouldListenTo { get { throw null; } set { throw null; } }
public System.Diagnostics.SampleActivity<string>? SampleUsingParentId { get { throw null; } set { throw null; } }
public System.Diagnostics.SampleActivity<ActivityContext>? Sample { get { throw null; } set { throw null; } }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,71 @@ public Activity AddEvent(ActivityEvent e)
return this;
}

/// <summary>
/// Add a <see cref="ActivityEvent" /> object containing the exception information to the <see cref="Events" /> list.
/// </summary>
/// <param name="exception">The exception to add to the attached events list.</param>
/// <param name="tags">The tags to add to the exception event.</param>
/// <param name="timestamp">The timestamp to add to the exception event.</param>
/// <returns><see langword="this" /> for convenient chaining.</returns>
/// <remarks>
/// - The name of the event will be "exception", and it will include the tags "exception.message", "exception.stacktrace", and "exception.type", in addition to the tags provided in the <paramref name="tags"/> parameter.
/// - Any registered <see cref="ActivityListener"/> with the <see cref="ActivityListener.ExceptionRecorder"/> callback will be notified about this exception addition before the <see cref="ActivityEvent" /> object is added to the <see cref="Events" /> list.
/// </remarks>
public Activity AddException(Exception exception, TagList tags = default, DateTimeOffset timestamp = default)
Copy link
Contributor

Choose a reason for hiding this comment

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

@tarekgh Sorry just noticed this but should we use in TagList tags here? It is kind of big struct that might save some copy cycles.

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 raised the question in #53641 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't be too worried either way. Presumably to get to this point the dev already threw and caught an Exception costing them 10s of microseconds. Spending an extra 10? nanoseconds copying a struct seems neglible.

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 already opened a PR #103009 for it.

The other point is we always pass TagList by ref in other APIs. Consistency is good beside getting minor perf is not bad.

{
if (exception == null)
{
throw new ArgumentNullException(nameof(exception));
}

TagList exceptionTags = tags;

Source.NotifyActivityAddException(this, exception, ref exceptionTags);

const string exceptionEventName = "exception";
const string exceptionMessageTag = "exception.message";
const string exceptionStackTraceTag = "exception.stacktrace";
const string exceptionTypeTag = "exception.type";

bool hasMessage = false;
bool hasStackTrace = false;
bool hasType = false;

for (int i = 0; i < exceptionTags.Count; i++)
{
if (exceptionTags[i].Key == exceptionMessageTag)
{
hasMessage = true;
}
else if (exceptionTags[i].Key == exceptionStackTraceTag)
{
hasStackTrace = true;
}
else if (exceptionTags[i].Key == exceptionTypeTag)
{
hasType = true;
}
}

if (!hasMessage)
{
exceptionTags.Add(new KeyValuePair<string, object?>(exceptionMessageTag, exception.Message));
}

if (!hasStackTrace)
{
exceptionTags.Add(new KeyValuePair<string, object?>(exceptionStackTraceTag, exception.ToString()));
}

if (!hasType)
{
exceptionTags.Add(new KeyValuePair<string, object?>(exceptionTypeTag, exception.GetType().ToString()));
}

return AddEvent(new ActivityEvent(exceptionEventName, timestamp, ref exceptionTags));
}

/// <summary>
/// Add an <see cref="ActivityLink"/> to the <see cref="Links"/> list.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,16 @@ public ActivityEvent(string name) : this(name, DateTimeOffset.UtcNow, tags: null
/// <param name="name">Event name.</param>
/// <param name="timestamp">Event timestamp. Timestamp MUST only be used for the events that happened in the past, not at the moment of this call.</param>
/// <param name="tags">Event Tags.</param>
public ActivityEvent(string name, DateTimeOffset timestamp = default, ActivityTagsCollection? tags = null)
public ActivityEvent(string name, DateTimeOffset timestamp = default, ActivityTagsCollection? tags = null) : this(name, timestamp, tags, tags is null ? 0 : tags.Count) { }

internal ActivityEvent(string name, DateTimeOffset timestamp, ref TagList tags) : this(name, timestamp, tags, tags.Count) { }

private ActivityEvent(string name, DateTimeOffset timestamp, IEnumerable<KeyValuePair<string, object?>>? tags, int tagsCount)
{
Name = name ?? string.Empty;
Timestamp = timestamp != default ? timestamp : DateTimeOffset.UtcNow;

_tags = tags?.Count > 0 ? new Activity.TagsLinkedList(tags) : null;
_tags = tagsCount > 0 ? new Activity.TagsLinkedList(tags!) : null;
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ namespace System.Diagnostics
/// </summary>
public delegate ActivitySamplingResult SampleActivity<T>(ref ActivityCreationOptions<T> options);

/// <summary>
/// Define the callback to be used in <see cref="ActivityListener"/> to receive notifications when exceptions are added to the <see cref="Activity"/>.
/// </summary>
public delegate void ExceptionRecorder(Activity activity, Exception exception, ref TagList tags);

/// <summary>
/// ActivityListener allows listening to the start and stop Activity events and give the opportunity to decide creating the Activity for sampling scenarios.
/// </summary>
Expand All @@ -32,6 +37,11 @@ public ActivityListener()
/// </summary>
public Action<Activity>? ActivityStopped { get; set; }

/// <summary>
/// Set or get the callback used to listen to <see cref="Activity"/> events when exceptions are added.
/// </summary>
public ExceptionRecorder? ExceptionRecorder { get; set; }

/// <summary>
/// Set or get the callback used to decide if want to listen to <see cref="Activity"/> objects events which created using <see cref="ActivitySource"/> object.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,18 @@ internal void NotifyActivityStop(Activity activity)
listeners.EnumWithAction((listener, obj) => listener.ActivityStopped?.Invoke((Activity)obj), activity);
}
}

internal void NotifyActivityAddException(Activity activity, Exception exception, ref TagList tags)
{
Debug.Assert(activity != null);

// _listeners can get assigned to null in Dispose.
SynchronizedList<ActivityListener>? listeners = _listeners;
if (listeners != null && listeners.Count > 0)
{
listeners.EnumWithExceptionNotification(activity, exception, ref tags);
}
}
}

// SynchronizedList<T> is a helper collection which ensure thread safety on the collection
Expand Down Expand Up @@ -498,5 +510,36 @@ public void EnumWithAction(Action<T, object> action, object arg)
}
}

public void EnumWithExceptionNotification(Activity activity, Exception exception, ref TagList tags)
{
if (typeof(T) != typeof(ActivityListener))
{
return;
}

uint version = _version;
int index = 0;

while (index < _list.Count)
{
T item;
lock (_list)
{
if (version != _version)
{
version = _version;
index = 0;
continue;
}

item = _list[index];
index++;
}

// Important to notify outside the lock.
// This is the whole point we are having this wrapper class.
(item as ActivityListener)!.ExceptionRecorder?.Invoke(activity, exception, ref tags);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1626,6 +1626,102 @@ public void AddLinkTest()
Assert.Equal(99, tag.Value);
}

[Fact]
public void AddExceptionTest()
{
using ActivitySource aSource = new ActivitySource("AddExceptionTest");

ActivityListener listener = new ActivityListener();
listener.ShouldListenTo = (activitySource) => object.ReferenceEquals(activitySource, aSource);
listener.Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllData;
ActivitySource.AddActivityListener(listener);

using Activity? activity = aSource.StartActivity("Activity1");
Assert.NotNull(activity);
Assert.Empty(activity.Events);

const string exceptionEventName = "exception";
const string exceptionMessageTag = "exception.message";
const string exceptionStackTraceTag = "exception.stacktrace";
const string exceptionTypeTag = "exception.type";

Exception exception = new ArgumentOutOfRangeException("Some message");
activity.AddException(exception);
List<ActivityEvent> events = activity.Events.ToList();
Assert.Equal(1, events.Count);
Assert.Equal(exceptionEventName, events[0].Name);
Assert.Equal(new TagList { { exceptionMessageTag, exception.Message}, { exceptionStackTraceTag, exception.ToString()}, { exceptionTypeTag, exception.GetType().ToString() } }, events[0].Tags);

try { throw new InvalidOperationException("Some other message"); } catch (Exception e) { exception = e; }
activity.AddException(exception);
events = activity.Events.ToList();
Assert.Equal(2, events.Count);
Assert.Equal(exceptionEventName, events[1].Name);
Assert.Equal(new TagList { { exceptionMessageTag, exception.Message}, { exceptionStackTraceTag, exception.ToString()}, { exceptionTypeTag, exception.GetType().ToString() } }, events[1].Tags);

listener.ExceptionRecorder = (Activity activity, Exception exception, ref TagList theTags) => theTags.Add("foo", "bar");
activity.AddException(exception, new TagList { { "hello", "world" } });
events = activity.Events.ToList();
Assert.Equal(3, events.Count);
Assert.Equal(exceptionEventName, events[2].Name);
Assert.Equal(new TagList
{
{ "hello", "world" },
{ "foo", "bar" },
{ exceptionMessageTag, exception.Message },
{ exceptionStackTraceTag, exception.ToString() },
{ exceptionTypeTag, exception.GetType().ToString() }
},
events[2].Tags);

listener.ExceptionRecorder = (Activity activity, Exception exception, ref TagList theTags) =>
{
theTags.Add("exception.escaped", "true");
theTags.Add("exception.message", "Overridden message");
theTags.Add("exception.stacktrace", "Overridden stacktrace");
theTags.Add("exception.type", "Overridden type");
};
activity.AddException(exception, new TagList { { "hello", "world" } });
events = activity.Events.ToList();
Assert.Equal(4, events.Count);
Assert.Equal(exceptionEventName, events[3].Name);
Assert.Equal(new TagList
{
{ "hello", "world" },
{ "exception.escaped", "true" },
{ "exception.message", "Overridden message" },
{ "exception.stacktrace", "Overridden stacktrace" },
{ "exception.type", "Overridden type" }
},
events[3].Tags);

ActivityListener listener1 = new ActivityListener();
listener1.ShouldListenTo = (activitySource) => object.ReferenceEquals(activitySource, aSource);
listener1.Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllData;
ActivitySource.AddActivityListener(listener1);
listener1.ExceptionRecorder = (Activity activity, Exception exception, ref TagList theTags) =>
{
theTags.Remove(new KeyValuePair<string, object?>("exception.message", "Overridden message"));
theTags.Remove(new KeyValuePair<string, object?>("exception.stacktrace", "Overridden stacktrace"));
theTags.Remove(new KeyValuePair<string, object?>("exception.type", "Overridden type"));
theTags.Add("secondListener", "win");
};
activity.AddException(exception, new TagList { { "hello", "world" } });
events = activity.Events.ToList();
Assert.Equal(5, events.Count);
Assert.Equal(exceptionEventName, events[4].Name);
Assert.Equal(new TagList
{
{ "hello", "world" },
{ "exception.escaped", "true" },
{ "secondListener", "win" },
{ "exception.message", exception.Message },
{ "exception.stacktrace", exception.ToString() },
{ "exception.type", exception.GetType().ToString() },
},
events[4].Tags);
}

[Fact]
public void TestIsAllDataRequested()
{
Expand Down