SqlClient instrumentation to support Microsoft.Data.SqlClient EventSource #1599

mbakalov · 2020-11-20T12:48:17Z

Changes

SqlClientInstrumentationOptions now has different public API on .NET Core and .NET Framework
Made SqlClientDiagnosticListener .NET Core-only (it was referencing the options properties no longer available on .NET Framework)
- For that had to refactor some common properties into a new SqlActivitySourceHelper
- DiagnosticListener is no longer created/used under .NET Framework
On .NET Framework the new SetStatementText is false by default
SqlEventSourceListener is now listening to both event sources ("Microsoft-AdoNet-SystemData" and "Microsoft.Data.SqlClient.EventSource")
Updated Microsoft.Data.SqlClient NuGet in the Test project from 1.1.1 to 2.1.0

Background

Recently, Microsoft.Data.SqlClient renamed their EventSource from "Microsoft-AdoNet-SystemData" to "Microsoft.Data.SqlClient.EventSource". The System.Data.SqlClient still uses the old event source, so the instrumentation needs to listen to both event sources.

TODO:

unit-tests, including "fake event source" ones to have a 2nd fake source
behavior change: new EventSource reports CommandText not only for StoredProcedures now but for Text statements as well
comments and README updates
CHANGELOG.md updated for non-trivial changes
* [ ] Changes in public API reviewed - shouldn't be any public API changes
Changes in public API reviewed - there are public API changes for SetStatementText

codecov · 2020-11-20T12:58:48Z

Codecov Report

Merging #1599 (6511d3b) into master (04f35ef) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1599   +/-   ##
=======================================
  Coverage   80.56%   80.56%           
=======================================
  Files         242      243    +1     
  Lines        6582     6582           
=======================================
  Hits         5303     5303           
  Misses       1279     1279

Impacted Files	Coverage Δ
...ation.SqlClient/SqlClientInstrumentationOptions.cs	`97.95% <ø> (ø)`
...qlClient/Implementation/SqlActivitySourceHelper.cs	`100.00% <100.00%> (ø)`
...ient/Implementation/SqlClientDiagnosticListener.cs	`75.75% <100.00%> (-0.72%)`	⬇️
...trumentation.SqlClient/SqlClientInstrumentation.cs	`100.00% <100.00%> (ø)`
...ation.SqlClient/TracerProviderBuilderExtensions.cs	`100.00% <100.00%> (ø)`

cijothomas

LGTM. Please add an entry in chagelog.md.
We need to update tests to use mds as well rigth?

mbakalov · 2020-11-20T16:57:27Z

LGTM. Please add an entry in chagelog.md.
We need to update tests to use mds as well rigth?

@cijothomas - thanks, yes, working on that still, will push later today. The semantics of the new EventSource are different a bit too (it now captures CommandText always, not just for SPs) so I want to make sure that is supported properly as well.

mbakalov · 2020-11-20T18:30:26Z

Actually, annoyingly that behavior is different enough that it is going to result in confusing API.

System.Data.SqlClient basically does this:

private void WriteBeginExecuteEvent()
{
    if (SqlEventSource.Log.IsEnabled() && Connection != null)
    {
        string commandText = CommandType == CommandType.StoredProcedure ? CommandText : string.Empty;
        SqlEventSource.Log.BeginExecute(GetHashCode(), Connection.DataSource, Connection.Database, commandText);
    }
}

While MDS does this:

private void WriteBeginExecuteEvent()
{
    if (Connection != null)
    {
        SqlClientEventSource.Log.BeginExecute(GetHashCode(), Connection.DataSource, Connection.Database, CommandText);
    }
}

So MDS always reports CommandText which is great, but now in our listener we cannot tell if we are dealing with an SP or a Text statement. Previously we could say "CommandText is non-empty so it has to be an SP", now we can't.

This becomes confusing because the "options" has two separate settings for those: SetTextCommandContent and SetStoredProcedureCommandName (currently SetTextCommandContent does nothing on System.Data.SqlClient netfx).

To clean this up, I am suggesting the following in the SqlClientInstrumentationOptions API, basically have only one toggle that does both (but only on netfx). Any thoughts?

        // .NET Framework implementation uses SqlEventSource from which we can't reliably distinguish
        // StoredProcedures from regular Text sql commands.
#if NETFRAMEWORK

        /// <summary>
        /// Gets or sets a value indicating whether or not the <see cref="SqlClientInstrumentation"/> should
        /// add the text of the executed Sql commands as the <see cref="SemanticConventions.AttributeDbStatement"/> tag.
        /// Default value: False.
        /// </summary>
        public bool SetStatementText { get; set; }
#else
        /// <summary>
        /// Gets or sets a value indicating whether or not the <see cref="SqlClientInstrumentation"/> should add the names of <see cref="CommandType.StoredProcedure"/> commands as the <see cref="SemanticConventions.AttributeDbStatement"/> tag. Default value: True.
        /// </summary>
        public bool SetStoredProcedureCommandName { get; set; } = true;

        /// <summary>
        /// Gets or sets a value indicating whether or not the <see cref="SqlClientInstrumentation"/> should add the text of <see cref="CommandType.Text"/> commands as the <see cref="SemanticConventions.AttributeDbStatement"/> tag. Default value: False.
        /// </summary>
        public bool SetTextCommandContent { get; set; }
#endif

mbakalov · 2020-11-20T18:36:23Z

Alternatively, the instrumentation can set db.statement on netfx MDS only if both SetStoredProcedureCommandName and SetTextCommandContent are set to true. Less code and less ifdefs in the codebase (especially tests), but might be more confusing to users?

cijothomas · 2020-11-20T18:38:13Z

Alternatively, the instrumentation can set db.statement on netfx MDS only if both SetStoredProcedureCommandName and SetTextCommandContent are set to true. Less code and less ifdefs in the codebase (especially tests), but might be more confusing to users?

Can we instead use separate listeners for MDS and SDS? Then listener know exactly if its getting sp or sqltext.
Sorry haven't fully read the code. but an initial thought.

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

mbakalov · 2020-11-20T18:42:55Z

Alternatively, the instrumentation can set db.statement on netfx MDS only if both SetStoredProcedureCommandName and SetTextCommandContent are set to true. Less code and less ifdefs in the codebase (especially tests), but might be more confusing to users?

Can we instead use separate listeners for MDS and SDS? Then listener know exactly if its getting sp or sqltext.
Sorry haven't fully read the code. but an initial thought.

Yeah that is the issue I think. Even if MDS was a separate listener, it just gets a CommandText and at that point has no way of telling if it is sp or sqltext.

CodeBlanch · 2020-11-20T19:00:20Z

@mbakalov Bummer about the change to command text by MS! I like your idea to change the options.

Separate SqlClientInstrumentationOptions for netcore and netfx. Upgrade MDS nuget to the newer version that actually uses the new EventSource name. Don't reference the diagnostic listener from the sql even source code.

mbakalov · 2020-11-21T00:47:00Z

Introduced a separate SetStatementText property just on .NET Framework (had to refactor code just a bit to get there) - please take a look.

It turned out that the reason net461 tests were failing was only somewhat related to the renaming of the EventSource. The test project was referencing MDS nuget version 1.1.1, which actually still uses the old EventSource name :), so the tests should have worked. And they mostly did, but were flaky for a reason similar to what was being discussed in elastic/apm-agent-dotnet#704 (comment).

When running both System.Data.SqlClient and Microsoft.Data.SqlClient tests in the same session, the OnEventSourceCreated was firing multiple types during a single test, overwriting the eventSource we already enabled and causing issues. Running just System.Data.SqlClient or just Microsoft.Data.SqlClient tests under net461 has always worked ok.

I upgraded the MDS nuget to the latest version (2.1.0) and now with the support of the new EventSource all tests pass.

I did a CHANGELOG.md update and a few minor comment clarifications and fixes. Still need to add a few more new tests and update README.

Capturing doesn't depend on commandtype.

mbakalov · 2020-11-21T19:34:47Z

Highlights of changes:

SqlClientInstrumentationOptions now has different public API on .NET Core and .NET Framework
Made SqlClientDiagnosticListener .NET Core-only (it was referencing the options properties no longer available on .NET Framework)
- For that had to refactor some common properties into a new SqlActivitySourceHelper
- DiagnosticListener is no longer created/used under .NET Framework
On .NET Framework the new SetStatementText is true by default and will capture both sp and sqltext if available. Not sure if good idea or not. now false by default (EDIT).
SqlEventSourceListener is now listening to both event sources
Updated Microsoft.Data.SqlClient NuGet in the Test project from 1.1.1 to 2.1.0

src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs

cijothomas · 2020-11-23T18:46:33Z

@mbakalov Thanks a lot!
LGTM overall. except we need to keep default as false. Given this bug fix requires API change, its okay to be merged before 1.0.0. We need 2 approvals as we are close to 1.0.0. @CodeBlanch - please share your review.

ALso, open to any other suggestion for the names for these:
.NET Fx
SetStatementText

.NET core
SetStoredProcedureCommandName
SetTextCommandContent

mbakalov · 2020-11-23T19:06:27Z

Thanks @cijothomas, I wasn't sure about the default, will update to false!

mbakalov · 2020-11-23T19:08:00Z

As for the property names, maybe:

netfx
- SetCommandText
netcore
- SetStoredProcedureCommandText
- SetTextCommandText

…try-dotnet into mds-netfx

…-dotnet into mds-netfx

mbakalov · 2020-11-23T23:42:15Z

Changed the default to false. Added more comments and warnings just in case. Let me know if need to update/reword anything there to make more clear.

cijothomas · 2020-11-24T02:15:01Z

For namings, alternate proposal:

netfx
SetDbStatement
netcore
SetDbStatementForStoredProcedure
SetDbStatementForText

"Sets DBStatement semantic convention" can be the general comment for all, as its coming directly from spec. Then specifically clarify SetDbStatement could be SPName or Text. SetDbStatementForStoredProcedure and SetDbStatementForText can be similarly clarified.

CodeBlanch · 2020-11-24T18:05:28Z

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

+        public const string ActivitySourceName = "OpenTelemetry.SqlClient";
+        public const string ActivityName = ActivitySourceName + ".Execute";
+
+        public const string MicrosoftSqlServerDatabaseSystemName = "mssql";


nit: Might be better to move this definition to semantic conventions, that's where it came from (https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md#connection-level-attributes).

Thanks, I'll update this! Other things ("db.statement") are being pulled from semantic conventions so better to be consistent.

...lemetry.Instrumentation.SqlClient.Tests/OpenTelemetry.Instrumentation.SqlClient.Tests.csproj

CodeBlanch

Left a couple of comments, but LGTM.

cijothomas · 2020-11-24T19:16:44Z

Good to merge as has enough approvals, and no blocking comments. We just need to see if any better names are possible for the options.

Listen to MDS EvenSource as well

3cc6571

cijothomas reviewed Nov 20, 2020

View reviewed changes

CodeBlanch reviewed Nov 20, 2020

View reviewed changes

src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs Outdated Show resolved Hide resolved

cijothomas added the pr:do-not-merge label Nov 20, 2020

mbakalov added 3 commits November 20, 2020 18:31

Use SqlEventSourceListener on NETFRAMEWORK only

af46235

Separate SqlClientInstrumentationOptions for netcore and netfx. Upgrade MDS nuget to the newer version that actually uses the new EventSource name. Don't reference the diagnostic listener from the sql even source code.

Better comments

7437183

CHANGELOG updates + minor code clarity

2ef7a4d

mbakalov and others added 5 commits November 21, 2020 11:06

Run EventSource tests for MDS source as well

12b5c1e

Default capture text to true

af7ca38

Capturing doesn't depend on commandtype.

Update readme for new SetStatementText behavior

3ec91f2

Minor md formatting fix

4d90613

Merge branch 'master' into mds-netfx

db50f01

mbakalov marked this pull request as ready for review November 21, 2020 18:32

mbakalov requested a review from a team November 21, 2020 18:32

Merge branch 'master' into mds-netfx

1d0b911

cijothomas reviewed Nov 23, 2020

View reviewed changes

src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs Outdated Show resolved Hide resolved

Merge branch 'master' into mds-netfx

fc73c50

Default command text capture to false

c2e5e69

mbakalov added 3 commits November 23, 2020 18:28

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

6f5a2ac

…try-dotnet into mds-netfx

Merge branch 'mds-netfx' of https://github.com/mbakalov/opentelemetry…

4cb6823

…-dotnet into mds-netfx

Markdown cleanup

3735142

cijothomas approved these changes Nov 24, 2020

View reviewed changes

CodeBlanch reviewed Nov 24, 2020

View reviewed changes

...lemetry.Instrumentation.SqlClient.Tests/OpenTelemetry.Instrumentation.SqlClient.Tests.csproj Show resolved Hide resolved

CodeBlanch approved these changes Nov 24, 2020

View reviewed changes

Merge branch 'master' into mds-netfx

9e975c8

cijothomas removed the pr:do-not-merge label Nov 24, 2020

Merge branch 'master' into mds-netfx

6511d3b

cijothomas merged commit 9e26699 into open-telemetry:master Nov 24, 2020

mbakalov mentioned this pull request Dec 6, 2020

SqlClient public API renaming proposal for db.statement properties #1645

Merged

1 task

mbakalov deleted the mds-netfx branch February 26, 2021 03:17

mbakalov mentioned this pull request Mar 3, 2021

Make SQL settings available unconditionally #1864

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SqlClient instrumentation to support Microsoft.Data.SqlClient EventSource #1599

SqlClient instrumentation to support Microsoft.Data.SqlClient EventSource #1599

mbakalov commented Nov 20, 2020 •

edited

Loading

codecov bot commented Nov 20, 2020 •

edited

Loading

cijothomas left a comment

mbakalov commented Nov 20, 2020

mbakalov commented Nov 20, 2020

mbakalov commented Nov 20, 2020

cijothomas commented Nov 20, 2020

mbakalov commented Nov 20, 2020

CodeBlanch commented Nov 20, 2020

mbakalov commented Nov 21, 2020 •

edited

Loading

mbakalov commented Nov 21, 2020 •

edited

Loading

cijothomas commented Nov 23, 2020

mbakalov commented Nov 23, 2020

mbakalov commented Nov 23, 2020

mbakalov commented Nov 23, 2020

cijothomas commented Nov 24, 2020

CodeBlanch Nov 24, 2020

mbakalov Nov 24, 2020

CodeBlanch left a comment

cijothomas commented Nov 24, 2020

SqlClient instrumentation to support Microsoft.Data.SqlClient EventSource #1599

SqlClient instrumentation to support Microsoft.Data.SqlClient EventSource #1599

Conversation

mbakalov commented Nov 20, 2020 • edited Loading

Changes

Background

codecov bot commented Nov 20, 2020 • edited Loading

Codecov Report

cijothomas left a comment

Choose a reason for hiding this comment

mbakalov commented Nov 20, 2020

mbakalov commented Nov 20, 2020

mbakalov commented Nov 20, 2020

cijothomas commented Nov 20, 2020

mbakalov commented Nov 20, 2020

CodeBlanch commented Nov 20, 2020

mbakalov commented Nov 21, 2020 • edited Loading

mbakalov commented Nov 21, 2020 • edited Loading

cijothomas commented Nov 23, 2020

mbakalov commented Nov 23, 2020

mbakalov commented Nov 23, 2020

mbakalov commented Nov 23, 2020

cijothomas commented Nov 24, 2020

CodeBlanch Nov 24, 2020

Choose a reason for hiding this comment

mbakalov Nov 24, 2020

Choose a reason for hiding this comment

CodeBlanch left a comment

Choose a reason for hiding this comment

cijothomas commented Nov 24, 2020

mbakalov commented Nov 20, 2020 •

edited

Loading

codecov bot commented Nov 20, 2020 •

edited

Loading

mbakalov commented Nov 21, 2020 •

edited

Loading

mbakalov commented Nov 21, 2020 •

edited

Loading