From 4808e0559b8896bceb2412ac800ee8b20e3858e1 Mon Sep 17 00:00:00 2001 From: mbakalov <46465618+mbakalov@users.noreply.github.com> Date: Mon, 7 Dec 2020 13:12:11 -0500 Subject: [PATCH] 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 --- .../.publicApi/net452/PublicAPI.Unshipped.txt | 2 +- .../.publicApi/net461/PublicAPI.Unshipped.txt | 2 +- .../netstandard2.0/PublicAPI.Unshipped.txt | 2 ++ .../CHANGELOG.md | 4 +++ .../SqlClientDiagnosticListener.cs | 5 +++ .../SqlEventSourceListener.netfx.cs | 3 +- .../README.md | 15 +++++++++ .../SqlClientInstrumentationOptions.cs | 10 ++++++ .../SqlClientTests.cs | 33 +++++++++++++++++-- .../SqlEventSourceTests.netfx.cs | 1 + 10 files changed, 71 insertions(+), 6 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/net452/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/net452/PublicAPI.Unshipped.txt index fdd7c461021..63a3249cb8c 100644 --- 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 configureSqlClientInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder \ No newline at end of file +static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddSqlClientInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action 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 index fdd7c461021..63a3249cb8c 100644 --- 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 configureSqlClientInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder \ No newline at end of file +static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddSqlClientInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action configureSqlClientInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt index 947f8ae63be..72b40614295 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt +++ b/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 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 index a0bc873a4e4..a6e00b21258 100644 --- 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 index 25972776887..9989cfba5e3 100644 --- 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 index dbddaf55976..c9bf750c827 100644 --- 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 index 2f62b301e17..dc8f13c4daa 100644 --- 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 index 761f9537dec..ed668a06bc2 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs @@ -107,6 +107,16 @@ public class SqlClientInstrumentationOptions /// public Action Enrich { get; set; } +#if !NETFRAMEWORK + /// + /// Gets or sets a value indicating whether the exception will be recorded as ActivityEvent or not. Default value: False. + /// + /// + /// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md. + /// + 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 index 89c335ccc3c..4b8c68d2db1 100644 --- 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>(); @@ -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,6 +210,7 @@ public void SqlClientCallsAreCollectedSuccessfully( captureStoredProcedureCommandName, captureTextCommandContent, false, + false, sqlConnection.DataSource, (Activity)processor.Invocations[2].Arguments[0]); } @@ -208,9 +218,11 @@ public void SqlClientCallsAreCollectedSuccessfully( [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 index fd7e72003fc..90d2af4ed54 100644 --- 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;