.NET Framework SqlCommand IL rewrite for SqlClient instrumentation

CHANGELOG.md

@@ -10,6 +10,10 @@ This component adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.h
 ### Added
 
 - Configuration based instrumentation.
+- IL rewrite for SqlCommand on .NET Framework to ensure CommandText is
+  available for [SqlClient instrumentation](https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/issues/4343).
+  This is enabled by default but can disabled via the
+  `OTEL_DOTNET_AUTO_SQLCLIENT_NETFX_ILREWRITE_ENABLED` environment variable.
 
 ### Changed
 

docs/config.md

@@ -26,6 +26,7 @@ with environment variables taking precedence over `App.config` or `Web.config` f
     - `OTEL_DOTNET_AUTO_LOG_DIRECTORY`
     - `OTEL_LOG_LEVEL`
     - `OTEL_DOTNET_AUTO_NETFX_REDIRECT_ENABLED`
+    - `OTEL_DOTNET_AUTO_SQLCLIENT_NETFX_ILREWRITE_ENABLED`
 
     Example with `OTEL_SERVICE_NAME` setting:
 
@@ -234,6 +235,7 @@ and [logs bridge](./log4net-bridge.md).
 | `OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_DISABLE_URL_QUERY_REDACTION`                 | Whether the ASP.NET Core instrumentation turns off redaction of the `url.query` attribute value.                                                                                                                                                                                                     | `false`       | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) |
 | `OTEL_DOTNET_EXPERIMENTAL_HTTPCLIENT_DISABLE_URL_QUERY_REDACTION`                 | Whether the HTTP client instrumentation turns off redaction of the `url.full` attribute value.                                                                                                                                                                                                       | `false`       | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) |
 | `OTEL_DOTNET_EXPERIMENTAL_ASPNET_DISABLE_URL_QUERY_REDACTION`                     | Whether the ASP.NET instrumentation turns off redaction of the `url.query` attribute value.                                                                                                                                                                                                          | `false`       | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) |
+| `OTEL_DOTNET_AUTO_SQLCLIENT_NETFX_ILREWRITE_ENABLED`        | Enables IL rewriting of `SqlCommand` on .NET Framework to ensure `CommandText` is present for `SqlClient` instrumentation, which is required for `db.query.text` and `db.query.summary` to be populated. | `true` | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) |
 
 ## Propagators
 
@@ -413,9 +415,9 @@ Important environment variables include:
 | `OTEL_DOTNET_AUTO_NETFX_REDIRECT_ENABLED`           | Enables automatic redirection of the assemblies used by the automatic instrumentation on the .NET Framework.                                                                                                                                                                                                                                                                                       | `true`        | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) |
 | `OTEL_DOTNET_AUTO_TRACES_ADDITIONAL_SOURCES`        | Comma-separated list of additional `System.Diagnostics.ActivitySource` names to be added to the tracer at the startup. Use it to capture manually instrumented spans.                                                                                                                                                                                                                              |               | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) |
 | `OTEL_DOTNET_AUTO_TRACES_ADDITIONAL_LEGACY_SOURCES` | Comma-separated list of additional legacy source names to be added to the tracer at the startup. Use it to capture `System.Diagnostics.Activity` objects created without using the `System.Diagnostics.ActivitySource` API.                                                                                                                                                                        |               | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) |
-| `OTEL_DOTNET_AUTO_FLUSH_ON_UNHANDLEDEXCEPTION`      | Controls whether the telemetry data is flushed when an [AppDomain.UnhandledException](https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.unhandledexception) event is raised. Set to `true` when you suspect that you are experiencing a problem with missing telemetry data and also experiencing unhandled exceptions.                                                                 | `false`       | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) |
+| `OTEL_DOTNET_AUTO_FLUSH_ON_UNHANDLEDEXCEPTION`      | Controls whether the telemetry data is flushed when an [AppDomain.UnhandledException](https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.unhandledexception) event is raised. Set to `true` when you suspect that you are experiencing a problem with missing telemetry data and also experiencing unhandled exceptions.                                                                | `false`       | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) |
 | `OTEL_DOTNET_AUTO_METRICS_ADDITIONAL_SOURCES`       | Comma-separated list of additional `System.Diagnostics.Metrics.Meter` names to be added to the meter at the startup. Use it to capture manually created metrics.                                                                                                                                                                                                                                   |               | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) |
-| `OTEL_DOTNET_AUTO_PLUGINS`                          | Colon-separated list of OTel SDK instrumentation plugin types, specified with the [assembly-qualified name](https://docs.microsoft.com/en-us/dotnet/api/system.type.assemblyqualifiedname?view=net-6.0#system-type-assemblyqualifiedname). *Note: This list must be colon-separated because the type names may include commas.* See more info on how to write plugins at [plugins.md](plugins.md). |               | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) |
+| `OTEL_DOTNET_AUTO_PLUGINS`               | Colon-separated list of OTel SDK instrumentation plugin types, specified with the [assembly-qualified name](https://docs.microsoft.com/en-us/dotnet/api/system.type.assemblyqualifiedname?view=net-6.0#system-type-assemblyqualifiedname). *Note: This list must be colon-separated because the type names may include commas.* See more info on how to write plugins at [plugins.md](plugins.md). |               | [Experimental](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md) | 
 
 ## RuleEngine
 

src/OpenTelemetry.AutoInstrumentation.Native/cor_profiler.cpp

@@ -621,6 +621,14 @@ HRESULT STDMETHODCALLTYPE CorProfiler::ModuleLoadFinished(ModuleID module_id, HR
                       " | IsResource = ", module_info.IsResource(), std::noboolalpha);
     }
 
+#ifdef _WIN32
+    if (runtime_information_.is_desktop() && module_info.assembly.name == WStr("System.Data") &&
+        IsSqlClientNetFxILRewriteEnabled())
+    {
+        RewriteILSystemDataCommandText(module_id);
+    }
+#endif
+
     if (module_info.IsNGEN())
     {
         // We check if the Module contains NGEN images and added to the
@@ -3234,6 +3242,143 @@ HRESULT CorProfiler::GenerateLoaderType(const ModuleID module_id,
     return S_OK;
 }
 
+// This method rewrites the IL of System.Data.SqlClient.SqlCommand.WriteBeginExecuteEvent
+// to ensure that the CommandText property is passed to the BeginExecute method. Previously,
+// the CommandText was not passed, except when executing a stored procedure, which limited
+// the usefulness of the telemetry data available for SQL instrumentation.
+HRESULT CorProfiler::RewriteILSystemDataCommandText(const ModuleID module_id)
+{
+    if (Logger::IsDebugEnabled())
+    {
+        Logger::Debug("RewriteILSystemDataCommandText: Attempting IL rewrite for System.Data");
+    }
+
+    ComPtr<IUnknown> metadata_interfaces;
+    auto             hr = this->info_->GetModuleMetaData(module_id, ofRead | ofWrite, IID_IMetaDataImport2,
+                                                         metadata_interfaces.GetAddressOf());
+
+    if (FAILED(hr))
+    {
+        Logger::Warn("RewriteILSystemDataCommandText: Failed to get metadata interface for ", module_id);
+        return hr;
+    }
+
+    const auto& metadata_import = metadata_interfaces.As<IMetaDataImport2>(IID_IMetaDataImport);
+    const auto& metadata_emit   = metadata_interfaces.As<IMetaDataEmit2>(IID_IMetaDataEmit);
+
+    // Find the type definition for System.Data.SqlClient.SqlCommand
+    mdTypeDef system_data_sqlclient_sqlcommand;
+    hr = metadata_import->FindTypeDefByName(WStr("System.Data.SqlClient.SqlCommand"), mdTokenNil,
+                                            &system_data_sqlclient_sqlcommand);
+
+    if (FAILED(hr))
+    {
+        Logger::Warn("RewriteILSystemDataCommandText: FindTypeDefByName System.Data.SqlClient.SqlCommand failed");
+        return hr;
+    }
+
+    SignatureBuilder::InstanceMethod write_begin_execute_event_signature{{SignatureBuilder::BuiltIn::Void}, {}};
+
+    // Find the method definition for WriteBeginExecuteEvent
+    mdMethodDef write_begin_execute_event_token;
+    hr = metadata_import->FindMethod(system_data_sqlclient_sqlcommand, WStr("WriteBeginExecuteEvent"),
+                                     write_begin_execute_event_signature.Head(),
+                                     write_begin_execute_event_signature.Size(), &write_begin_execute_event_token);
+
+    if (FAILED(hr))
+    {
+        Logger::Warn("RewriteILSystemDataCommandText: FindMethod "
+                     "System.Data.SqlClient.SqlCommand::WriteBeginExecuteEvent failed");
+        return hr;
+    }
+
+    ILRewriter rewriter(this->info_, nullptr, module_id, write_begin_execute_event_token);
+    hr = rewriter.Import();
+
+    if (FAILED(hr))
+    {
+        Logger::Warn("RewriteILSystemDataCommandText: ILRewriter.Import failed");
+        return hr;
+    }
+
+    // 1. Find the last callvirt instruction (BeginExecute)
+    // callvirt instance void System.Data.SqlEventSource::BeginExecute(int32, string, string, string)
+    ILInstr* lastInstr      = rewriter.GetILList()->m_pPrev;
+    ILInstr* targetCallvirt = nullptr;
+    for (ILInstr* instr = lastInstr; instr != rewriter.GetILList(); instr = instr->m_pPrev)
+    {
+        if (instr->m_opcode == CEE_CALLVIRT)
+        {
+            targetCallvirt = instr;
+            break;
+        }
+    }
+
+    if (!targetCallvirt)
+    {
+        Logger::Warn("RewriteILSystemDataCommandText: Could not find callvirt instruction for BeginExecute");
+        return E_FAIL;
+    }
+
+    // 2. Insert pop before the callvirt (remove last argument)
+    ILInstr* popInstr  = rewriter.NewILInstr();
+    popInstr->m_opcode = CEE_POP;
+    rewriter.InsertBefore(targetCallvirt, popInstr);
+
+    // 3. Insert ldarg.0 after pop (load 'this')
+    ILInstr* ldarg0Instr  = rewriter.NewILInstr();
+    ldarg0Instr->m_opcode = CEE_LDARG_0;
+    rewriter.InsertAfter(popInstr, ldarg0Instr);
+
+    // 4. Insert callvirt to get_CommandText after ldarg.0
+
+    SignatureBuilder::InstanceMethod get_command_text_signature{{SignatureBuilder::BuiltIn::String}, {}};
+
+    mdTypeRef dbCommandTypeRef;
+    hr = metadata_emit->DefineTypeRefByName(mdTokenNil, WStr("System.Data.Common.DbCommand"), &dbCommandTypeRef);
+
+    if (FAILED(hr))
+    {
+        Logger::Warn("RewriteILSystemDataCommandText: DefineTypeRefByName System.Data.Common.DbCommand failed");
+        return hr;
+    }
+
+    mdMemberRef getCommandTextMemberRef;
+    hr = metadata_emit->DefineMemberRef(dbCommandTypeRef, WStr("get_CommandText"), get_command_text_signature.Head(),
+                                        get_command_text_signature.Size(), &getCommandTextMemberRef);
+
+    if (FAILED(hr))
+    {
+        Logger::Warn("RewriteILSystemDataCommandText: DefineMemberRef get_CommandText failed");
+        return hr;
+    }
+
+    ILInstr* callvirtGetCommandTextInstr  = rewriter.NewILInstr();
+    callvirtGetCommandTextInstr->m_opcode = CEE_CALLVIRT;
+    callvirtGetCommandTextInstr->m_Arg32  = getCommandTextMemberRef;
+    rewriter.InsertAfter(ldarg0Instr, callvirtGetCommandTextInstr);
+
+    if (IsDumpILRewriteEnabled())
+    {
+        mdToken      token = 0;
+        TypeInfo     typeInfo{};
+        WSTRING      methodName = WStr("WriteBeginExecuteEvent");
+        FunctionInfo caller(token, methodName, typeInfo, MethodSignature(), FunctionMethodSignature());
+        Logger::Info(
+            GetILCodes("*** ModifyWriteBeginExecuteEvent: Modified Code: ", &rewriter, caller, metadata_import));
+    }
+
+    hr = rewriter.Export();
+
+    if (FAILED(hr))
+    {
+        Logger::Warn("RewriteILSystemDataCommandText: Call to ILRewriter.Export() failed for ModuleID=", module_id);
+        return hr;
+    }
+
+    return hr;
+}
+
 HRESULT CorProfiler::GenerateLoaderMethod(const ModuleID module_id, mdMethodDef* ret_method_token)
 {
     ComPtr<IUnknown> metadata_interfaces;

src/OpenTelemetry.AutoInstrumentation.Native/cor_profiler.h

@@ -223,6 +223,11 @@ class CorProfiler : public CorProfilerBase
     //
     void ConfigureContinuousProfiler(bool threadSamplingEnabled, unsigned int threadSamplingInterval, bool allocationSamplingEnabled, unsigned int maxMemorySamplesPerMinute, unsigned int selectedThreadsSamplingInterval);
 
+    //
+    // IL Rewriting methods
+    //
+    HRESULT RewriteILSystemDataCommandText(const ModuleID module_id);
+
     friend class TracerMethodRewriter;
 };
 

src/OpenTelemetry.AutoInstrumentation.Native/environment_variables.h

@@ -69,6 +69,9 @@ const WSTRING netfx_assembly_redirection_enabled = WStr("OTEL_DOTNET_AUTO_NETFX_
 // Enable the fail fast mode.
 const WSTRING fail_fast_enabled = WStr("OTEL_DOTNET_AUTO_FAIL_FAST_ENABLED");
 
+// Enable the IL rewrite of SqlClient instrumentation for .NET Framework applications to capture command text.
+const WSTRING sqlclient_netfx_ilrewrite_enabled = WStr("OTEL_DOTNET_AUTO_SQLCLIENT_NETFX_ILREWRITE_ENABLED");
+
 // The list of startup hooks defined for .NET Core 3.1+ applications.
 // This is a .NET runtime environment variable. 
 // See https://github.com/dotnet/runtime/blob/main/docs/design/features/host-startup-hook.md

src/OpenTelemetry.AutoInstrumentation.Native/environment_variables_util.cpp

@@ -43,4 +43,9 @@ bool IsNetFxAssemblyRedirectionEnabled()
     ToBooleanWithDefault(GetEnvironmentValue(environment::netfx_assembly_redirection_enabled), true);
 }
 
+bool IsSqlClientNetFxILRewriteEnabled()
+{
+    ToBooleanWithDefault(GetEnvironmentValue(environment::sqlclient_netfx_ilrewrite_enabled), true);
+}
+
 } // namespace trace

src/OpenTelemetry.AutoInstrumentation.Native/environment_variables_util.h

@@ -50,6 +50,7 @@ bool IsDumpILRewriteEnabled();
 bool IsAzureAppServices();
 bool IsFailFastEnabled();
 bool IsNetFxAssemblyRedirectionEnabled();
+bool IsSqlClientNetFxILRewriteEnabled();
 
 }  // namespace trace
 

test/IntegrationTests/SqlClientSystemDataTests.cs

@@ -28,6 +28,30 @@ public void SubmitTraces()
         collector.AssertExpectations();
     }
 
+    [SkippableTheory]
+    [Trait("Category", "EndToEnd")]
+    [InlineData(true)]
+    [InlineData(false)]
+    public void SqlClientIlRewrite(bool enableIlRewrite)
+    {
+        SetEnvironmentVariable("OTEL_DOTNET_AUTO_SQLCLIENT_NETFX_ILREWRITE_ENABLED", enableIlRewrite.ToString());
+        using var collector = new MockSpansCollector(Output);
+        SetExporter(collector);
+
+        if (enableIlRewrite)
+        {
+            collector.Expect("OpenTelemetry.Instrumentation.SqlClient", span => span.Attributes.Any(attr => attr.Key == "db.statement" && !string.IsNullOrWhiteSpace(attr.Value?.StringValue)));
+        }
+        else
+        {
+            collector.Expect("OpenTelemetry.Instrumentation.SqlClient", span => span.Attributes.All(attr => attr.Key != "db.statement"));
+        }
+
+        RunTestApplication();
+
+        collector.AssertExpectations();
+    }
+
     [Fact]
     [Trait("Category", "EndToEnd")]
     public void SubmitMetrics()

test/IntegrationTests/SqlClientSystemTests.cs

@@ -18,6 +18,19 @@ public SqlClientSystemTests(ITestOutputHelper output, SqlServerFixture sqlServer
         _sqlServerFixture = sqlServerFixture;
     }
 
+    public static TheoryData<string, bool> GetDataForIlRewrite()
+    {
+        var theoryData = new TheoryData<string, bool>();
+
+        foreach (var version in LibraryVersion.SqlClientSystem)
+        {
+            theoryData.Add(version, true);
+            theoryData.Add(version, false);
+        }
+
+        return theoryData;
+    }
+
     [SkippableTheory]
     [Trait("Category", "EndToEnd")]
     [Trait("Containers", "Linux")]
@@ -41,6 +54,39 @@ public void SubmitTraces(string packageVersion)
         collector.AssertExpectations();
     }
 
+#if NETFRAMEWORK
+    [SkippableTheory]
+    [Trait("Category", "EndToEnd")]
+    [Trait("Containers", "Linux")]
+    [MemberData(nameof(GetDataForIlRewrite))]
+    public void SqlClientIlRewrite(string packageVersion, bool enableIlRewrite)
+    {
+        // Skip the test if fixture does not support current platform
+        _sqlServerFixture.SkipIfUnsupportedPlatform();
+
+        SetEnvironmentVariable("OTEL_DOTNET_AUTO_SQLCLIENT_NETFX_ILREWRITE_ENABLED", enableIlRewrite.ToString());
+        using var collector = new MockSpansCollector(Output);
+        SetExporter(collector);
+
+        if (enableIlRewrite)
+        {
+            collector.Expect("OpenTelemetry.Instrumentation.SqlClient", span => span.Attributes.Any(attr => attr.Key == "db.statement" && !string.IsNullOrWhiteSpace(attr.Value?.StringValue)));
+        }
+        else
+        {
+            collector.Expect("OpenTelemetry.Instrumentation.SqlClient", span => span.Attributes.All(attr => attr.Key != "db.statement"));
+        }
+
+        RunTestApplication(new()
+        {
+            Arguments = $"{_sqlServerFixture.Password} {_sqlServerFixture.Port}",
+            PackageVersion = packageVersion
+        });
+
+        collector.AssertExpectations();
+    }
+#endif
+
     [SkippableTheory]
     [Trait("Category", "EndToEnd")]
     [Trait("Containers", "Linux")]