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

alanwest · 2024-10-30T17:30:09Z

Fixes #2236
Towards #2224
Towards #2237

[3]: When connected to a default instance, db.namespace SHOULD be set to the name of the database. When connected to a named instance, db.namespace SHOULD be set to the combination of instance and database name following the {instance_name}.{database_name} pattern.

In addition to including the instance name in the db.namespace attribute, this PR begins to address conforming the activity name to the conventions and starting the activity with required tags.

It includes db.namespace and server.address/server.port in the activity name when appropriate.
It adds the following attributes when starting the activity
- db.system
- db.namespace
- server.address
- server.port

codecov · 2024-10-30T17:32:45Z

Codecov Report

Attention: Patch coverage is 96.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.90%. Comparing base (71655ce) to head (6a62be6).
Report is 567 commits behind head on main.

Files with missing lines	Patch %	Lines
...ient/Implementation/SqlClientDiagnosticListener.cs	83.33%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2277       +/-   ##
===========================================
+ Coverage   73.91%   90.90%   +16.99%     
===========================================
  Files         267        8      -259     
  Lines        9615      308     -9307     
===========================================
- Hits         7107      280     -6827     
+ Misses       2508       28     -2480

Flag	Coverage Δ
unittests-Instrumentation.SqlClient	`90.90% <96.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...qlClient/Implementation/SqlActivitySourceHelper.cs	`100.00% <100.00%> (ø)`
...ent/Implementation/SqlEventSourceListener.netfx.cs	`87.50% <100.00%> (ø)`
...ient/Implementation/SqlClientDiagnosticListener.cs	`83.54% <83.33%> (ø)`

... and 269 files with indirect coverage changes

alanwest · 2024-10-30T17:42:49Z

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

+    private static SqlClientTestCase[] TestCases =>
+    [
+        new SqlClientTestCase
+            {
+                ConnectionString = @"Data Source=SomeServer",


I will be expanding these test cases in follow up PRs, but wanted to model in this PR what I plan to do.

I find the InlineData attributes onerous when they include a lot of parameters. It's more code, but I prefer creating a class to represent complex test cases. I find it easier to read and understand the cases covered.

alanwest · 2024-10-30T18:00:12Z

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

+            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.


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.

CodeBlanch · 2024-10-30T18:17:56Z

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


-    public static readonly IEnumerable<KeyValuePair<string, object?>> CreationTags = new[]
+    public static TagList GetTagListFromConnectionInfo(string? dataSource, string? databaseName, SqlClientTraceInstrumentationOptions options, out string activityName)


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 🤷

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.

CodeBlanch · 2024-10-30T18:21:42Z

src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs

            ActivityKind.Client,
            default(ActivityContext),
-            SqlActivitySourceHelper.CreationTags);
+            startTags);


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;

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.

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.

CodeBlanch

Some feedback but LGTM

Start activity with required tags and name that includes the instance

ae7c672

alanwest requested a review from a team as a code owner October 30, 2024 17:30

github-actions bot added the comp:instrumentation.sqlclient Things related to OpenTelemetry.Instrumentation.SqlClient label Oct 30, 2024

alanwest changed the title ~~Start activity with required tags and name that includes the instance~~ [SqlClient] Include instance in db.namespace and activity name, start activity with required tags Oct 30, 2024

alanwest added 2 commits October 30, 2024 10:38

Update changelog

6731ac3

Update changelog

3a87e3a

alanwest commented Oct 30, 2024

View reviewed changes

Fix formatting

af4922c

alanwest commented Oct 30, 2024

View reviewed changes

Merge branch 'main' into alanwest/database-namespace

6a62be6

CodeBlanch reviewed Oct 30, 2024

View reviewed changes

CodeBlanch approved these changes Oct 30, 2024

View reviewed changes

alanwest merged commit 72d1030 into open-telemetry:main Oct 30, 2024
56 checks passed

alanwest deleted the alanwest/database-namespace branch October 30, 2024 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alanwest commented Oct 30, 2024

codecov bot commented Oct 30, 2024 •

edited

Loading

alanwest Oct 30, 2024 •

edited

Loading

alanwest Oct 30, 2024

CodeBlanch Oct 30, 2024

alanwest Oct 30, 2024

CodeBlanch Oct 30, 2024

alanwest Oct 30, 2024 •

edited

Loading

tarekgh Oct 30, 2024 •

edited

Loading

CodeBlanch left a comment


		public static readonly IEnumerable<KeyValuePair<string, object?>> CreationTags = new[]
		public static TagList GetTagListFromConnectionInfo(string? dataSource, string? databaseName, SqlClientTraceInstrumentationOptions options, out string activityName)

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

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

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

Conversation

alanwest commented Oct 30, 2024

codecov bot commented Oct 30, 2024 • edited Loading

Codecov Report

alanwest Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

alanwest Oct 30, 2024

Choose a reason for hiding this comment

CodeBlanch Oct 30, 2024

Choose a reason for hiding this comment

alanwest Oct 30, 2024

Choose a reason for hiding this comment

CodeBlanch Oct 30, 2024

Choose a reason for hiding this comment

alanwest Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

tarekgh Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

CodeBlanch left a comment

Choose a reason for hiding this comment

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

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

codecov bot commented Oct 30, 2024 •

edited

Loading

alanwest Oct 30, 2024 •

edited

Loading

alanwest Oct 30, 2024 •

edited

Loading

tarekgh Oct 30, 2024 •

edited

Loading