diff --git a/CHANGELOG.md b/CHANGELOG.md index 8066237669..6b8036e62c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/config.md b/docs/config.md index 1e4b902d45..0871820085 100644 --- a/docs/config.md +++ b/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 diff --git a/src/OpenTelemetry.AutoInstrumentation.Native/cor_profiler.cpp b/src/OpenTelemetry.AutoInstrumentation.Native/cor_profiler.cpp index fca639ef8d..4422120ec1 100644 --- a/src/OpenTelemetry.AutoInstrumentation.Native/cor_profiler.cpp +++ b/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 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(IID_IMetaDataImport); + const auto& metadata_emit = metadata_interfaces.As(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 metadata_interfaces; diff --git a/src/OpenTelemetry.AutoInstrumentation.Native/cor_profiler.h b/src/OpenTelemetry.AutoInstrumentation.Native/cor_profiler.h index 2b54636aff..f837277243 100644 --- a/src/OpenTelemetry.AutoInstrumentation.Native/cor_profiler.h +++ b/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; }; diff --git a/src/OpenTelemetry.AutoInstrumentation.Native/environment_variables.h b/src/OpenTelemetry.AutoInstrumentation.Native/environment_variables.h index 35d7d4ee74..68ccad6eb8 100644 --- a/src/OpenTelemetry.AutoInstrumentation.Native/environment_variables.h +++ b/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 diff --git a/src/OpenTelemetry.AutoInstrumentation.Native/environment_variables_util.cpp b/src/OpenTelemetry.AutoInstrumentation.Native/environment_variables_util.cpp index f6a6ae1155..23a51b1340 100644 --- a/src/OpenTelemetry.AutoInstrumentation.Native/environment_variables_util.cpp +++ b/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 \ No newline at end of file diff --git a/src/OpenTelemetry.AutoInstrumentation.Native/environment_variables_util.h b/src/OpenTelemetry.AutoInstrumentation.Native/environment_variables_util.h index e34de45629..1e1e9f1a40 100644 --- a/src/OpenTelemetry.AutoInstrumentation.Native/environment_variables_util.h +++ b/src/OpenTelemetry.AutoInstrumentation.Native/environment_variables_util.h @@ -50,6 +50,7 @@ bool IsDumpILRewriteEnabled(); bool IsAzureAppServices(); bool IsFailFastEnabled(); bool IsNetFxAssemblyRedirectionEnabled(); +bool IsSqlClientNetFxILRewriteEnabled(); } // namespace trace diff --git a/test/IntegrationTests/SqlClientSystemDataTests.cs b/test/IntegrationTests/SqlClientSystemDataTests.cs index 5b98a99826..90f3ceb55e 100644 --- a/test/IntegrationTests/SqlClientSystemDataTests.cs +++ b/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() diff --git a/test/IntegrationTests/SqlClientSystemTests.cs b/test/IntegrationTests/SqlClientSystemTests.cs index cef5271070..9ab537500d 100644 --- a/test/IntegrationTests/SqlClientSystemTests.cs +++ b/test/IntegrationTests/SqlClientSystemTests.cs @@ -18,6 +18,19 @@ public SqlClientSystemTests(ITestOutputHelper output, SqlServerFixture sqlServer _sqlServerFixture = sqlServerFixture; } + public static TheoryData GetDataForIlRewrite() + { + var theoryData = new TheoryData(); + + 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")]