Activity should have AddExceptionMethod

[DR9885]

Edited by @tarekgh

Proposal

This proposal aims to enable the recording of exceptions during tracing operations, in accordance with the OpenTelemetry Record Exception specification

namespace System.Diagnostics;

public class Activity
{
    public void RecordException(Exception exception, TagList tags = default, DateTimeOffset timestamp = default);
}

public delegate void ExceptionRecording(Activity activity, Exception exception, ref TagList tags);

public sealed class ActivityListener : IDisposable
{
    // Will be invoked when Activity.RecordException is called.
    public ExceptionRecording? ExceptionRecording { get; set; }
}

---

Notes

• RecordException will save details of the exception into an ActivityEvent entry, adhering to the guidelines of the OpenTelemetry semantic convention.
• RecordException captures information such as the stack trace, exception type name, and exception message, but not the exception object itself. This approach is chosen because the exception might be rethrown, potentially altering the stack trace. Additionally, exception objects can be large and complex, leading to performance issues.
• Anyone interested in accessing the actual exception object during recording can use ActivityListener.ExceptionRecording, which provides a notification that includes the exception objects along with the tags to be recorded. When listeners are enabled, the behavior of RecordException will be as follows:
  • RecordException will iterate through all non-nullable ExceptionRecording listeners one by one, passing the exception object and the input tags list provided to RecordException. Listeners can add or modify tags as necessary.
  • If multiple registered listeners modify the same tags, the changes made by the last listener will prevail and be recorded in the event.
  • After calling all listeners, RecordException adds the populated event to the Activity.Events list. If any tags specified in the OpenTelemetry semantic convention are missing, they will be added to the tag list before the event is stored.
• RecordException will always log the event, and there will be no option to suppress its addition.

The recorded event will have the name as exception and will include the following tags:

• exception.message, with the value of Exception.Message.
• exception.stacktrace with the value of Exception.ToString
• exception.type with the value of exceptionObject.GetType()

---

Usage Example

    using ActivitySource aSource =  new ActivitySource("SourceActivityListener");
    
    using ActivityListener listener = new ActivityListener();
    listener.ShouldListenTo = (activitySource) => object.ReferenceEquals(aSource, activitySource);
    listener.Sample = (ref ActivityCreationOptions<ActivityContext> activityOptions) => ActivitySamplingResult.AllData;
    listener.ExceptionRecording = (activity, exception, ref tags) => { /* access the exception and the tags */  };
    ActivitySource.AddActivityListener(listener);

    using Activity activity = aSource.StartActivity("AllDataRequestedActivity");
    activity.RecordException(new Exception("Exception to be recorded"), new TagList { { "exception.escaped", "true" } }, DateTimeOffset.UtcNow );
    
    ActivityEvent exceptionEvent = activity.Events.First();

Original Proposal

Activity should have AddExceptionMethod, rather than having to set these tags everytime.

        public static Activity AddException(this Activity activity, Exception ex)
        {
            return activity?.AddTag("event", "error")
                .AddTag("error", "true")
                .AddTag("error.kind", ex.GetType().Name)
                .AddTag("error.message", ex.Message)
                .AddTag("error.stack", ex.StackTrace);
        }

This should also have constants for these strings, for performance.

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Activity should have AddExceptionMethod, rather than having to set these tags everytime.

        public static Activity AddException(this Activity activity, Exception ex)
        {
            return activity?.AddTag("event", "error")
                .AddTag("error", "true")
                .AddTag("error.kind", ex.GetType().Name)
                .AddTag("error.message", ex.Message)
                .AddTag("error.stack", ex.StackTrace);
        }

This should also have constants for these strings, for performance.

Author:	DR9885
Assignees:	-
Labels:	area-System.Diagnostics.Tracing, untriaged
Milestone:	-

Tagging subscribers to this area: @tarekgh
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Activity should have AddExceptionMethod, rather than having to set these tags everytime.

        public static Activity AddException(this Activity activity, Exception ex)
        {
            return activity?.AddTag("event", "error")
                .AddTag("error", "true")
                .AddTag("error.kind", ex.GetType().Name)
                .AddTag("error.message", ex.Message)
                .AddTag("error.stack", ex.StackTrace);
        }

This should also have constants for these strings, for performance.

Author:	DR9885
Assignees:	-
Labels:	area-System.Diagnostics.Activity
Milestone:	-

[tarekgh]

Why can't use Activity.SetCustomProperty?

Something like:

activity.SetCustomProperty("exception", exceptionObject);

In general, for error reporting we followed up OpenTelemetry specs and provided Activity.SetStatus. So, you can also do:

activity.SetStatus(ActivityStatusCode.Error, exceptionObject.ToString());

I don't think adding a method for exception will be desirable for most scenarios as wouldn't be portable.

CC @noahfalk

[noahfalk]

There is a semantic convention for exceptions but it is currently marked experimental. I do not recommend we codify it into the BCL until it is more stable, but once it was stable AddException() seems like a useful API.

[DR9885]

Theres also a few tags listed on microsoft docs:

otel.status_code
otel.status_description

https://docs.microsoft.com/en-us/dotnet/core/diagnostics/distributed-tracing-instrumentation-walkthroughs#optional-add-status

... However ... I Just realized this is done by SetStatus, so maybe redundant to an earlier comment

Based on all comments though, It might be better if SetStatus did all of this:

  public static Activity SetStatus(this Activity activity, Status status, Exception ex = null)
        {
            return activity?.SetTag("event", "error")
                .SetTag("otel.status_code", status.ToString())
                .SetTag("otel.status_description", ex.Message)
                .SetTag("error", "true")
                .SetTag("error.kind", ex.GetType().Name)
                .SetTag("error.message", ex.Message)
                .SetTag("error.stack", ex.StackTrace);
        }

[CodeBlanch]

@DR9885 Just to clarify this a bit, the OTel spec doesn't say to add tags for an exception, it says to add an event (with tags). Here is the extension method we have in the OTel.NET library for this: https://github.com/open-telemetry/opentelemetry-dotnet/blob/4c22707a98abf356fa4224ebbc872f3bad0d72c6/src/OpenTelemetry.Api/Trace/ActivityExtensions.cs#L95

But yes it would be great to have this API on activity so that users don't need the OpenTelemetry.Api dependency.

I was thinking the .NET API would be something simple like...

class Activity
{
   public Exception? Exception { get; set; }
}

...or...

class Activity
{
   public Exception? Exception { get; }

   public void RecordException(Exception exception) { ... }
}

And then the OpenTelemetry library (or anyone interested) could worry about what to do with it.

/cc @cijothomas

[cijothomas]

Related issue : open-telemetry/opentelemetry-dotnet-contrib#1794
(There are privacy concerns about storing stack as activity event)