Add diagnostics support #493

yang-xiaodong · 2018-05-03T08:03:39Z

Add diagnostics support for MySqlConnection, MySqlCommand, MySqlTransaction

Related issues: Instrument with DiagnosticSource #471

bgrainger · 2018-05-03T20:06:29Z

See similar npgsql PR: npgsql/npgsql#1910

bgrainger · 2018-05-03T20:12:16Z

src/MySqlConnector/Core/DiagnosticListenerExtensions.cs

+	/// </summary>
+	internal static class DiagnosticListenerExtensions
+	{
+		public const string DiagnosticListenerName = "MySqlClientDiagnosticListener";


Per the guide:

DO NOT - name the listener after the Listener (thus something like System.Net.HttpDiagnosticListener is bad).

MySqlConnector seems like the right name for the diagnostic listener.

As @roji wrote on his PR, separate DiagnosticListeners for Connection, Transaction, and Command seem to fit the guidelines better.

https://github.com/npgsql/npgsql/pull/1910/files#r185728056

bgrainger · 2018-05-03T20:12:37Z

src/MySqlConnector/Core/DiagnosticListenerExtensions.cs

+	{
+		public const string DiagnosticListenerName = "MySqlClientDiagnosticListener";
+
+		private const string MySqlClientPrefix = "System.Data.MySqlClient.";


Drop this prefix.

DO - keep the names reasonably short (< 16 characters). Keep in mind that event names are already qualified by the Listener so the name only needs to be unique within a listener.

bgrainger · 2018-05-03T20:13:44Z

src/MySqlConnector/Core/DiagnosticListenerExtensions.cs

+		private const string MySqlClientPrefix = "System.Data.MySqlClient.";
+
+		public const string SqlBeforeExecuteCommand = MySqlClientPrefix + nameof(WriteCommandBefore);
+		public const string SqlAfterExecuteCommand = MySqlClientPrefix + nameof(WriteCommandAfter);


The guide recommends "Start" and "Stop", not "Before" and "After".

DO - use the 'Start' and 'Stop' suffixes for events that define an interval of time.

bgrainger · 2018-05-03T20:42:12Z

Before I review this PR too much further, I added a comment about the general approach here: npgsql/npgsql#1910 (comment)

yang-xiaodong · 2018-05-04T03:41:35Z

Benchmark Report：

OS : Windows 10
CPU : Intel i5-7500
Memory : 8G
MySql version : 5.7.2

Concurrency Testing

dotnet run concurrency 10 100 10 --framework netcoreapp2.0

Add Diagnostics Before:

Test                     Insert One
Iterations:              10
Concurrency:             100
Operations:              10
Times (Min, Average, Max) 00:00:00.3141075, 00:00:00.3611378, 00:00:00.4516936

Records Inserted: 10000

Test                     Select Ten
Iterations:              10
Concurrency:             100
Operations:              10
Times (Min, Average, Max) 00:00:00.0541539, 00:00:00.1100442, 00:00:00.2744543

Records Selected: 100000
First Record: Title 10000

Test                     Sleep 1ms
Iterations:              10
Concurrency:             100
Operations:              10
Times (Min, Average, Max) 00:00:00.0392744, 00:00:00.0448056, 00:00:00.0486941

Total Sleep Commands: 10000

Add Diagnostics After(Disable Listener):

Test                     Insert One
Iterations:              10
Concurrency:             100
Operations:              10
Times (Min, Average, Max) 00:00:00.3179845, 00:00:00.3408534, 00:00:00.4055424

Records Inserted: 10000

Test                     Select Ten
Iterations:              10
Concurrency:             100
Operations:              10
Times (Min, Average, Max) 00:00:00.0456241, 00:00:00.0527765, 00:00:00.0607766

Records Selected: 100000
First Record: Title 10000

Test                     Sleep 1ms
Iterations:              10
Concurrency:             100
Operations:              10
Times (Min, Average, Max) 00:00:00.0381915, 00:00:00.0459215, 00:00:00.0541452

Total Sleep Commands: 10000

Add Diagnostics After(Enabled Listener):

Test                     Insert One
Iterations:              10
Concurrency:             100
Operations:              10
Times (Min, Average, Max) 00:00:00.3221905, 00:00:00.3545479, 00:00:00.4463972

Records Inserted: 10000

Test                     Select Ten
Iterations:              10
Concurrency:             100
Operations:              10
Times (Min, Average, Max) 00:00:00.0454466, 00:0
8000
0:00.0514560, 00:00:00.0657003

Records Selected: 100000
First Record: Title 10000

Test                     Sleep 1ms
Iterations:              10
Concurrency:             100
Operations:              10
Times (Min, Average, Max) 00:00:00.0392514, 00:00:00.0475578, 00:00:00.0628332

Total Sleep Commands: 10000

bgrainger · 2018-05-04T16:10:22Z

Thanks for the benchmarks; that's looking good.

I feel strongly that this implementation should be aligned with npgsql, so will just wait for some questions on that PR (npgsql/npgsql#1910 (comment)) to be answered.

ElderJames · 2019-12-29T11:53:20Z

How is it going? We need this feature to support tracing.

bgrainger · 2019-12-30T16:44:07Z

I'd like to have some consensus from @roji et al around supporting/implementing this consistently in the ADO.NET ecosystem.

martinjt · 2020-10-07T19:07:57Z

Looks like this has been sat for a while? @bgrainger are you happy with the responses are does there need to be more work? I'd be happy to put some effort in if there's a desire to get this out.

bgrainger · 2020-10-07T22:37:08Z

The main blocking issue for me is that we still don't have a clear understanding of how this should be implemented (consistently) across the ADO.NET ecosystem (or at least M.D.SqlClient, npgsql, and MySqlConnector).

It still seems that .NET tracing/diagnostics is a WIP, e.g., see the discussion about Activity here: https://github.com/dotnet/designs/blob/main/accepted/2020/diagnostics/activity-improvements.md

Is ADO.NET tracing going to use DiagnosticSource/DiagnosticListener, or Activity, or both? What are the event names, and how are the payloads defined? Are they public types or anonymous classes, etc.?

An issue tracking this is dotnet/runtime#26085 but it seems to have stalled out.

So: help is need 8000 ed/wanted but I think the next step is writing, circulating, and getting approval for a proposal, not writing C# code.

roji · 2020-10-08T06:57:31Z

So: help is needed/wanted but I think the next step is writing, circulating, and getting approval for a proposal, not writing C# code.

I really agree with this - we should try to avoid every provider doing its own thing here.

We should definitely try to get a good, consistent ADO.NET diagnostics story in for .NET 6.0 (which providers such as MySqlConnector/Npgsql could implement early as long as consensus is reached).

MikeGoldsmith · 2020-10-22T14:58:32Z

Hey all - I'm a maintainer on the C# .NET OpenTelemetry project, which after working with Microsoft engineers, we have settled on using the newer Activity based diagnostics API.

There are examples of tooling and frameworks being instrumented in both the main and contrib repositories. If you have any questions regarding this, let me know and I can try to answer.

bgrainger · 2020-10-22T15:17:07Z

Those repositories are https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/examples and https://github.com/open-telemetry/opentelemetry-dotnet-contrib, is that correct?

martinjt · 2021-01-19T22:36:49Z

Any thoughts on this @bgrainger? I'm going to need to look at a workaround is all.

bgrainger · 2021-01-20T15:32:24Z

I'm not going to merge this PR as (aside from the merge conflicts) I expect that the final solution will use the new Activity classes. This is still blocked on a unified vision for ADO.NET diagnostics (across Microsoft.Data.SqlClient, Npgsql, and MySqlConnector among others). I'm hopeful that this will come to fruition in the .NET 6.0 timeframe.

@martinjt For now, you will probably need to use a workaround if you need something before then :(

One way to work around it is to write your own DbConnection, DbDataReader, etc. that wraps the "real" one but logs activity. I've written that before in the past so if that's the direction you'd like to take you're welcome to use this MIT-licensed code as a starting point: https://github.com/Faithlife/FaithlifeTracing/blob/master/src/Faithlife.Tracing.Data/TracingDbConnection.cs https://github.com/Faithlife/FaithlifeTracing/blob/master/src/Faithlife.Tracing.Data/TracingDbDataReader.cs etc.

杨晓东 added 5 commits May 3, 2018 15:50

Add diagnostic listener extensions.

7e0d83b

Add diagnostic support for MySqlTransaction.

8000
3d7da04

Add diagnostic support for MySqlCommand.

3b0b175

Add diagnostic support for MySqlConnection.

cb08a38

Add package System.Diagnostics.DiagnosticSource

3b4cdfd

bgrainger reviewed May 3, 2018

View reviewed changes

bgrainger mentioned this pull request May 3, 2018

Instrument with DiagnosticSource npgsql/npgsql#1910

Closed

yang-xiaodong added 2 commits May 4, 2018 12:44

Revise according to the guideline.

0a7db6b

Revise according to the guideline.

2cc8834

liuhaoyang mentioned this pull request Sep 17, 2018

Support Dapper SkyAPM/SkyAPM-dotnet#82

Closed

3 tasks

ElderJames mentioned this pull request Dec 29, 2019

Requirement for supporting mysql with MySqlConnector SkyAPM/SkyAPM-dotnet#213

Closed

4 tasks

roji mentioned this pull request Jan 31, 2020

DiagnosticSource listener/event names for SqlClient and other ADO providers dotnet/runtime#26085

Open

bgrainger force-pushed the master branch from 4ab1880 to 6dbef5b Compare March 26, 2020 20:06

bgrainger mentioned this pull request Oct 7, 2020

Instrument with DiagnosticSource #471

Closed

bgrainger closed this Jan 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add diagnostics support #493

Add diagnostics support #493

Add diagnostics support #493

Add diagnostics support #493

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment