From 72d1030d5ee16d5fdc9f6e2d61c9aa1a1747da0d Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Wed, 30 Oct 2024 11:56:05 -0700 Subject: [PATCH] [SqlClient] Include instance in `db.namespace` and activity name, start activity with required tags (#2277) --- .../CHANGELOG.md | 8 +- .../Implementation/SqlActivitySourceHelper.cs | 71 +++++++--- .../SqlClientDiagnosticListener.cs | 55 +++----- .../SqlEventSourceListener.netfx.cs | 23 +-- .../SqlClientTestCase.cs | 53 +++++++ .../SqlClientTests.cs | 132 ++++++++++++++---- .../SqlEventSourceTests.netfx.cs | 12 +- 7 files changed, 247 insertions(+), 107 deletions(-) create mode 100644 test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTestCase.cs diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md index b6fa12db57..bfc0dc4d1e 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md @@ -24,7 +24,8 @@ specification for more information regarding the new database semantic conventions for [spans](https://github.com/open-telemetry/semantic-conventions/blob/v1.28.0/docs/database/database-spans.md). - ([#2229](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2229)) + ([#2229](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2229), + [#2277](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2277)) * **Breaking change**: The `peer.service` and `server.socket.address` attributes are no longer emitted. Users should rely on the `server.address` attribute for the same information. Note that `server.address` is only included when @@ -37,6 +38,11 @@ ([#2233](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2233)) * The `EnableConnectionLevelAttributes` option is now enabled by default. ([#2249](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2249)) +* The following attributes are now provided when starting an activity for a database + call: `db.system`, `db.name` (old conventions), `db.namespace` (new conventions), + `server.address`, and `server.port`. These attributes are now available for sampling + decisions. + ([#2277](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2277)) ## 1.9.0-beta.1 diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlActivitySourceHelper.cs b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlActivitySourceHelper.cs index 88bf750c9d..b20a2d5174 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlActivitySourceHelper.cs +++ b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlActivitySourceHelper.cs @@ -20,41 +20,72 @@ internal sealed class SqlActivitySourceHelper public static readonly AssemblyName AssemblyName = Assembly.GetName(); public static readonly string ActivitySourceName = AssemblyName.Name!; public static readonly ActivitySource ActivitySource = new(ActivitySourceName, Assembly.GetPackageVersion()); - public static readonly string ActivityName = ActivitySourceName + ".Execute"; - public static readonly IEnumerable> CreationTags = new[] + public static TagList GetTagListFromConnectionInfo(string? dataSource, string? databaseName, SqlClientTraceInstrumentationOptions options, out string activityName) { - new KeyValuePair(SemanticConventions.AttributeDbSystem, MicrosoftSqlServerDatabaseSystemName), - }; + activityName = 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 tags = new TagList { - var connectionDetails = SqlConnectionDetails.ParseFromDataSource((string)dataSource); + { SemanticConventions.AttributeDbSystem, MicrosoftSqlServerDatabaseSystemName }, + }; - // 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)) + if (options.EnableConnectionLevelAttributes && dataSource != null) + { + var connectionDetails = SqlConnectionDetails.ParseFromDataSource(dataSource); + + if (options.EmitOldAttributes && !string.IsNullOrEmpty(databaseName)) + { + tags.Add(SemanticConventions.AttributeDbName, databaseName); + activityName = databaseName!; + } + + if (options.EmitNewAttributes && !string.IsNullOrEmpty(databaseName)) + { + var dbNamespace = !string.IsNullOrEmpty(connectionDetails.InstanceName) + ? $"{connectionDetails.InstanceName}.{databaseName}" // TODO: Refactor SqlConnectionDetails to include database to avoid string allocation here. + : databaseName!; + tags.Add(SemanticConventions.AttributeDbNamespace, dbNamespace); + activityName = dbNamespace; + } + + var serverAddress = connectionDetails.ServerHostName ?? connectionDetails.ServerIpAddress; + if (!string.IsNullOrEmpty(serverAddress)) { - activity.SetTag(SemanticConventions.AttributeDbMsSqlInstanceName, connectionDetails.InstanceName); + tags.Add(SemanticConventions.AttributeServerAddress, serverAddress); + if (connectionDetails.Port.HasValue) + { + tags.Add(SemanticConventions.AttributeServerPort, connectionDetails.Port); + } + + if (activityName == MicrosoftSqlServerDatabaseSystemName) + { + activityName = connectionDetails.Port.HasValue + ? $"{serverAddress}:{connectionDetails.Port}" // TODO: Another opportunity to refactor SqlConnectionDetails + : serverAddress!; + } } - if (!string.IsNullOrEmpty(connectionDetails.ServerHostName)) + if (options.EmitOldAttributes && !string.IsNullOrEmpty(connectionDetails.InstanceName)) { - activity.SetTag(SemanticConventions.AttributeServerAddress, connectionDetails.ServerHostName); + tags.Add(SemanticConventions.AttributeDbMsSqlInstanceName, connectionDetails.InstanceName); } - else + } + else if (!string.IsNullOrEmpty(databaseName)) + { + if (options.EmitNewAttributes) { - activity.SetTag(SemanticConventions.AttributeServerAddress, connectionDetails.ServerIpAddress); + tags.Add(SemanticConventions.AttributeDbNamespace, databaseName); } - if (connectionDetails.Port.HasValue) + if (options.EmitOldAttributes) { - activity.SetTag(SemanticConventions.AttributeServerPort, connectionDetails.Port); + tags.Add(SemanticConventions.AttributeDbName, databaseName); } + + activityName = databaseName!; } + + return tags; } } diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs index b16f71402a..f0739ac7ec 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs @@ -27,8 +27,8 @@ internal sealed class SqlClientDiagnosticListener : ListenerHandler private readonly PropertyFetcher commandFetcher = new("Command"); private readonly PropertyFetcher connectionFetcher = new("Connection"); - private readonly PropertyFetcher dataSourceFetcher = new("DataSource"); - private readonly PropertyFetcher databaseFetcher = new("Database"); + private readonly PropertyFetcher dataSourceFetcher = new("DataSource"); + private readonly PropertyFetcher databaseFetcher = new("Database"); private readonly PropertyFetcher commandTypeFetcher = new("CommandType"); private readonly PropertyFetcher commandTextFetcher = new("CommandText"); private readonly PropertyFetcher exceptionFetcher = new("Exception"); @@ -50,12 +50,23 @@ public override void OnEventWritten(string name, object? payload) case SqlDataBeforeExecuteCommand: case SqlMicrosoftBeforeExecuteCommand: { - // SqlClient does not create an Activity. So the activity coming in here will be null or the root span. + _ = this.commandFetcher.TryFetch(payload, out var command); + if (command == null) + { + SqlClientInstrumentationEventSource.Log.NullPayload(nameof(SqlClientDiagnosticListener), name); + return; + } + + _ = this.connectionFetcher.TryFetch(command, out var connection); + _ = this.databaseFetcher.TryFetch(connection, out var databaseName); + _ = this.dataSourceFetcher.TryFetch(connection, out var dataSource); + + var startTags = SqlActivitySourceHelper.GetTagListFromConnectionInfo(dataSource, databaseName, this.options, out var activityName); activity = SqlActivitySourceHelper.ActivitySource.StartActivity( - SqlActivitySourceHelper.ActivityName, + activityName, ActivityKind.Client, default(ActivityContext), - SqlActivitySourceHelper.CreationTags); + startTags); if (activity == null) { @@ -63,14 +74,6 @@ public override void OnEventWritten(string name, object? payload) return; } - _ = this.commandFetcher.TryFetch(payload, out var command); - if (command == null) - { - SqlClientInstrumentationEventSource.Log.NullPayload(nameof(SqlClientDiagnosticListener), name); - activity.Stop(); - return; - } - if (activity.IsAllDataRequested) { try @@ -91,34 +94,8 @@ public override void OnEventWritten(string name, object? payload) return; } - _ = this.connectionFetcher.TryFetch(command, out var connection); - _ = this.databaseFetcher.TryFetch(connection, out var database); - - // TODO: Need to understand what scenario (if any) database will be null here - // so that we set DisplayName and db.name/db.namespace correctly. - if (database != null) - { - activity.DisplayName = (string)database; - - if (this.options.EmitOldAttributes) - { - activity.SetTag(SemanticConventions.AttributeDbName, database); - } - - if (this.options.EmitNewAttributes) - { - activity.SetTag(SemanticConventions.AttributeDbNamespace, database); - } - } - - _ = this.dataSourceFetcher.TryFetch(connection, out var dataSource); _ = this.commandTextFetcher.TryFetch(command, out var commandText); - if (dataSource != null) - { - SqlActivitySourceHelper.AddConnectionLevelDetailsToActivity((string)dataSource, activity, this.options); - } - if (this.commandTypeFetcher.TryFetch(command, out CommandType commandType)) { switch (commandType) diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs index 0b4085183a..19a6f7526d 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs @@ -112,11 +112,14 @@ private void OnBeginExecute(EventWrittenEventArgs eventData) return; } + string dataSource = (string)eventData.Payload[1]; + string databaseName = (string)eventData.Payload[2]; + var startTags = SqlActivitySourceHelper.GetTagListFromConnectionInfo(dataSource, databaseName, this.options, out var activityName); var activity = SqlActivitySourceHelper.ActivitySource.StartActivity( - SqlActivitySourceHelper.ActivityName, + activityName, ActivityKind.Client, default(ActivityContext), - SqlActivitySourceHelper.CreationTags); + startTags); if (activity == null) { @@ -124,24 +127,8 @@ private void OnBeginExecute(EventWrittenEventArgs eventData) return; } - string? databaseName = (string)eventData.Payload[2]; - - activity.DisplayName = databaseName; - if (activity.IsAllDataRequested) { - if (this.options.EmitOldAttributes) - { - activity.SetTag(SemanticConventions.AttributeDbName, databaseName); - } - - if (this.options.EmitNewAttributes) - { - activity.SetTag(SemanticConventions.AttributeDbNamespace, databaseName); - } - - SqlActivitySourceHelper.AddConnectionLevelDetailsToActivity((string)eventData.Payload[1], activity, this.options); - string commandText = (string)eventData.Payload[3]; if (!string.IsNullOrEmpty(commandText) && this.options.SetDbStatementForText) { diff --git a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTestCase.cs b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTestCase.cs new file mode 100644 index 0000000000..f2b7e07e2b --- /dev/null +++ b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTestCase.cs @@ -0,0 +1,53 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +using System.Collections; + +namespace OpenTelemetry.Instrumentation.SqlClient.Tests; + +public class SqlClientTestCase : IEnumerable +{ + public string ConnectionString { get; set; } = string.Empty; + + public string ExpectedActivityName { get; set; } = string.Empty; + + public string? ExpectedServerAddress { get; set; } + + public int? ExpectedPort { get; set; } + + public string? ExpectedDbNamespace { get; set; } + + public string? ExpectedInstanceName { get; set; } + + private static SqlClientTestCase[] TestCases => + [ + new SqlClientTestCase + { + ConnectionString = @"Data Source=SomeServer", + ExpectedActivityName = "SomeServer", + ExpectedServerAddress = "SomeServer", + ExpectedPort = null, + ExpectedDbNamespace = null, + ExpectedInstanceName = null, + }, + new SqlClientTestCase + { + ConnectionString = @"Data Source=SomeServer,1434", + ExpectedActivityName = "SomeServer:1434", + ExpectedServerAddress = "SomeServer", + ExpectedPort = 1434, + ExpectedDbNamespace = null, + ExpectedInstanceName = null, + }, + ]; + + public IEnumerator GetEnumerator() + { + foreach (var testCase in TestCases) + { + yield return new object[] { testCase }; + } + } + + IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator(); +} diff --git a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTests.cs b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTests.cs index 0328780b18..d978beed88 100644 --- a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTests.cs @@ -200,27 +200,27 @@ public void SqlClientAddsConnectionLevelAttributes( bool emitOldAttributes = true, bool emitNewAttributes = false) { - var activity = new Activity("Test Sql Activity"); var options = new SqlClientTraceInstrumentationOptions() { EnableConnectionLevelAttributes = enableConnectionLevelAttributes, EmitOldAttributes = emitOldAttributes, EmitNewAttributes = emitNewAttributes, }; - SqlActivitySourceHelper.AddConnectionLevelDetailsToActivity(dataSource, activity, options); - Assert.Equal(expectedServerHostName ?? expectedServerIpAddress, activity.GetTagValue(SemanticConventions.AttributeServerAddress)); + var tags = SqlActivitySourceHelper.GetTagListFromConnectionInfo(dataSource, databaseName: null, options, out var _); + + Assert.Equal(expectedServerHostName ?? expectedServerIpAddress, tags.FirstOrDefault(x => x.Key == SemanticConventions.AttributeServerAddress).Value); if (emitOldAttributes) { - Assert.Equal(expectedInstanceName, activity.GetTagValue(SemanticConventions.AttributeDbMsSqlInstanceName)); + Assert.Equal(expectedInstanceName, tags.FirstOrDefault(x => x.Key == SemanticConventions.AttributeDbMsSqlInstanceName).Value); } else { - Assert.Null(activity.GetTagValue(SemanticConventions.AttributeDbMsSqlInstanceName)); + Assert.Null(tags.FirstOrDefault(x => x.Key == SemanticConventions.AttributeDbMsSqlInstanceName).Value); } - Assert.Equal(expectedPort, activity.GetTagValue(SemanticConventions.AttributeServerPort)); + Assert.Equal(expectedPort, tags.FirstOrDefault(x => x.Key == SemanticConventions.AttributeServerPort).Value); } [Theory] @@ -291,27 +291,17 @@ public void SqlClientErrorsAreCollectedSuccessfully(string beforeCommand, string } [Theory] - [InlineData(SqlClientDiagnosticListener.SqlDataBeforeExecuteCommand)] - [InlineData(SqlClientDiagnosticListener.SqlMicrosoftBeforeExecuteCommand)] - public void SqlClientCreatesActivityWithDbSystem( - string beforeCommand) + [ClassData(typeof(SqlClientTestCase))] + public void SqlDataStartsActivityWithExpectedAttributes(SqlClientTestCase testCase) { - using var sqlConnection = new SqlConnection(TestConnectionString); - using var sqlCommand = sqlConnection.CreateCommand(); - - var sampler = new TestSampler - { - SamplingAction = _ => new SamplingResult(SamplingDecision.Drop), - }; - using (Sdk.CreateTracerProviderBuilder() - .AddSqlClientInstrumentation() - .SetSampler(sampler) - .Build()) - { - this.fakeSqlClientDiagnosticSource.Write(beforeCommand, new { }); - } + this.RunSqlClientTestCase(testCase, SqlClientDiagnosticListener.SqlDataBeforeExecuteCommand, SqlClientDiagnosticListener.SqlDataAfterExecuteCommand); + } - VerifySamplingParameters(sampler.LatestSamplingParameters); + [Theory] + [ClassData(typeof(SqlClientTestCase))] + public void MicrosoftDataStartsActivityWithExpectedAttributes(SqlClientTestCase testCase) + { + this.RunSqlClientTestCase(testCase, SqlClientDiagnosticListener.SqlMicrosoftBeforeExecuteCommand, SqlClientDiagnosticListener.SqlMicrosoftAfterExecuteCommand); } [Fact] @@ -384,7 +374,15 @@ internal static void VerifyActivityData( bool emitOldAttributes = true, bool emitNewAttributes = false) { - Assert.Equal("master", activity.DisplayName); + if (emitNewAttributes) + { + Assert.Equal("MSSQLLocalDB.master", activity.DisplayName); + } + else + { + Assert.Equal("master", activity.DisplayName); + } + Assert.Equal(ActivityKind.Client, activity.Kind); if (!isFailure) @@ -429,7 +427,7 @@ internal static void VerifyActivityData( if (emitNewAttributes) { - Assert.Equal("master", activity.GetTagValue(SemanticConventions.AttributeDbNamespace)); + Assert.Equal("MSSQLLocalDB.master", activity.GetTagValue(SemanticConventions.AttributeDbNamespace)); } switch (commandType) @@ -488,6 +486,60 @@ internal static void VerifySamplingParameters(SamplingParameters samplingParamet && (string)kvp.Value == SqlActivitySourceHelper.MicrosoftSqlServerDatabaseSystemName); } + internal static void VerifySamplingParameters(SqlClientTestCase testCase, Activity activity, SamplingParameters samplingParameters) + { + Assert.NotNull(samplingParameters.Tags); + + Assert.Equal(testCase.ExpectedActivityName, activity.DisplayName); + Assert.Equal(SqlActivitySourceHelper.MicrosoftSqlServerDatabaseSystemName, activity.GetTagItem(SemanticConventions.AttributeDbSystem)); + Assert.Equal(testCase.ExpectedDbNamespace, activity.GetTagItem(SemanticConventions.AttributeDbName)); + Assert.Equal(testCase.ExpectedServerAddress, activity.GetTagItem(SemanticConventions.AttributeServerAddress)); + Assert.Equal(testCase.ExpectedPort, activity.GetTagItem(SemanticConventions.AttributeServerPort)); + Assert.Equal(testCase.ExpectedInstanceName, activity.GetTagItem(SemanticConventions.AttributeDbMsSqlInstanceName)); + + Assert.Contains( + samplingParameters.Tags, + kvp => kvp.Key == SemanticConventions.AttributeDbSystem + && kvp.Value is string + && (string)kvp.Value == SqlActivitySourceHelper.MicrosoftSqlServerDatabaseSystemName); + + if (testCase.ExpectedDbNamespace != null) + { + Assert.Contains( + samplingParameters.Tags, + kvp => kvp.Key == SemanticConventions.AttributeDbName + && kvp.Value is string + && (string)kvp.Value == testCase.ExpectedDbNamespace); + } + + if (testCase.ExpectedServerAddress != null) + { + Assert.Contains( + samplingParameters.Tags, + kvp => kvp.Key == SemanticConventions.AttributeServerAddress + && kvp.Value is string + && (string)kvp.Value == testCase.ExpectedServerAddress); + } + + if (testCase.ExpectedPort.HasValue) + { + Assert.Contains( + samplingParameters.Tags, + kvp => kvp.Key == SemanticConventions.AttributeServerPort + && kvp.Value is int + && (int)kvp.Value == testCase.ExpectedPort); + } + + if (testCase.ExpectedInstanceName != null) + { + Assert.Contains( + samplingParameters.Tags, + kvp => kvp.Key == SemanticConventions.AttributeDbMsSqlInstanceName + && kvp.Value is string + && (string)kvp.Value == testCase.ExpectedInstanceName); + } + } + internal static void ActivityEnrichment(Activity activity, string method, object obj) { activity.SetTag("enriched", "yes"); @@ -504,6 +556,32 @@ internal static void ActivityEnrichment(Activity activity, string method, object } #if !NETFRAMEWORK + private void RunSqlClientTestCase(SqlClientTestCase testCase, string beforeCommand, string afterCommand) + { + using var sqlConnection = new SqlConnection(testCase.ConnectionString); + using var sqlCommand = sqlConnection.CreateCommand(); + + var exportedItems = new List(); + + var sampler = new TestSampler + { + SamplingAction = _ => new SamplingResult(SamplingDecision.RecordAndSample), + }; + + using (Sdk.CreateTracerProviderBuilder() + .AddSqlClientInstrumentation() + .SetSampler(sampler) + .AddInMemoryExporter(exportedItems) + .Build()) + { + this.fakeSqlClientDiagnosticSource.Write(beforeCommand, new { Command = sqlCommand }); + this.fakeSqlClientDiagnosticSource.Write(afterCommand, new { Command = sqlCommand }); + } + + Assert.Single(exportedItems); + VerifySamplingParameters(testCase, exportedItems.First(), sampler.LatestSamplingParameters); + } + private Activity[] RunCommandWithFilter( Action sqlCommandSetup, Func filter) diff --git a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlEventSourceTests.netfx.cs b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlEventSourceTests.netfx.cs index 1d1c234c85..2280e5502f 100644 --- a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlEventSourceTests.netfx.cs +++ b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlEventSourceTests.netfx.cs @@ -243,7 +243,15 @@ private static void VerifyActivityData( bool emitOldAttributes = true, bool emitNewAttributes = false) { - Assert.Equal("master", activity.DisplayName); + if (emitNewAttributes && enableConnectionLevelAttributes) + { + Assert.Equal("instanceName.master", activity.DisplayName); + } + else + { + Assert.Equal("master", activity.DisplayName); + } + Assert.Equal(ActivityKind.Client, activity.Kind); Assert.Equal(SqlActivitySourceHelper.MicrosoftSqlServerDatabaseSystemName, activity.GetTagValue(SemanticConventions.AttributeDbSystem)); @@ -282,7 +290,7 @@ private static void VerifyActivityData( if (emitNewAttributes) { - Assert.Equal("master", activity.GetTagValue(SemanticConventions.AttributeDbNamespace)); + Assert.Equal(!enableConnectionLevelAttributes ? "master" : "instanceName.master", activity.GetTagValue(SemanticConventions.AttributeDbNamespace)); } if (captureText)