.NET Framework `SqlCommand` IL rewrite for `SqlClient` instrumentation #4470

stevejgordon · 2025-09-19T11:48:25Z

Why

Details in #4343. In short, on .NET Framework System.Data doesn't emit the CommandText (except for stored procedures) in its events. This prevents the SqlClient instrumentation from enriching with the DB query text and summary.

Fixes #4343

What

As discussed in the issue. This rewrites the WriteBeginExecuteEvent to ensure the CommandText is always present. The implementation currently enables this by default, but provides configuration to disable this behaviour if required.

Tests

The SqlClientSystemTests integration test was updated to to the new OTEL_DOTNET_AUTO_SQLCLIENT_NETFX_ILREWRITE_ENABLED environmnet variable.

Checklist

CHANGELOG.md is updated.
Documentation is updated.
New features are covered by tests.

cc @alanwest, @martincostello

src/OpenTelemetry.AutoInstrumentation.Native/environment_variables.h

src/OpenTelemetry.AutoInstrumentation.Native/cor_profiler.cpp

test/IntegrationTests/SqlClientSystemTests.cs

docs/config.md

stevejgordon · 2025-09-29T10:42:18Z

@zacharycmontoya and @Kielek - Thanks for the review. I am back from vacation and will update this ASAP.

test/IntegrationTests/SqlClientSystemTests.cs

Kielek · 2025-10-15T16:09:47Z

@stevejgordon, do you need any help here or just more time?
It will be great to include it in the next release.

stevejgordon · 2025-10-16T14:06:47Z

@Kielek I may need a bit of help. Previously I had the test running in Visual Studio and it was passing. Now it doesn't! ~~I looked at the logs and I don't see the rewrite occurring.~~ UPDATE: I was looking in the wrong log. I see the IL rewrite has occurred as expected. So I'm not immediately sure why the assertion is failing.

In an isolated test application running in IIS, with the files from tracer-home copied over the existing install, I see the rewrite logging appear. There are some errors in the managed log where it looks like it's picking up the wrong DLLs which didn't used to occur when I last tested, but I've since been messing with other things that likely screwed that up. However, as I see the IL rewrite, that's the main thing.

Would you perhaps be able to review if I've missed something in this test and/or test locally yourself to see if this is working as expected? I'll continue to dig into it on my end too and try to figure it out.

stevejgordon · 2025-10-16T14:34:37Z

@Kielek Could it be anything to do with the removal of the OTEL_DOTNET_AUTO_SQLCLIENT_SET_DBSTATEMENT_FOR_TEXT option?

Kielek · 2025-10-22T16:15:03Z

@stevejgordon, sorry for delay. Either @zacharycmontoya will check it today or I will try to help tommorow.

zacharycmontoya · 2025-10-22T18:29:02Z

Would you perhaps be able to review if I've missed something in this test and/or test locally yourself to see if this is working as expected? I'll continue to dig into it on my end too and try to figure it out.

I was able to get your integration test running on my machine and it passes ✅ However, I'd also like to add this same testing to our test case that uses the .NET Framework built-in System.Data.SqlCommmand in the SqlClientSystemDataTests.cs class. Can you add this or would it be fine if I added onto your PR?

stevejgordon · 2025-10-22T18:48:29Z

@zacharycmontoya Thanks for checking. Ill have to try to figure out why they are failing on my machine. Feel free to duplicate this for the other scenarios and add to this PR in the mean time.

…ramework-only TestApplication.SqlClient.System.NetFramework test that uses the built-in System.Data.SqlClient.SqlCommand class from System.Data.dll

zacharycmontoya · 2025-10-23T00:16:02Z

@stevejgordon I've added one test case in commit 355be86, which also passed locally on my machine. If CI passes, then I think it's good-to-go

Kielek · 2025-10-23T04:46:55Z

docs/config.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) |


I have some doubts here. As t might impact end-users, I would consider this as opt-in feature.

I am actually leaning towards on by default because the effect is that the user gets one more attribute on their spans, so overall this improves the tracing quality automatically and makes it more appealing to use our automatic package. If there are any side-effects from turning this on, then we should promptly identify and resolve them.

Unfortunately, I think that the side-effects that I'm most concerned about are outside of our control. The code that we are modifying causes the sql to get written to an EventSource. When things are written to an EventSource, that data can now be consumed by other things, including things that can result in data getting persisted outside of the process. Those things may not be sanitizing the sql, or may assume that the data was safe to begin with (just the stored procedure).
I think having it on would be fine for the majority of users, but there are probably a few that would run into problems with having this data present by default.

Thanks, I overlooked the fact that we're writing to an EventSource and not directly updating the span. It makes sense then to have users opt-in to the feature

Kielek · 2025-10-23T04:53:34Z

src/OpenTelemetry.AutoInstrumentation.Native/environment_variables_util.h

 bool IsAzureAppServices();
 bool IsFailFastEnabled();
 bool IsNetFxAssemblyRedirectionEnabled();
+bool IsSqlClientNetFxILRewriteEnabled();


Ideally, it should be propagated from the managed coded. We are in the middle of introducing File based configuration and it need to be reworked in the near feature. Not a blocker for this PR IMO.

Ref: #4492

stevejgordon added 2 commits September 19, 2025 12:38

Implement SqlCommand IL rewrite for NETFX

bc134fa

Update docs and tests

9fcab4e

stevejgordon requested a review from a team as a code owner September 19, 2025 11:48

Apply native formatting

e316036

zacharycmontoya reviewed Sep 22, 2025

View reviewed changes

src/OpenTelemetry.AutoInstrumentation.Native/environment_variables.h Outdated Show resolved Hide resolved

zacharycmontoya reviewed Sep 22, 2025

View reviewed changes

src/OpenTelemetry.AutoInstrumentation.Native/cor_profiler.cpp Show resolved Hide resolved

zacharycmontoya reviewed Sep 22, 2025

View reviewed changes

test/IntegrationTests/SqlClientSystemTests.cs Outdated Show resolved Hide resolved

Kielek reviewed Sep 23, 2025

View reviewed changes

docs/config.md Outdated Show resolved Hide resolved

stevejgordon and others added 2 commits October 7, 2025 09:27

Code review feedback

3f1876d

Merge branch 'main' into sql-bytecode

394e517

nrcventura reviewed Oct 7, 2025

View reviewed changes

test/IntegrationTests/SqlClientSystemTests.cs Outdated Show resolved Hide resolved

Fix SqlClientSystemTests

8b6c94b

zacharycmontoya added 2 commits October 22, 2025 14:21

Merge branch 'main' into sql-bytecode

3691cf4

Add IlRewrite test to SqlClientSystemDataTests, which runs the .NET F…

355be86

…ramework-only TestApplication.SqlClient.System.NetFramework test that uses the built-in System.Data.SqlClient.SqlCommand class from System.Data.dll

Fix linting issues in CHANGELOG.md

23ced4c

Kielek reviewed Oct 23, 2025

View reviewed changes

Uh oh!

.NET Framework SqlCommand IL rewrite for SqlClient instrumentation #4470

Are you sure you want to change the base?

.NET Framework SqlCommand IL rewrite for SqlClient instrumentation #4470

Uh oh!

Conversation

stevejgordon commented Sep 19, 2025

Why

What

Tests

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevejgordon commented Sep 29, 2025

Uh oh!

Uh oh!

Kielek commented Oct 15, 2025

Uh oh!

stevejgordon commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevejgordon commented Oct 16, 2025

Uh oh!

Kielek commented Oct 22, 2025

Uh oh!

zacharycmontoya commented Oct 22, 2025

Uh oh!

stevejgordon commented Oct 22, 2025

Uh oh!

zacharycmontoya commented Oct 23, 2025

Uh oh!

Kielek Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

nrcventura Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Kielek Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

.NET Framework `SqlCommand` IL rewrite for `SqlClient` instrumentation #4470

.NET Framework `SqlCommand` IL rewrite for `SqlClient` instrumentation #4470

stevejgordon commented Oct 16, 2025 •

edited

Loading