[SqlClient] Address review feedback

- Guard setting attributes behind `IsAllDataRequested`.
- Only set after any filtering decisions are made.
- Only set when using the new semantic conventions.
- Add test for case sensitivity pending feedback on attribute naming behaviour.

Author: martincostello

src/OpenTelemetry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs

@@ -125,17 +125,9 @@ public override void OnEventWritten(string name, object? payload)
                         return;
                     }
 
-                    object? command = null;
-
-                    if (this.options.SetDbQueryParameters)
-                    {
-                        command = this.commandFetcher.Fetch(payload);
-                        SqlParameterProcessor.AddQueryParameters(activity, command);
-                    }
-
                     if (activity.IsAllDataRequested)
                     {
-                        command ??= this.commandFetcher.Fetch(payload);
+                        var command = this.commandFetcher.Fetch(payload);
 
                         try
                         {
@@ -163,6 +155,11 @@ public override void OnEventWritten(string name, object? payload)
                             return;
                         }
 
+                        if (options.EmitNewAttributes && this.options.SetDbQueryParameters)
+                        {
+                            SqlParameterProcessor.AddQueryParameters(activity, command);
+                        }
+
                         if (this.commandTypeFetcher.Fetch(command) is CommandType commandType)
                         {
                             var commandText = this.commandTextFetcher.Fetch(command);

src/OpenTelemetry.Instrumentation.EntityFrameworkCore/README.md

@@ -140,6 +140,16 @@ services.AddOpenTelemetry()
         .AddConsoleExporter());
 ```
 
+### SetDbStatementForText
+
+`SetDbStatementForText` controls whether the `db.statement` attribute is
+emitted. The behavior of `SetDbStatementForText` depends on the runtime used,
+see below for more details.
+
+Query text may contain sensitive data, so when `SetDbStatementForText` is
+enabled the raw query text is sanitized by automatically replacing literal
+values with a `?` character.
+
 ## References
 
 * [OpenTelemetry Project](https://opentelemetry.io/)

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

@@ -121,11 +121,6 @@ public override void OnEventWritten(string name, object? payload)
                     }
 #endif
 
-                    if (options.SetDbQueryParameters)
-                    {
-                        SqlParameterProcessor.AddQueryParameters(activity, command);
-                    }
-
                     if (activity.IsAllDataRequested)
                     {
                         try
@@ -146,6 +141,11 @@ public override void OnEventWritten(string name, object? payload)
                             return;
                         }
 
+                        if (options.EmitNewAttributes && options.SetDbQueryParameters)
+                        {
+                            SqlParameterProcessor.AddQueryParameters(activity, command);
+                        }
+
                         if (this.commandTypeFetcher.TryFetch(command, out var commandType) &&
                             this.commandTextFetcher.TryFetch(command, out var commandText))
                         {

test/OpenTelemetry.Contrib.Shared.Tests/SqlParameterProcessorTests.cs

@@ -89,14 +89,16 @@ public static void HandlesMixedCommandParameters()
         command.Parameters.Add(new MyDbCommandParameter("foo", 1));
         command.Parameters.Add(new MyDbCommandParameter(2));
         command.Parameters.Add(new object());
+        command.Parameters.Add(new MyDbCommandParameter("FOO", 3));
 
         // Act
         SqlParameterProcessor.AddQueryParameters(activity, command);
 
         // Assert
         Assert.Equal(1, activity.GetTagValue("db.query.parameter.foo"));
         Assert.Equal(2, activity.GetTagValue("db.query.parameter.1"));
-        Assert.Equal(2, activity.TagObjects.Count());
+        Assert.Equal(3, activity.GetTagValue("db.query.parameter.FOO"));
+        Assert.Equal(3, activity.TagObjects.Count());
     }
 
 #nullable disable

test/OpenTelemetry.Instrumentation.EntityFrameworkCore.Tests/EntityFrameworkIntegrationTests.cs

@@ -310,6 +310,8 @@ bool ActivityFilter(string? providerName, IDbCommand command)
     public async Task SuccessfulParameterizedQueryTest(string provider)
     {
         // Arrange
+        using var scope = SemanticConventionScope.Get(useNewConventions: true);
+
         var activities = new List<Activity>();
         using var tracerProvider = Sdk.CreateTracerProviderBuilder()
             .AddInMemoryExporter(activities)

test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientIntegrationTests.cs

@@ -134,6 +134,8 @@ public async Task SuccessfulParameterizedQueryTest()
         var sampler = new TestSampler();
         var activities = new List<Activity>();
 
+        using var scope = SemanticConventionScope.Get(useNewConventions: true);
+
         using var tracerProvider = Sdk.CreateTracerProviderBuilder()
             .SetSampler(sampler)
             .AddInMemoryExporter(activities)
@@ -299,4 +301,22 @@ private string GetConnectionString()
             _ => throw new InvalidOperationException($"Container type '${this.fixture.DatabaseContainer.GetType().Name}' is not supported."),
         };
     }
+
+    private sealed class SemanticConventionScope(string? previous) : IDisposable
+    {
+        private const string ConventionsOptIn = "OTEL_SEMCONV_STABILITY_OPT_IN";
+
+        public static SemanticConventionScope Get(bool useNewConventions)
+        {
+            var previous = Environment.GetEnvironmentVariable(ConventionsOptIn);
+
+            Environment.SetEnvironmentVariable(
+                ConventionsOptIn,
+                useNewConventions ? "database" : string.Empty);
+
+            return new SemanticConventionScope(previous);
+        }
+
+        public void Dispose() => Environment.SetEnvironmentVariable(ConventionsOptIn, previous);
+    }
 }