RecordException in SqlClient instrumentation (#1592)

* RecordException in SqlClient instrumentation * Add RecordException to public API. * RecordException tests for SqlEventSource * Tests for exceptions in SqlEventSource. Support different fully-qualified exception types for AdoNet and MDS sources. * More unit-tests * CHANGELOG updates * Cleanup tests to use semantic conventions * Make RecordException netcore only * RecordException removed from netfx public api Also updated readme and changelog Co-authored-by: Cijo Thomas <[email protected]>
open-telemetry · Dec 7, 2020 · 4808e05 · 4808e05
1 parent 1fb7695
commit 4808e05
Show file tree

Hide file tree

Showing 10 changed files with 71 additions and 6 deletions.
diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/net452/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/net452/PublicAPI.Unshipped.txt
@@ -7,4 +7,4 @@ OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetState
 OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetStatementText.set -> void
 OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SqlClientInstrumentationOptions() -> void
 OpenTelemetry.Trace.TracerProviderBuilderExtensions
-static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddSqlClientInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions> configureSqlClientInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder
+static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddSqlClientInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions> configureSqlClientInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder
diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/net461/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/net461/PublicAPI.Unshipped.txt
@@ -7,4 +7,4 @@ OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetState
 OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetStatementText.set -> void
 OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SqlClientInstrumentationOptions() -> void
 OpenTelemetry.Trace.TracerProviderBuilderExtensions
-static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddSqlClientInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions> configureSqlClientInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder
+static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddSqlClientInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions> configureSqlClientInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder
diff --git a/...OpenTelemetry.Instrumentation.SqlClient/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/...OpenTelemetry.Instrumentation.SqlClient/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt
@@ -3,6 +3,8 @@ OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.EnableCo
 OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.EnableConnectionLevelAttributes.set -> void
 OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.Enrich.get -> System.Action<System.Diagnostics.Activity, string, object>
 OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.Enrich.set -> void
+OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.RecordException.get -> bool
+OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.RecordException.set -> void
 OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetStoredProcedureCommandName.get -> bool
 OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetStoredProcedureCommandName.set -> void
 OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetTextCommandContent.get -> bool

diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md
@@ -10,6 +10,10 @@
   Framework they are replaced by a single `SetStatementText` property.
 * On .NET Framework, "db.statement_type" attribute is no longer set for
   activities created by the instrumentation.
+* New setting on SqlClientInstrumentationOptions on .NET Core: `RecordException`
+  can be set to instruct the instrumentation to record SqlExceptions as Activity
+  [events](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md).
+  ([#1592](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1592))
 
 ## 1.0.0-rc1.1
 

diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs
@@ -176,6 +176,11 @@ public override void OnCustom(string name, Activity activity, object payload)
                                 if (this.exceptionFetcher.TryFetch(payload, out Exception exception) && exception != null)
                                 {
                                     activity.SetStatus(Status.Error.WithDescription(exception.Message));
+
+                                    if (this.options.RecordException)
+                                    {
+                                        activity.RecordException(exception);
+                                    }
                                 }
                                 else
                                 {

diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs
@@ -183,7 +183,8 @@ private void OnEndExecute(EventWrittenEventArgs eventData)
                     }
                     else if ((compositeState & 0b010) == 0b010)
                     {
-                        activity.SetStatus(Status.Error.WithDescription($"SqlExceptionNumber {eventData.Payload[2]} thrown."));
+                        var errorText = $"SqlExceptionNumber {eventData.Payload[2]} thrown.";
+                        activity.SetStatus(Status.Error.WithDescription(errorText));
                     }
                     else
                     {

diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/README.md b/src/OpenTelemetry.Instrumentation.SqlClient/README.md
@@ -175,6 +175,21 @@ is the general extensibility point to add additional properties to any activity.
 The `Enrich` option is specific to this instrumentation, and is provided to
 get access to `SqlCommand` object.
 
+### RecordException
+
+This option, available on .NET Core only, can be set to instruct the instrumentation
+to record SqlExceptions as Activity [events](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md).
+
+The default value is `false` and can be changed by the code like below.
+
+```csharp
+using var tracerProvider = Sdk.CreateTracerProviderBuilder()
+    .AddSqlClientInstrumentation(
+        options => options.RecordException = true)
+    .AddConsoleExporter()
+    .Build();
+```
+
 ## References
 
 * [OpenTelemetry Project](https://opentelemetry.io/)
diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs
@@ -107,6 +107,16 @@ public class SqlClientInstrumentationOptions
         /// </example>
         public Action<Activity, string, object> Enrich { get; set; }
 
+#if !NETFRAMEWORK
+        /// <summary>
+        /// Gets or sets a value indicating whether the exception will be recorded as ActivityEvent or not. Default value: False.
+        /// </summary>
+        /// <remarks>
+        /// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md.
+        /// </remarks>
+        public bool RecordException { get; set; }
+#endif
+
         internal static SqlConnectionDetails ParseDataSource(string dataSource)
         {
             Match match = DataSourceRegex.Match(dataSource);

diff --git a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTests.cs b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTests.cs
@@ -17,6 +17,7 @@
 using System;
 using System.Data;
 using System.Diagnostics;
+using System.Linq;
 #if NET452
 using System.Data.SqlClient;
 #else
@@ -71,7 +72,8 @@ public void SqlClient_BadArgs()
         [InlineData(CommandType.Text, "select 1/1", false, true)]
 #endif
         [InlineData(CommandType.Text, "select 1/0", false, false, true)]
-        [InlineData(CommandType.Text, "select 1/0", false, false, true, false)]
+        [InlineData(CommandType.Text, "select 1/0", false, false, true, false, false)]
+        [InlineData(CommandType.Text, "select 1/0", false, false, true, true, false)]
         [InlineData(CommandType.StoredProcedure, "sp_who", false)]
         [InlineData(CommandType.StoredProcedure, "sp_who", true)]
         public void SuccessfulCommandTest(
@@ -80,6 +82,7 @@ public void SuccessfulCommandTest(
             bool captureStoredProcedureCommandName,
             bool captureTextCommandContent = false,
             bool isFailure = false,
+            bool recordException = false,
             bool shouldEnrich = true)
         {
             var activityProcessor = new Mock<BaseProcessor<Activity>>();
@@ -90,6 +93,7 @@ public void SuccessfulCommandTest(
 #if !NETFRAMEWORK
                     options.SetStoredProcedureCommandName = captureStoredProcedureCommandName;
                     options.SetTextCommandContent = captureTextCommandContent;
+                    options.RecordException = recordException;
 #else
                     options.SetStatementText = captureStoredProcedureCommandName;
 #endif
@@ -100,6 +104,11 @@ public void SuccessfulCommandTest(
                 })
                 .Build();
 
+#if NETFRAMEWORK
+            // RecordException not available on netfx
+            recordException = false;
+#endif
+
             using SqlConnection sqlConnection = new SqlConnection(SqlConnectionString);
 
             sqlConnection.Open();
@@ -125,7 +134,7 @@ public void SuccessfulCommandTest(
 
             var activity = (Activity)activityProcessor.Invocations[1].Arguments[0];
 
-            VerifyActivityData(commandType, commandText, captureStoredProcedureCommandName, captureTextCommandContent, isFailure, dataSource, activity);
+            VerifyActivityData(commandType, commandText, captureStoredProcedureCommandName, captureTextCommandContent, isFailure, recordException, dataSource, activity);
         }
 
         // DiagnosticListener-based instrumentation is only available on .NET Core
@@ -201,16 +210,19 @@ public void SqlClientCallsAreCollectedSuccessfully(
                 captureStoredProcedureCommandName,
                 captureTextCommandContent,
                 false,
+                false,
                 sqlConnection.DataSource,
                 (Activity)processor.Invocations[2].Arguments[0]);
         }
 
         [Theory]
         [InlineData(SqlClientDiagnosticListener.SqlDataBeforeExecuteCommand, SqlClientDiagnosticListener.SqlDataWriteCommandError)]
         [InlineData(SqlClientDiagnosticListener.SqlDataBeforeExecuteCommand, SqlClientDiagnosticListener.SqlDataWriteCommandError, false)]
+        [InlineData(SqlClientDiagnosticListener.SqlDataBeforeExecuteCommand, SqlClientDiagnosticListener.SqlDataWriteCommandError, false, true)]
         [InlineData(SqlClientDiagnosticListener.SqlMicrosoftBeforeExecuteCommand, SqlClientDiagnosticListener.SqlMicrosoftWriteCommandError)]
         [InlineData(SqlClientDiagnosticListener.SqlMicrosoftBeforeExecuteCommand, SqlClientDiagnosticListener.SqlMicrosoftWriteCommandError, false)]
-        public void SqlClientErrorsAreCollectedSuccessfully(string beforeCommand, string errorCommand, bool shouldEnrich = true)
+        [InlineData(SqlClientDiagnosticListener.SqlMicrosoftBeforeExecuteCommand, SqlClientDiagnosticListener.SqlMicrosoftWriteCommandError, false, true)]
+        public void SqlClientErrorsAreCollectedSuccessfully(string beforeCommand, string errorCommand, bool shouldEnrich = true, bool recordException = false)
         {
             using var sqlConnection = new SqlConnection(TestConnectionString);
             using var sqlCommand = sqlConnection.CreateCommand();
@@ -219,6 +231,7 @@ public void SqlClientErrorsAreCollectedSuccessfully(string beforeCommand, string
             using (Sdk.CreateTracerProviderBuilder()
                 .AddSqlClientInstrumentation(options =>
                 {
+                    options.RecordException = recordException;
                     if (shouldEnrich)
                     {
                         options.Enrich = ActivityEnrichment;
@@ -263,6 +276,7 @@ public void SqlClientErrorsAreCollectedSuccessfully(string beforeCommand, string
                 true,
                 false,
                 true,
+                recordException,
                 sqlConnection.DataSource,
                 (Activity)processor.Invocations[2].Arguments[0]);
         }
@@ -274,6 +288,7 @@ private static void VerifyActivityData(
             bool captureStoredProcedureCommandName,
             bool captureTextCommandContent,
             bool isFailure,
+            bool recordException,
             string dataSource,
             Activity activity)
         {
@@ -289,6 +304,18 @@ private static void VerifyActivityData(
                 var status = activity.GetStatus();
                 Assert.Equal(Status.Error.StatusCode, status.StatusCode);
                 Assert.NotNull(status.Description);
+
+                if (recordException)
+                {
+                    var events = activity.Events.ToList();
+                    Assert.Single(events);
+
+                    Assert.Equal(SemanticConventions.AttributeExceptionEventName, events[0].Name);
+                }
+                else
+                {
+                    Assert.Empty(activity.Events);
+                }
             }
 
             Assert.Equal(SqlActivitySourceHelper.MicrosoftSqlServerDatabaseSystemName, activity.GetTagValue(SemanticConventions.AttributeDbSystem));

diff --git a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlEventSourceTests.netfx.cs b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlEventSourceTests.netfx.cs
@@ -19,6 +19,7 @@
 using System.Data.SqlClient;
 using System.Diagnostics;
 using System.Diagnostics.Tracing;
+using System.Linq;
 using System.Threading.Tasks;
 using Moq;
 using OpenTelemetry.Instrumentation.SqlClient.Implementation;