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 <cithomas@microsoft.com>

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

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

src/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

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
 

src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

@@ -176,6 +176,11 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Implementation
                                 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
                                 {

src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs

@@ -183,7 +183,8 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Implementation
                     }
                     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
                     {

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/)

src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs

@@ -107,6 +107,16 @@ namespace OpenTelemetry.Instrumentation.SqlClient
         /// </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);

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 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Tests
         [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 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Tests
             bool captureStoredProcedureCommandName,
             bool captureTextCommandContent = false,
             bool isFailure = false,
+            bool recordException = false,
             bool shouldEnrich = true)
         {
             var activityProcessor = new Mock<BaseProcessor<Activity>>();
@@ -90,6 +93,7 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Tests
 #if !NETFRAMEWORK
                     options.SetStoredProcedureCommandName = captureStoredProcedureCommandName;
                     options.SetTextCommandContent = captureTextCommandContent;
+                    options.RecordException = recordException;
 #else
                     options.SetStatementText = captureStoredProcedureCommandName;
 #endif
@@ -100,6 +104,11 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Tests
                 })
                 .Build();
 
+#if NETFRAMEWORK
+            // RecordException not available on netfx
+            recordException = false;
+#endif
+
             using SqlConnection sqlConnection = new SqlConnection(SqlConnectionString);
 
             sqlConnection.Open();
@@ -125,7 +134,7 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Tests
 
             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,6 +210,7 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Tests
                 captureStoredProcedureCommandName,
                 captureTextCommandContent,
                 false,
+                false,
                 sqlConnection.DataSource,
                 (Activity)processor.Invocations[2].Arguments[0]);
         }
@@ -208,9 +218,11 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Tests
         [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 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Tests
             using (Sdk.CreateTracerProviderBuilder()
                 .AddSqlClientInstrumentation(options =>
                 {
+                    options.RecordException = recordException;
                     if (shouldEnrich)
                     {
                         options.Enrich = ActivityEnrichment;
@@ -263,6 +276,7 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Tests
                 true,
                 false,
                 true,
+                recordException,
                 sqlConnection.DataSource,
                 (Activity)processor.Invocations[2].Arguments[0]);
         }
@@ -274,6 +288,7 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Tests
             bool captureStoredProcedureCommandName,
             bool captureTextCommandContent,
             bool isFailure,
+            bool recordException,
             string dataSource,
             Activity activity)
         {
@@ -289,6 +304,18 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Tests
                 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));

test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlEventSourceTests.netfx.cs

@@ -19,6 +19,7 @@ using System.Data;
 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;