[SqlClient] Refactor SqlConnectionDetails

src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md

@@ -30,6 +30,9 @@
   for the same information. Note that `server.address` is only included when
   the `EnableConnectionLevelAttributes` option is enabled.
   ([#2229](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2229))
+* Fix bug where `server.port` attribute was captured as a string. It should be an
+  integer.
+  ([#2233](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2233))
 
 ## 1.9.0-beta.1
 

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

@@ -26,4 +26,35 @@ internal sealed class SqlActivitySourceHelper
     {
         new KeyValuePair<string, object?>(SemanticConventions.AttributeDbSystem, MicrosoftSqlServerDatabaseSystemName),
     };
+
+    public static void AddConnectionLevelDetailsToActivity(string dataSource, Activity activity, SqlClientTraceInstrumentationOptions options)
+    {
+        // TODO: The attributes added here are required. We need to consider
+        // collecting these attributes by default.
+        if (options.EnableConnectionLevelAttributes)
+        {
+            var connectionDetails = SqlConnectionDetails.ParseDataSource((string)dataSource);
+
+            // TODO: In the new conventions, instance name should now be captured
+            // as a part of db.namespace, when available.
+            if (options.EmitOldAttributes && !string.IsNullOrEmpty(connectionDetails.InstanceName))
+            {
+                activity.SetTag(SemanticConventions.AttributeDbMsSqlInstanceName, connectionDetails.InstanceName);
+            }
+
+            if (!string.IsNullOrEmpty(connectionDetails.ServerHostName))
+            {
+                activity.SetTag(SemanticConventions.AttributeServerAddress, connectionDetails.ServerHostName);
+            }
+            else
+            {
+                activity.SetTag(SemanticConventions.AttributeServerAddress, connectionDetails.ServerIpAddress);
+            }
+
+            if (connectionDetails.Port.HasValue)
+            {
+                activity.SetTag(SemanticConventions.AttributeServerPort, connectionDetails.Port);
+            }
+        }
+    }
 }

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

@@ -116,7 +116,7 @@ public override void OnEventWritten(string name, object? payload)
 
                         if (dataSource != null)
                         {
-                            this.options.AddConnectionLevelDetailsToActivity((string)dataSource, activity);
+                            SqlActivitySourceHelper.AddConnectionLevelDetailsToActivity((string)dataSource, activity, this.options);
                         }
 
                         if (this.commandTypeFetcher.TryFetch(command, out CommandType commandType))

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

@@ -0,0 +1,126 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+using System.Collections.Concurrent;
+using System.Text.RegularExpressions;
+
+namespace OpenTelemetry.Instrumentation.SqlClient.Implementation;
+
+internal sealed class SqlConnectionDetails
+{
+    /*
+     * Match...
+     *  protocol[ ]:[ ]serverName
+     *  serverName
+     *  serverName[ ]\[ ]instanceName
+     *  serverName[ ],[ ]port
+     *  serverName[ ]\[ ]instanceName[ ],[ ]port
+     *
+     * [ ] can be any number of white-space, SQL allows it for some reason.
+     *
+     * Optional "protocol" can be "tcp", "lpc" (shared memory), or "np" (named pipes). See:
+     *  https://docs.microsoft.com/troubleshoot/sql/connect/use-server-name-parameter-connection-string, and
+     *  https://docs.microsoft.com/dotnet/api/system.data.sqlclient.sqlconnection.connectionstring?view=dotnet-plat-ext-5.0
+     *
+     * In case of named pipes the Data Source string can take form of:
+     *  np:serverName\instanceName, or
+     *  np:\\serverName\pipe\pipeName, or
+     *  np:\\serverName\pipe\MSSQL$instanceName\pipeName - in this case a separate regex (see NamedPipeRegex below)
+     *  is used to extract instanceName
+     */
+    private static readonly Regex DataSourceRegex = new("^(.*\\s*:\\s*\\\\{0,2})?(.*?)\\s*(?:[\\\\,]|$)\\s*(.*?)\\s*(?:,|$)\\s*(.*)$", RegexOptions.Compiled);
+
+    /// <summary>
+    /// In a Data Source string like "np:\\serverName\pipe\MSSQL$instanceName\pipeName" match the
+    /// "pipe\MSSQL$instanceName" segment to extract instanceName if it is available.
+    /// </summary>
+    /// <see>
+    /// <a href="https://docs.microsoft.com/previous-versions/sql/sql-server-2016/ms189307(v=sql.130)"/>
+    /// </see>
+    private static readonly Regex NamedPipeRegex = new("pipe\\\\MSSQL\\$(.*?)\\\\", RegexOptions.Compiled);
+
+    private static readonly ConcurrentDictionary<string, SqlConnectionDetails> ConnectionDetailCache = new(StringComparer.OrdinalIgnoreCase);
+
+    public string? ServerHostName { get; private set; }
+
+    public string? ServerIpAddress { get; private set; }
+
+    public string? InstanceName { get; private set; }
+
+    public int? Port { get; private set; }
+
+    public static SqlConnectionDetails ParseDataSource(string dataSource)
+    {
+        if (ConnectionDetailCache.TryGetValue(dataSource, out SqlConnectionDetails? connectionDetails))
+        {
+            return connectionDetails;
+        }
+
+        var match = DataSourceRegex.Match(dataSource);
+
+        string? serverHostName = match.Groups[2].Value;
+        string? serverIpAddress = null;
+        string? instanceName = null;
+        int? port = null;
+
+        var uriHostNameType = Uri.CheckHostName(serverHostName);
+        if (uriHostNameType == UriHostNameType.IPv4 || uriHostNameType == UriHostNameType.IPv6)
+        {
+            serverIpAddress = serverHostName;
+            serverHostName = null;
+        }
+
+        string maybeProtocol = match.Groups[1].Value;
+        bool isNamedPipe = maybeProtocol.Length > 0 &&
+                           maybeProtocol.StartsWith("np", StringComparison.OrdinalIgnoreCase);
+
+        if (isNamedPipe)
+        {
+            string pipeName = match.Groups[3].Value;
+            if (pipeName.Length > 0)
+            {
+                var namedInstancePipeMatch = NamedPipeRegex.Match(pipeName);
+                if (namedInstancePipeMatch.Success)
+                {
+                    instanceName = namedInstancePipeMatch.Groups[1].Value;
+                }
+            }
+        }
+        else
+        {
+            if (match.Groups[4].Length > 0)
+            {
+                instanceName = match.Groups[3].Value;
+                port = int.TryParse(match.Groups[4].Value, out int parsedPort)
+                    ? parsedPort == 1433 ? null : parsedPort
+                    : null;
+            }
+            else if (int.TryParse(match.Groups[3].Value, out int parsedPort))
+            {
+                instanceName = null;
+                port = parsedPort == 1433 ? null : parsedPort;
+            }
+            else
+            {
+                instanceName = match.Groups[3].Value;
+                if (string.IsNullOrEmpty(instanceName))
+                {
+                    instanceName = null;
+                }
+
+                port = null;
+            }
+        }
+
+        connectionDetails = new SqlConnectionDetails
+        {
+            ServerHostName = serverHostName,
+            ServerIpAddress = serverIpAddress,
+            InstanceName = instanceName,
+            Port = port,
+        };
+
+        ConnectionDetailCache.TryAdd(dataSource, connectionDetails);
+        return connectionDetails;
+    }
+}

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

@@ -140,7 +140,7 @@ private void OnBeginExecute(EventWrittenEventArgs eventData)
                 activity.SetTag(SemanticConventions.AttributeDbNamespace, databaseName);
             }
 
-            this.options.AddConnectionLevelDetailsToActivity((string)eventData.Payload[1], activity);
+            SqlActivitySourceHelper.AddConnectionLevelDetailsToActivity((string)eventData.Payload[1], activity, this.options);
 
             string commandText = (string)eventData.Payload[3];
             if (!string.IsNullOrEmpty(commandText) && this.options.SetDbStatementForText)

src/OpenTelemetry.Instrumentation.SqlClient/SqlClientTraceInstrumentationOptions.cs

@@ -1,10 +1,8 @@
 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0
 
-using System.Collections.Concurrent;
 using System.Data;
 using System.Diagnostics;
-using System.Text.RegularExpressions;
 using Microsoft.Extensions.Configuration;
 using OpenTelemetry.Trace;
 using static OpenTelemetry.Internal.DatabaseSemanticConventionHelper;
@@ -19,39 +17,6 @@ namespace OpenTelemetry.Instrumentation.SqlClient;
 /// </remarks>
 public class SqlClientTraceInstrumentationOptions
 {
-    /*
-     * Match...
-     *  protocol[ ]:[ ]serverName
-     *  serverName
-     *  serverName[ ]\[ ]instanceName
-     *  serverName[ ],[ ]port
-     *  serverName[ ]\[ ]instanceName[ ],[ ]port
-     *
-     * [ ] can be any number of white-space, SQL allows it for some reason.
-     *
-     * Optional "protocol" can be "tcp", "lpc" (shared memory), or "np" (named pipes). See:
-     *  https://docs.microsoft.com/troubleshoot/sql/connect/use-server-name-parameter-connection-string, and
-     *  https://docs.microsoft.com/dotnet/api/system.data.sqlclient.sqlconnection.connectionstring?view=dotnet-plat-ext-5.0
-     *
-     * In case of named pipes the Data Source string can take form of:
-     *  np:serverName\instanceName, or
-     *  np:\\serverName\pipe\pipeName, or
-     *  np:\\serverName\pipe\MSSQL$instanceName\pipeName - in this case a separate regex (see NamedPipeRegex below)
-     *  is used to extract instanceName
-     */
-    private static readonly Regex DataSourceRegex = new("^(.*\\s*:\\s*\\\\{0,2})?(.*?)\\s*(?:[\\\\,]|$)\\s*(.*?)\\s*(?:,|$)\\s*(.*)$", RegexOptions.Compiled);
-
-    /// <summary>
-    /// In a Data Source string like "np:\\serverName\pipe\MSSQL$instanceName\pipeName" match the
-    /// "pipe\MSSQL$instanceName" segment to extract instanceName if it is available.
-    /// </summary>
-    /// <see>
-    /// <a href="https://docs.microsoft.com/previous-versions/sql/sql-server-2016/ms189307(v=sql.130)"/>
-    /// </see>
-    private static readonly Regex NamedPipeRegex = new("pipe\\\\MSSQL\\$(.*?)\\\\", RegexOptions.Compiled);
-
-    private static readonly ConcurrentDictionary<string, SqlConnectionDetails> ConnectionDetailCache = new(StringComparer.OrdinalIgnoreCase);
-
     /// <summary>
     /// Initializes a new instance of the <see cref="SqlClientTraceInstrumentationOptions"/> class.
     /// </summary>
@@ -195,134 +160,4 @@ internal SqlClientTraceInstrumentationOptions(IConfiguration configuration)
     /// Gets or sets a value indicating whether the new database attributes should be emitted.
     /// </summary>
     internal bool EmitNewAttributes { get; set; }
-
-    internal static SqlConnectionDetails ParseDataSource(string dataSource)
-    {
-        Match match = DataSourceRegex.Match(dataSource);
-
-        string? serverHostName = match.Groups[2].Value;
-        string? serverIpAddress = null;
-
-        string? instanceName;
-
-        var uriHostNameType = Uri.CheckHostName(serverHostName);
-        if (uriHostNameType == UriHostNameType.IPv4 || uriHostNameType == UriHostNameType.IPv6)
-        {
-            serverIpAddress = serverHostName;
-            serverHostName = null;
-        }
-
-        string maybeProtocol = match.Groups[1].Value;
-        bool isNamedPipe = maybeProtocol.Length > 0 &&
-                           maybeProtocol.StartsWith("np", StringComparison.OrdinalIgnoreCase);
-
-        if (isNamedPipe)
-        {
-            string pipeName = match.Groups[3].Value;
-            if (pipeName.Length > 0)
-            {
-                var namedInstancePipeMatch = NamedPipeRegex.Match(pipeName);
-                if (namedInstancePipeMatch.Success)
-                {
-                    instanceName = namedInstancePipeMatch.Groups[1].Value;
-                    return new SqlConnectionDetails
-                    {
-                        ServerHostName = serverHostName,
-                        ServerIpAddress = serverIpAddress,
-                        InstanceName = instanceName,
-                        Port = null,
-                    };
-                }
-            }
-
-            return new SqlConnectionDetails
-            {
-                ServerHostName = serverHostName,
-                ServerIpAddress = serverIpAddress,
-                InstanceName = null,
-                Port = null,
-            };
-        }
-
-        string? port;
-        if (match.Groups[4].Length > 0)
-        {
-            instanceName = match.Groups[3].Value;
-            port = match.Groups[4].Value;
-            if (port == "1433")
-            {
-                port = null;
-            }
-        }
-        else if (int.TryParse(match.Groups[3].Value, out int parsedPort))
-        {
-            port = parsedPort == 1433 ? null : match.Groups[3].Value;
-            instanceName = null;
-        }
-        else
-        {
-            instanceName = match.Groups[3].Value;
-
-            if (string.IsNullOrEmpty(instanceName))
-            {
-                instanceName = null;
-            }
-
-            port = null;
-        }
-
-        return new SqlConnectionDetails
-        {
-            ServerHostName = serverHostName,
-            ServerIpAddress = serverIpAddress,
-            InstanceName = instanceName,
-            Port = port,
-        };
-    }
-
-    internal void AddConnectionLevelDetailsToActivity(string dataSource, Activity sqlActivity)
-    {
-        // TODO: The attributes added here are required. We need to consider
-        // collecting these attributes by default.
-        if (this.EnableConnectionLevelAttributes)
-        {
-            if (!ConnectionDetailCache.TryGetValue(dataSource, out SqlConnectionDetails? connectionDetails))
-            {
-                connectionDetails = ParseDataSource(dataSource);
-                ConnectionDetailCache.TryAdd(dataSource, connectionDetails);
-            }
-
-            // TODO: In the new conventions, instance name should now be captured
-            // as a part of db.namespace, when available.
-            if (this.EmitOldAttributes && !string.IsNullOrEmpty(connectionDetails.InstanceName))
-            {
-                sqlActivity.SetTag(SemanticConventions.AttributeDbMsSqlInstanceName, connectionDetails.InstanceName);
-            }
-
-            if (!string.IsNullOrEmpty(connectionDetails.ServerHostName))
-            {
-                sqlActivity.SetTag(SemanticConventions.AttributeServerAddress, connectionDetails.ServerHostName);
-            }
-            else
-            {
-                sqlActivity.SetTag(SemanticConventions.AttributeServerAddress, connectionDetails.ServerIpAddress);
-            }
-
-            if (!string.IsNullOrEmpty(connectionDetails.Port))
-            {
-                sqlActivity.SetTag(SemanticConventions.AttributeServerPort, connectionDetails.Port);
-            }
-        }
-    }
-
-    internal sealed class SqlConnectionDetails
-    {
-        public string? ServerHostName { get; set; }
-
-        public string? ServerIpAddress { get; set; }
-
-        public string? InstanceName { get; set; }
-
-        public string? Port { get; set; }
-    }
 }