Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SqlClient] Include instance in db.namespace and activity name, start activity with required tags #2277

Merged
merged 5 commits into from
Oct 30, 2024
+247 −107
Merged

[SqlClient] Include instance in db.namespace and activity name, start activity with required tags #2277

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ae7c672
Start activity with required tags and name that includes the instance
alanwest Oct 30, 2024
6731ac3
Update changelog
alanwest Oct 30, 2024
3a87e3a
Update changelog
alanwest Oct 30, 2024
af4922c
Fix formatting
alanwest Oct 30, 2024
6a62be6
Merge branch 'main' into alanwest/database-namespace
alanwest Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down
71 changes: 51 additions & 20 deletions src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlActivitySourceHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<KeyValuePair<string, object?>> CreationTags = new[]
public static TagList GetTagListFromConnectionInfo(string? dataSource, string? databaseName, SqlClientTraceInstrumentationOptions options, out string activityName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TagList is a sizeable struct. It would probably be faster to return by reference a la:

-public static TagList GetTagListFromConnectionInfo(
+public static void GetTagListFromConnectionInfo(
    string? dataSource,
    string? databaseName,
    SqlClientTraceInstrumentationOptions options,
-   out string activityName)
+   out string activityName,
+   out TagList tags)

Probably the name sucks too 🤣 GetSamplerTagsFromConnectionInfo 🤷

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably the name sucks too 🤣 GetSamplerTagsFromConnectionInfo

Yea I agree. I'm gonna leave it sucky for now cause I don't know what to call it yet. This code may continue to be refactored a bit once I integrate metric instrumentation which will also need what this method is doing.

{
new KeyValuePair<string, object?>(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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple TODOs here. I'm considering adding database name to the SqlConnectionDetails that gets cached. This could mean though that the cache could be a little bigger in the event that multiple databases were accessed for a given instance.

: 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;
}
}
55 changes: 16 additions & 39 deletions src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ internal sealed class SqlClientDiagnosticListener : ListenerHandler

private readonly PropertyFetcher<object> commandFetcher = new("Command");
private readonly PropertyFetcher<object> connectionFetcher = new("Connection");
private readonly PropertyFetcher<object> dataSourceFetcher = new("DataSource");
private readonly PropertyFetcher<object> databaseFetcher = new("Database");
private readonly PropertyFetcher<string> dataSourceFetcher = new("DataSource");
private readonly PropertyFetcher<string> databaseFetcher = new("Database");
private readonly PropertyFetcher<CommandType> commandTypeFetcher = new("CommandType");
private readonly PropertyFetcher<object> commandTextFetcher = new("CommandText");
private readonly PropertyFetcher<Exception> exceptionFetcher = new("Exception");
Expand All @@ -50,27 +50,30 @@ 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)
{
// There is no listener or it decided not to sample the current request.
return;
}

_ = this.commandFetcher.TryFetch(payload, out var command);
if (command == null)
{
SqlClientInstrumentationEventSource.Log.NullPayload(nameof(SqlClientDiagnosticListener), name);
activity.Stop();
return;
}

if (activity.IsAllDataRequested)
{
try
Expand All @@ -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)
Expand Down
23 changes: 5 additions & 18 deletions src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,36 +112,23 @@ 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that StartActivity doesn't have an overload which accepts TagList. We should ask @tarekgh for one 🤣

In the meantime, maybe don't use TagList at all. Because this will box it up.

A ThreadStatic is probably fine we did that here:

opentelemetry-dotnet-contrib/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/ActivityHelper.cs

Lines 30 to 31 in 0351a0b

[ThreadStatic]
private static KeyValuePair<string, object?>[]? cachedTagsStorage;

Copy link
Member Author

@alanwest alanwest Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I was bummed there was no StartActivity with TagList as well 😢.

A ThreadStatic is probably fine we did that here

I had a similar thought to use ThreadStatic. Ultimately, these tags will need to be shared by the metric instrumentation as well. A ThreadStatic would allow us to capture them on command start and then use them on command end when recording the metric. I'll try this in a follow up.

Copy link

@tarekgh tarekgh Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that StartActivity doesn't have an overload which accepts TagList. We should ask @tarekgh for one 🤣

You may update dotnet/runtime#103245 (comment) with whatever request. the runtime issue was talking about CreateActivity but can include StartActivity too.


if (activity == null)
{
// There is no listener or it decided not to sample the current request.
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)
{
Expand Down
53 changes: 53 additions & 0 deletions test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTestCase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<object[]>
{
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<object[]> GetEnumerator()
{
foreach (var testCase in TestCases)
{
yield return new object[] { testCase };
}
}

IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator();
}
Loading
Loading