[Instrumentation.GrpcCore] Record exceptions as activity events flag (#…

…1648) Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com> Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com>
open-telemetry · May 17, 2024 · 30f5fd1 · 30f5fd1
1 parent c6010d7
commit 30f5fd1
Show file tree

Hide file tree

Showing 10 changed files with 102 additions and 29 deletions.
diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/.publicApi/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Instrumentation.GrpcCore/.publicApi/PublicAPI.Unshipped.txt
@@ -3,12 +3,16 @@ OpenTelemetry.Instrumentation.GrpcCore.ClientTracingInterceptor.ClientTracingInt
 OpenTelemetry.Instrumentation.GrpcCore.ClientTracingInterceptorOptions
 OpenTelemetry.Instrumentation.GrpcCore.ClientTracingInterceptorOptions.ClientTracingInterceptorOptions() -> void
 OpenTelemetry.Instrumentation.GrpcCore.ClientTracingInterceptorOptions.Propagator.get -> OpenTelemetry.Context.Propagation.TextMapPropagator
+OpenTelemetry.Instrumentation.GrpcCore.ClientTracingInterceptorOptions.RecordException.get -> bool
+OpenTelemetry.Instrumentation.GrpcCore.ClientTracingInterceptorOptions.RecordException.set -> void
 OpenTelemetry.Instrumentation.GrpcCore.ClientTracingInterceptorOptions.RecordMessageEvents.get -> bool
 OpenTelemetry.Instrumentation.GrpcCore.ClientTracingInterceptorOptions.RecordMessageEvents.set -> void
 OpenTelemetry.Instrumentation.GrpcCore.ServerTracingInterceptor
 OpenTelemetry.Instrumentation.GrpcCore.ServerTracingInterceptor.ServerTracingInterceptor(OpenTelemetry.Instrumentation.GrpcCore.ServerTracingInterceptorOptions options) -> void
 OpenTelemetry.Instrumentation.GrpcCore.ServerTracingInterceptorOptions
 OpenTelemetry.Instrumentation.GrpcCore.ServerTracingInterceptorOptions.Propagator.get -> OpenTelemetry.Context.Propagation.TextMapPropagator
+OpenTelemetry.Instrumentation.GrpcCore.ServerTracingInterceptorOptions.RecordException.get -> bool
+OpenTelemetry.Instrumentation.GrpcCore.ServerTracingInterceptorOptions.RecordException.set -> void
 OpenTelemetry.Instrumentation.GrpcCore.ServerTracingInterceptorOptions.RecordMessageEvents.get -> bool
 OpenTelemetry.Instrumentation.GrpcCore.ServerTracingInterceptorOptions.RecordMessageEvents.set -> void
 OpenTelemetry.Instrumentation.GrpcCore.ServerTracingInterceptorOptions.ServerTracingInterceptorOptions() -> void

diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.GrpcCore/CHANGELOG.md
@@ -10,6 +10,8 @@
   ([#1624](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1624))
 * Update `OpenTelemetry.Api` to `1.8.1`.
   ([#1668](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1668))
+* `RecordException` option for both client and server interceptors.
+  ([#1648](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1648))
 
 ## 1.0.0-beta.5
 

diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptor.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptor.cs
@@ -298,7 +298,7 @@ private sealed class ClientRpcScope<TRequest, TResponse> : RpcScope<TRequest, TR
         /// <param name="context">The context.</param>
         /// <param name="options">The options.</param>
         public ClientRpcScope(ClientInterceptorContext<TRequest, TResponse> context, ClientTracingInterceptorOptions options)
-            : base(context.Method?.FullName, options.RecordMessageEvents)
+            : base(context.Method?.FullName, options.RecordMessageEvents, options.RecordException)
         {
             this.context = context;
 

diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptorOptions.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptorOptions.cs
@@ -16,6 +16,11 @@ public class ClientTracingInterceptorOptions
     /// </summary>
     public bool RecordMessageEvents { get; set; }
 
+    /// <summary>
+    /// Gets or sets a value indicating whether the exception will be recorded as ActivityEvent or not.
+    /// </summary>
+    public bool RecordException { get; set; }
+
     /// <summary>
     /// Gets the propagator.
     /// </summary>

diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/RpcScope.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/RpcScope.cs
@@ -25,6 +25,11 @@ internal abstract class RpcScope<TRequest, TResponse> : IDisposable
     /// </summary>
     private readonly bool recordMessageEvents;
 
+    /// <summary>
+    /// The record exception as ActivityEvent flag.
+    /// </summary>
+    private readonly bool recordException;
+
     /// <summary>
     /// The RPC activity.
     /// </summary>
@@ -50,10 +55,12 @@ internal abstract class RpcScope<TRequest, TResponse> : IDisposable
     /// </summary>
     /// <param name="fullServiceName">Full name of the service.</param>
     /// <param name="recordMessageEvents">if set to <c>true</c> [record message events].</param>
-    protected RpcScope(string fullServiceName, bool recordMessageEvents)
+    /// <param name="recordException">If set to <c>true</c> [record exception].</param>
+    protected RpcScope(string fullServiceName, bool recordMessageEvents, bool recordException)
     {
         this.FullServiceName = fullServiceName?.TrimStart('/') ?? "unknownservice/unknownmethod";
         this.recordMessageEvents = recordMessageEvents;
+        this.recordException = recordException;
     }
 
     /// <summary>
@@ -118,16 +125,7 @@ public void CompleteWithException(Exception exception)
             return;
         }
 
-        var grpcStatusCode = Grpc.Core.StatusCode.Unknown;
-        var description = exception.Message;
-
-        if (exception is RpcException rpcException)
-        {
-            grpcStatusCode = rpcException.StatusCode;
-            description = rpcException.Message;
-        }
-
-        this.StopActivity((int)grpcStatusCode, description);
+        this.StopActivity(exception);
     }
 
     /// <inheritdoc/>
@@ -176,19 +174,58 @@ protected void SetActivity(Activity activity)
     /// Stops the activity.
     /// </summary>
     /// <param name="statusCode">The status code.</param>
-    /// <param name="statusDescription">The description, if any.</param>
-    private void StopActivity(int statusCode, string statusDescription = null)
+    /// <param name="markAsCompleted">If set to <c>true</c> [mark as completed].</param>
+    private void StopActivity(int statusCode, bool markAsCompleted = true)
+    {
+        if (markAsCompleted && !this.TryMarkAsCompleted())
+        {
+            return;
+        }
+
+        this.activity.SetTag(SemanticConventions.AttributeRpcGrpcStatusCode, statusCode);
+        this.activity.Stop();
+    }
+
+    /// <summary>
+    /// Stops the activity.
+    /// </summary>
+    /// <param name="exception">The exception.</param>
+    private void StopActivity(Exception exception)
     {
-        if (Interlocked.CompareExchange(ref this.complete, 1, 0) == 0)
+        if (!this.TryMarkAsCompleted())
+        {
+            return;
+        }
+
+        var grpcStatusCode = Grpc.Core.StatusCode.Unknown;
+        var description = exception.Message;
+
+        if (exception is RpcException rpcException)
+        {
+            grpcStatusCode = rpcException.StatusCode;
+            description = rpcException.Message;
+        }
+
+        if (!string.IsNullOrEmpty(description))
         {
-            this.activity.SetTag(SemanticConventions.AttributeRpcGrpcStatusCode, statusCode);
-            if (statusDescription != null)
-            {
-                this.activity.SetStatus(Trace.Status.Error.WithDescription(statusDescription));
-            }
+            this.activity.SetStatus(Trace.Status.Error.WithDescription(description));
+        }
 
-            this.activity.Stop();
+        if (this.activity.IsAllDataRequested && this.recordException)
+        {
+            this.activity.RecordException(exception);
         }
+
+        this.StopActivity((int)grpcStatusCode, markAsCompleted: false);
+    }
+
+    /// <summary>
+    /// Tries to mark <see cref="RpcScope{TRequest, TResponse}"/> as completed.
+    /// </summary>
+    /// <returns>Returns <c>true</c> if marked as completed successfully.</returns>
+    private bool TryMarkAsCompleted()
+    {
+        return Interlocked.CompareExchange(ref this.complete, 1, 0) == 0;
     }
 
     /// <summary>

diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/SemanticConventions.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/SemanticConventions.cs
@@ -22,5 +22,11 @@ internal static class SemanticConventions
 
     // Used for unit testing only.
     internal const string AttributeActivityIdentifier = "activityidentifier";
+
+    // https://github.com/open-telemetry/semantic-conventions/blob/main/docs/exceptions/exceptions-spans.md
+    internal const string AttributeExceptionEventName = "exception";
+    internal const string AttributeExceptionType = "exception.type";
+    internal const string AttributeExceptionMessage = "exception.message";
+    internal const string AttributeExceptionStacktrace = "exception.stacktrace";
 #pragma warning restore SA1600 // Elements should be documented
 }
diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptor.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptor.cs
@@ -180,7 +180,7 @@ private class ServerRpcScope<TRequest, TResponse> : RpcScope<TRequest, TResponse
         /// <param name="context">The context.</param>
         /// <param name="options">The options.</param>
         public ServerRpcScope(ServerCallContext context, ServerTracingInterceptorOptions options)
-            : base(context.Method, options.RecordMessageEvents)
+            : base(context.Method, options.RecordMessageEvents, options.RecordException)
         {
             if (!GrpcCoreInstrumentation.ActivitySource.HasListeners())
             {

diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptorOptions.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptorOptions.cs
@@ -16,6 +16,11 @@ public class ServerTracingInterceptorOptions
     /// </summary>
     public bool RecordMessageEvents { get; set; }
 
+    /// <summary>
+    /// Gets or sets a value indicating whether the exception will be recorded as ActivityEvent or not.
+    /// </summary>
+    public bool RecordException { get; set; }
+
     /// <summary>
     /// Gets the propagator.
     /// </summary>

diff --git a/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreClientInterceptorTests.cs b/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreClientInterceptorTests.cs
@@ -306,10 +306,12 @@ static void ValidateNewTagOnActivity(InterceptorActivityListener listener)
     /// <param name="activity">The activity.</param>
     /// <param name="expectedStatusCode">The expected status code.</param>
     /// <param name="recordedMessages">if set to <c>true</c> [recorded messages].</param>
+    /// <param name="recordedExceptions">if set to <c>true</c> [recorded exceptions].</param>
     internal static void ValidateCommonActivityTags(
         Activity activity,
         StatusCode expectedStatusCode = StatusCode.OK,
-        bool recordedMessages = false)
+        bool recordedMessages = false,
+        bool recordedExceptions = false)
     {
         Assert.NotNull(activity);
         Assert.NotNull(activity.Tags);
@@ -355,6 +357,18 @@ static void ValidateCommonEventAttributes(ActivityEvent activityEvent)
             Assert.Contains(responseMessage.Tags, t => t.Key == SemanticConventions.AttributeMessageType && (string)t.Value == "RECEIVED");
             Assert.Contains(requestMessage.Tags, t => t.Key == SemanticConventions.AttributeMessageCompressedSize && (int)t.Value == FoobarService.DefaultResponseMessageSize);
         }
+
+        if (recordedExceptions)
+        {
+            Assert.NotNull(activity.Events);
+            Assert.Single(activity.Events, e => e.Name == SemanticConventions.AttributeExceptionEventName);
+
+            var exceptionEvent = activity.Events.First(e => e.Name == SemanticConventions.AttributeExceptionEventName);
+            Assert.NotNull(exceptionEvent.Tags);
+            Assert.Contains(exceptionEvent.Tags, t => t.Key == SemanticConventions.AttributeExceptionType && (string)t.Value == typeof(RpcException).FullName);
+            Assert.Contains(exceptionEvent.Tags, t => t.Key == SemanticConventions.AttributeExceptionMessage);
+            Assert.Contains(exceptionEvent.Tags, t => t.Key == SemanticConventions.AttributeExceptionStacktrace);
+        }
     }
 
     /// <summary>
@@ -472,21 +486,21 @@ private static async Task TestHandlerFailure(
         string serverUriString = null)
     {
         using var server = FoobarService.Start();
-        var clientInterceptorOptions = new ClientTracingInterceptorOptions { Propagator = new TraceContextPropagator(), ActivityIdentifierValue = Guid.NewGuid() };
+        var interceptorOptions = new ClientTracingInterceptorOptions { Propagator = new TraceContextPropagator(), ActivityIdentifierValue = Guid.NewGuid(), RecordException = true };
         var client = FoobarService.ConstructRpcClient(
             serverUriString ?? server.UriString,
-            new ClientTracingInterceptor(clientInterceptorOptions),
+            new ClientTracingInterceptor(interceptorOptions),
             new List<Metadata.Entry>
             {
                 new(FoobarService.RequestHeaderFailWithStatusCode, statusCode.ToString()),
                 new(FoobarService.RequestHeaderErrorDescription, "fubar"),
             });
 
-        using var activityListener = new InterceptorActivityListener(clientInterceptorOptions.ActivityIdentifierValue);
+        using var activityListener = new InterceptorActivityListener(interceptorOptions.ActivityIdentifierValue);
         await Assert.ThrowsAsync<RpcException>(async () => await clientRequestFunc(client, null).ConfigureAwait(false));
 
         var activity = activityListener.Activity;
-        ValidateCommonActivityTags(activity, statusCode, false);
+        ValidateCommonActivityTags(activity, statusCode, interceptorOptions.RecordMessageEvents, interceptorOptions.RecordException);
 
         if (validateErrorDescription)
         {

diff --git a/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreServerInterceptorTests.cs b/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreServerInterceptorTests.cs
@@ -145,7 +145,7 @@ private static async Task TestHandlerSuccess(Func<Foobar.FoobarClient, Metadata,
     private static async Task TestHandlerFailure(Func<Foobar.FoobarClient, Metadata, Task> clientRequestFunc, Metadata additionalMetadata = null)
     {
         // starts the server with the server interceptor
-        var interceptorOptions = new ServerTracingInterceptorOptions { Propagator = new TraceContextPropagator(), ActivityIdentifierValue = Guid.NewGuid() };
+        var interceptorOptions = new ServerTracingInterceptorOptions { Propagator = new TraceContextPropagator(), ActivityIdentifierValue = Guid.NewGuid(), RecordException = true };
         using var server = FoobarService.Start(new ServerTracingInterceptor(interceptorOptions));
 
         using var activityListener = new InterceptorActivityListener(interceptorOptions.ActivityIdentifierValue);
@@ -161,7 +161,7 @@ private static async Task TestHandlerFailure(Func<Foobar.FoobarClient, Metadata,
         await Assert.ThrowsAsync<RpcException>(async () => await clientRequestFunc(client, additionalMetadata).ConfigureAwait(false));
 
         var activity = activityListener.Activity;
-        GrpcCoreClientInterceptorTests.ValidateCommonActivityTags(activity, StatusCode.ResourceExhausted, interceptorOptions.RecordMessageEvents);
+        GrpcCoreClientInterceptorTests.ValidateCommonActivityTags(activity, StatusCode.ResourceExhausted, interceptorOptions.RecordMessageEvents, interceptorOptions.RecordException);
         Assert.Equal(FoobarService.DefaultParentFromTraceparentHeader.SpanId, activity.ParentSpanId);
     }
 }