Add diagnostics support

[yang-xiaodong]

Add diagnostics support for MySqlConnection, MySqlCommand, MySqlTransaction

• Related issues: Instrument with DiagnosticSource #471

[bgrainger]

See similar npgsql PR: npgsql/npgsql#1910

[bgrainger]

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.

[bgrainger]

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

[yang-xiaodong]

Fixed

[bgrainger]

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.

[yang-xiaodong]

Fixed

[bgrainger]

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.

[yang-xiaodong]

Fixed

[bgrainger]

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

[yang-xiaodong]

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:00: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]

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]

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

[bgrainger]

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

[martinjt]

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]

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 needed/wanted but I think the next step is writing, circulating, and getting approval for a proposal, not writing C# code.

[roji]

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]

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]

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]

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

[bgrainger]

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.