SqlClient instrumentation to support Microsoft.Data.SqlClient EventSource

src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/net452/PublicAPI.Unshipped.txt

@@ -3,10 +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.SetStoredProcedureCommandName.get -> bool
-OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetStoredProcedureCommandName.set -> void
-OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetTextCommandContent.get -> bool
-OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetTextCommandContent.set -> void
+OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetStatementText.get -> bool
+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

src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/net461/PublicAPI.Unshipped.txt

@@ -3,10 +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.SetStoredProcedureCommandName.get -> bool
-OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetStoredProcedureCommandName.set -> void
-OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetTextCommandContent.get -> bool
-OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetTextCommandContent.set -> void
+OpenTelemetry.Instrumentation.SqlClient.SqlClientInstrumentationOptions.SetStatementText.get -> bool
+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

src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md

@@ -2,6 +2,15 @@
 
 ## Unreleased
 
+* Microsoft.Data.SqlClient v2.0.0 and higher is now properly instrumented
+  on .NET Framework.
+  ([#1599](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1599))
+* SqlClientInstrumentationOptions API changes: `SetStoredProcedureCommandName`
+  and `SetTextCommandContent` are now only available on .NET Core. On .NET
+  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.
+
 ## 1.0.0-rc1.1
 
 Released 2020-Nov-17

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

@@ -0,0 +1,37 @@
+// <copyright file="SqlActivitySourceHelper.cs" company="OpenTelemetry Authors">
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+// </copyright>
+using System;
+using System.Diagnostics;
+
+namespace OpenTelemetry.Instrumentation.SqlClient.Implementation
+{
+    /// <summary>
+    /// Helper class to hold common properties used by both SqlClientDiagnosticListener on .NET Core
+    /// and SqlEventSourceListener on .NET Framework.
+    /// </summary>
+    internal class SqlActivitySourceHelper
+    {
+        public const string ActivitySourceName = "OpenTelemetry.SqlClient";
+        public const string ActivityName = ActivitySourceName + ".Execute";
+
+        public const string MicrosoftSqlServerDatabaseSystemName = "mssql";
+
+        private static readonly Version Version = typeof(SqlActivitySourceHelper).Assembly.GetName().Version;
+#pragma warning disable SA1202 // Elements should be ordered by access <- In this case, Version MUST come before ActivitySource otherwise null ref exception is thrown.
+        internal static readonly ActivitySource ActivitySource = new ActivitySource(ActivitySourceName, Version.ToString());
+#pragma warning restore SA1202 // Elements should be ordered by access
+    }
+}

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

@@ -13,6 +13,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 // </copyright>
+#if !NETFRAMEWORK
 using System;
 using System.Data;
 using System.Diagnostics;
@@ -22,11 +23,6 @@ namespace OpenTelemetry.Instrumentation.SqlClient.Implementation
 {
     internal class SqlClientDiagnosticListener : ListenerHandler
     {
-        public const string ActivitySourceName = "OpenTelemetry.SqlClient";
-        public const string ActivityName = ActivitySourceName + ".Execute";
-
-        public const string CommandCustomPropertyName = "OTel.SqlHandler.Command";
-
         public const string SqlDataBeforeExecuteCommand = "System.Data.SqlClient.WriteCommandBefore";
         public const string SqlMicrosoftBeforeExecuteCommand = "Microsoft.Data.SqlClient.WriteCommandBefore";
 
@@ -36,13 +32,6 @@ internal class SqlClientDiagnosticListener : ListenerHandler
         public const string SqlDataWriteCommandError = "System.Data.SqlClient.WriteCommandError";
         public const string SqlMicrosoftWriteCommandError = "Microsoft.Data.SqlClient.WriteCommandError";
 
-        public const string MicrosoftSqlServerDatabaseSystemName = "mssql";
-
-        private static readonly Version Version = typeof(SqlClientDiagnosticListener).Assembly.GetName().Version;
-#pragma warning disable SA1202 // Elements should be ordered by access <- In this case, Version MUST come before SqlClientActivitySource otherwise null ref exception is thrown.
-        internal static readonly ActivitySource SqlClientActivitySource = new ActivitySource(ActivitySourceName, Version.ToString());
-#pragma warning restore SA1202 // Elements should be ordered by access
-
         private readonly PropertyFetcher<object> commandFetcher = new PropertyFetcher<object>("Command");
         private readonly PropertyFetcher<object> connectionFetcher = new PropertyFetcher<object>("Connection");
         private readonly PropertyFetcher<object> dataSourceFetcher = new PropertyFetcher<object>("DataSource");
@@ -68,7 +57,7 @@ public override void OnCustom(string name, Activity activity, object payload)
                 case SqlMicrosoftBeforeExecuteCommand:
                     {
                         // SqlClient does not create an Activity. So the activity coming in here will be null or the root span.
-                        activity = SqlClientActivitySource.StartActivity(ActivityName, ActivityKind.Client);
+                        activity = SqlActivitySourceHelper.ActivitySource.StartActivity(SqlActivitySourceHelper.ActivityName, ActivityKind.Client);
                         if (activity == null)
                         {
                             // There is no listener or it decided not to sample the current request.
@@ -93,7 +82,7 @@ public override void OnCustom(string name, Activity activity, object payload)
                             _ = this.dataSourceFetcher.TryFetch(connection, out var dataSource);
                             _ = this.commandTextFetcher.TryFetch(command, out var commandText);
 
-                            activity.SetTag(SemanticConventions.AttributeDbSystem, MicrosoftSqlServerDatabaseSystemName);
+                            activity.SetTag(SemanticConventions.AttributeDbSystem, SqlActivitySourceHelper.MicrosoftSqlServerDatabaseSystemName);
                             activity.SetTag(SemanticConventions.AttributeDbName, (string)database);
 
                             this.options.AddConnectionLevelDetailsToActivity((string)dataSource, activity);
@@ -147,7 +136,7 @@ public override void OnCustom(string name, Activity activity, object payload)
                             return;
                         }
 
-                        if (activity.Source != SqlClientActivitySource)
+                        if (activity.Source != SqlActivitySourceHelper.ActivitySource)
                         {
                             return;
                         }
@@ -175,7 +164,7 @@ public override void OnCustom(string name, Activity activity, object payload)
                             return;
                         }
 
-                        if (activity.Source != SqlClientActivitySource)
+                        if (activity.Source != SqlActivitySourceHelper.ActivitySource)
                         {
                             return;
                         }
@@ -205,3 +194,4 @@ public override void OnCustom(string name, Activity activity, object payload)
         }
     }
 }
+#endif

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

@@ -15,26 +15,40 @@
 // </copyright>
 #if NETFRAMEWORK
 using System;
-using System.Data;
 using System.Diagnostics;
 using System.Diagnostics.Tracing;
 using OpenTelemetry.Trace;
 
 namespace OpenTelemetry.Instrumentation.SqlClient.Implementation
 {
     /// <summary>
-    /// .NET Framework SqlClient doesn't emit DiagnosticSource events.
-    /// We hook into its EventSource if it is available:
-    /// See: <a href="https://github.com/microsoft/referencesource/blob/3b1eaf5203992df69de44c783a3eda37d3d4cd10/System.Data/System/Data/Common/SqlEventSource.cs#L29">reference source</a>.
+    /// On .NET Framework, neither System.Data.SqlClient nor Microsoft.Data.SqlClient emit DiagnosticSource events.
+    /// Instead they use EventSource:
+    /// For System.Data.SqlClient see: <a href="https://github.com/microsoft/referencesource/blob/3b1eaf5203992df69de44c783a3eda37d3d4cd10/System.Data/System/Data/Common/SqlEventSource.cs#L29">reference source</a>.
+    /// For Microsoft.Data.SqlClient see: <a href="https://github.com/dotnet/SqlClient/blob/ac8bb3f9132e6c104dc3e307fe2d569daed0776f/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlClientEventSource.cs#L15">SqlClientEventSource</a>.
+    ///
+    /// We hook into these event sources and process their BeginExecute/EndExecute events.
     /// </summary>
+    /// <remarks>
+    /// Note that before version 2.0.0, Microsoft.Data.SqlClient used "Microsoft-AdoNet-SystemData"
+    /// EventSource (same as System.Data.SqlClient), but since 2.0.0 has switched to "Microsoft.Data.SqlClient.EventSource".
+    ///
+    /// Due to the limitation of the "Microsoft-AdoNet-SystemData", it is not possible to capture sql statement text
+    /// for CommandType.Text when using that EventSource. It only reports text for CommandType.StoredProcedure.
+    ///
+    /// "Microsoft.Data.SqlClient.EventSource" doesn't have that issue.
+    /// </remarks>
     internal class SqlEventSourceListener : EventListener
     {
         internal const string AdoNetEventSourceName = "Microsoft-AdoNet-SystemData";
+        internal const string MdsEventSourceName = "Microsoft.Data.SqlClient.EventSource";
+
         internal const int BeginExecuteEventId = 1;
         internal const int EndExecuteEventId = 2;
 
         private readonly SqlClientInstrumentationOptions options;
-        private EventSource eventSource;
+        private EventSource adoNetEventSource;
+        private EventSource mdsEventSource;
 
         public SqlEventSourceListener(SqlClientInstrumentationOptions options = null)
         {
@@ -43,9 +57,14 @@ public SqlEventSourceListener(SqlClientInstrumentationOptions options = null)
 
         public override void Dispose()
         {
-            if (this.eventSource != null)
+            if (this.adoNetEventSource != null)
+            {
+                this.DisableEvents(this.adoNetEventSource);
+            }
+
+            if (this.mdsEventSource != null)
             {
-                this.DisableEvents(this.eventSource);
+                this.DisableEvents(this.mdsEventSource);
             }
 
             base.Dispose();
@@ -55,7 +74,12 @@ protected override void OnEventSourceCreated(EventSource eventSource)
         {
             if (eventSource?.Name.StartsWith(AdoNetEventSourceName, StringComparison.Ordinal) == true)
             {
-                this.eventSource = eventSource;
+                this.adoNetEventSource = eventSource;
+                this.EnableEvents(eventSource, EventLevel.Informational, (EventKeywords)1);
+            }
+            else if (eventSource?.Name.StartsWith(MdsEventSourceName, StringComparison.Ordinal) == true)
+            {
+                this.mdsEventSource = eventSource;
                 this.EnableEvents(eventSource, EventLevel.Informational, (EventKeywords)1);
             }
 
@@ -88,7 +112,11 @@ private void OnBeginExecute(EventWrittenEventArgs eventData)
                 [0] -> ObjectId
                 [1] -> DataSource
                 [2] -> Database
-                [3] -> CommandText ([3] = CommandType == CommandType.StoredProcedure ? CommandText : string.Empty)
+                [3] -> CommandText
+
+                Note:
+                - For "Microsoft-AdoNet-SystemData": [3] CommandText = (CommandType == CommandType.StoredProcedure ? CommandText : string.Empty;
+                - For "Microsoft.Data.SqlClient.EventSource": [3] CommandText = sqlCommand.CommandText (so it is set for all command types).
              */
 
             if ((eventData?.Payload?.Count ?? 0) < 4)
@@ -97,7 +125,7 @@ private void OnBeginExecute(EventWrittenEventArgs eventData)
                 return;
             }
 
-            var activity = SqlClientDiagnosticListener.SqlClientActivitySource.StartActivity(SqlClientDiagnosticListener.ActivityName, ActivityKind.Client);
+            var activity = SqlActivitySourceHelper.ActivitySource.StartActivity(SqlActivitySourceHelper.ActivityName, ActivityKind.Client);
             if (activity == null)
             {
                 // There is no listener or it decided not to sample the current request.
@@ -110,23 +138,15 @@ private void OnBeginExecute(EventWrittenEventArgs eventData)
 
             if (activity.IsAllDataRequested)
             {
-                activity.SetTag(SemanticConventions.AttributeDbSystem, SqlClientDiagnosticListener.MicrosoftSqlServerDatabaseSystemName);
+                activity.SetTag(SemanticConventions.AttributeDbSystem, SqlActivitySourceHelper.MicrosoftSqlServerDatabaseSystemName);
                 activity.SetTag(SemanticConventions.AttributeDbName, databaseName);
 
                 this.options.AddConnectionLevelDetailsToActivity((string)eventData.Payload[1], activity);
 
                 string commandText = (string)eventData.Payload[3];
-                if (string.IsNullOrEmpty(commandText))
-                {
-                    activity.SetTag(SpanAttributeConstants.DatabaseStatementTypeKey, nameof(CommandType.Text));
-                }
-                else
+                if (!string.IsNullOrEmpty(commandText) && this.options.SetStatementText)
                 {
-                    activity.SetTag(SpanAttributeConstants.DatabaseStatementTypeKey, nameof(CommandType.StoredProcedure));
-                    if (this.options.SetStoredProcedureCommandName)
-                    {
-                        activity.SetTag(SemanticConventions.AttributeDbStatement, commandText);
-                    }
+                    activity.SetTag(SemanticConventions.AttributeDbStatement, commandText);
                 }
             }
         }
@@ -147,7 +167,7 @@ private void OnEndExecute(EventWrittenEventArgs eventData)
             }
 
             var activity = Activity.Current;
-            if (activity?.Source != SqlClientDiagnosticListener.SqlClientActivitySource)
+            if (activity?.Source != SqlActivitySourceHelper.ActivitySource)
             {
                 return;
             }

src/OpenTelemetry.Instrumentation.SqlClient/README.md

@@ -60,7 +60,7 @@ For an ASP.NET application, adding instrumentation is typically done in the
 This instrumentation can be configured to change the default behavior by using
 `SqlClientInstrumentationOptions`.
 
-### SetStoredProcedureCommandName
+### SetStoredProcedureCommandName (.NET Core)
 
 By default, when CommandType is CommandType.StoredProcedure this
 instrumentation will set the `db.statement` attribute to the stored procedure
@@ -77,18 +77,12 @@ using Sdk.CreateTracerProviderBuilder()
     .Build();
 ```
 
-### SetTextCommandContent
+### SetTextCommandContent (.NET Core)
 
 By default, when CommandType is CommandType.Text, this instrumentation will not
 set the `db.statement` attribute. This behavior can be enabled by setting
 `SetTextCommandContent` to true.
 
-For .NET Framework, `SetTextCommandContent` is unavailable when using
-System.Data.SqlClient. It is only available when using
-[`Microsoft.Data.SqlClient`](https://www.nuget.org/packages/Microsoft.Data.SqlClient/).
-`SetTextCommandContent` is fully functional in .NET Core when using either
-System.Data.SqlClient or Microsoft.Data.SqlClient.
-
 The following example shows how to use `SetTextCommandContent`.
 
 ```csharp
@@ -99,7 +93,33 @@ using Sdk.CreateTracerProviderBuilder()
     .Build();
 ```
 
-### EnableConnectionLevelAttributes
+## SetStatementText (.NET Framework)
+
+For .NET Framework, `SetTextCommandContent` and `SetStoredProcedureCommandName`
+are not available. Instead, `SetStatementText` should be used to control whether
+this instrumentation should set the `db.statement` attribute to the text of the
+`SqlCommand` being executed.
+
+Text capturing is _disabled_ by default. If enabled, the instrumentation will
+capture both `CommandType.Text` and `CommandType.StoredProcedure` when using
+[`Microsoft.Data.SqlClient`](https://www.nuget.org/packages/Microsoft.Data.SqlClient/),
+and only `CommandType.StoredProcedure` when using `System.Data.SqlClient`.
+
+To turn statement capturing on, use the options like in below example. Be
+aware that `CommandType.Text` SQL might contain sensitive data.
+On [`Microsoft.Data.SqlClient`](https://www.nuget.org/packages/Microsoft.Data.SqlClient/)
+only set this to `true` if you are absolutely sure that you are using
+exclusively stored procedures, or have no sensitive data in your `sqlCommand.CommandText`.
+
+```csharp
+using var tracerProvider = Sdk.CreateTracerProviderBuilder()
+    .AddSqlClientInstrumentation(
+        options => options.SetStatementText = true)
+    .AddConsoleExporter()
+    .Build();
+```
+
+## EnableConnectionLevelAttributes
 
 By default, `EnabledConnectionLevelAttributes` is disabled and this
 instrumentation sets the `peer.service` attribute to the