DiagnosticListener no longer works on .NET Core 3.1 or .NET 5

[mattjohnsonpint]

Describe the bug

SqlClient 5.1.0 dropped support for .NET Core 3.1, and targeted .NET 6 instead. However, there are still .NET Standard 2.0 and 2.1 targets, so applications running .NET Core 3.1 or .NET 5.0 can still install the newer version - but in doing so they'll get the .NET Standard version of SqlClient.

Unfortunately, the SqlDiagnosticListener class is unimplemented for .NET Standard. Thus, if a .NET Core 3.1 or .NET 5.0 application updates to the latest version, they will silently lose all diagnostic messages from Sql Client at runtime.

We found this by a test breaking in our CI. See getsentry/sentry-dotnet#2189

To reproduce

Make a .NET Core 3.1 or .NET 5 app that uses Microsoft.Data.SqlClient version 5.0.1 and subscribes to diagnostic listeners. Perform any operation so you can see the diagnostic information.

Then update to Microsoft.Data.SqlClient version 5.1.0. Notice there are no compilation errors, but the diagnostic data is no longer emitted.

Expected behavior

Any of the following would have been acceptable:

• Diagnostic data still is emitted (requiring implementation of SqlDiagnosticListener for .NET Standard)
• The application didn't compile due to the breaking change (which would require some msbuild trickery)
• The version number of the release should have been 6.0.0 to indicate a significant breaking change.

Yes, I understand both .NET Core 3.1 and .NET 5 are out of support, but that doesn't mean it's fine to release changes that will break behavior in them without bumping the major.

[mattjohnsonpint]

I should also point out that I see a lot of other files in this repository that are split between .NetCoreApp.cs vs .NetStandard.cs extensions. So there may be a lot of other things that changed behavior, not just diagnostic listeners.

[mattjohnsonpint]

Here's a minimal reproduction.

using System;
using System.Diagnostics;
using Microsoft.Data.SqlClient;

namespace SqlClientDiagDemo
{
    static class Program
    {
        static void Main()
        {
            var subscriber = new Subscriber();
            DiagnosticListener.AllListeners.Subscribe(subscriber);

            // This is enough to get the first diag message.
            // We don't actually have to connect.
            using var connection = new SqlConnection();
        }
    }

    class Subscriber : IObserver<DiagnosticListener>
    {
        public void OnCompleted() { }

        public void OnError(Exception error) { }

        public void OnNext(DiagnosticListener listener)
        {
            Console.WriteLine($"Subscribing to {listener.Name}");
        }
    }
}

Target netcoreapp3.1 and add a package reference to Microsoft.Data.SqlClient 5.0.1. Run and it will give the output:

Subscribing to SqlClientDiagnosticListener

Now update to version 5.1.0 and run again - the output will be empty.

[JRahnama]

@mattjohnsonpint we will look into this and get back to you soon.

[mattjohnsonpint]

Thanks. I mainly was bringing it to your attention because I believe it will affect our shared customers.

[JRahnama]

@mattjohnsonpint basically, we cannot support TFMs that are not supported by Microsoft, such as netcoreapp3.1 or net5, but regarding Net standard versions we will look into it.

[lcheunglci]

If you want to continue to run on .NET Core 3.1 and .NET5, then you can continue to run with MDS 5.0.x.

[mattjohnsonpint]

If you want to continue to run on .NET Core 3.1 and .NET5, then you can continue to run with MDS 5.0.x.

Sorry, but how is this an answer? According to #152, this library is supposed to follow SemVer. Dropping the older TFMs created a serious breaking change. IMHO, 5.1.0 should be de-listed and re-released as 6.0.0. The major version bump is the signal that people need so they know not to take this update.

At least update the changelog and FAQ to describe the behavior caused by the breaking change.

[mattjohnsonpint]

@DavoudEshtehari @JRahnama - I'm also concerned that the issue was closed by a contributor without input from a member of the @dotnet platform team. Is the response the official Microsoft position?

[DavoudEshtehari]

@mattjohnsonpint This is the result from the bug triage meeting and @lcheunglci is officially in dotnet team. I agree it would be a bit controversial, but it doesn't break the main functionality of the driver and it couldn't be a major bump. It seems a good idea adding it under FAQ.
@David-Engel

[David-Engel]

@mattjohnsonpint We are not de-listing 5.1 and bumping to 6.0. There were no API-level breaking changes in 5.1. Just like OS versions and SQL Server versions regularly drop out of support, we dropped support for out-of-support versions of .NET in 5.1.

And this was specifically noted in the release notes:
https://github.com/dotnet/SqlClient/blob/main/release-notes/5.1/5.1.0.md#breaking-changes

I recommend upgrading to a version of .NET which is in support.

[mattjohnsonpint]

@DavoudEshtehari - Thanks for the clarification, I'm glad this was brought up in triage.

@lcheunglci - I didn't mean any offense, I was just confused about your role here. From my view on GitHub, you just appear as a regular contributor and not as a MS employee or collaborator of this repo, nor as a member of the @dotnet team. Please consider updating your org visibility to public to prevent confusion in the future.

@David-Engel - The release notes mention that .NET Core 3.1 support was dropped, but actually the library continues to work with .NET Core 3.1 and .NET 5.0 due to them being covered by the .NET Standard implementation - which has a slightly reduced feature set. The release notes do not mention which behavioral changes are breaking, but certainly SqlDiagnosticListener support is one of them (and there may be others).

My understanding was that this library followed the same breaking change rules as the rest of the .NET platform, which covers behavioral changes in addition to source and binary API changes. In particular, it states here:

Platform Support

✓ Allowed

• An operation previously not supported on a specific platform, is now supported

✗ Disallowed

• An operation previously supported on a specific platform is no longer supported, or now requires a specific service-pack

As far as recommending upgrading - absolutely. I will recommend to any customer of ours (that is, Sentry customers using the Sentry .NET SDK) that complains when they are no longer seeing telemetry data for SQL queries appear in their dashboard that they either need to update to a supported version of .NET or downgrade to M.D.S. 5.0.1.

I can't speak for the other libraries and applications out there that may have built features reliant on SqlDataListener that will also be broken in the same way. For example, the SqlClient Instrumentation for OpenTelemetry also uses this feature. I believe Application Insights in Azure Monitor does as well. I'm not sure how many others do, which is why I raised the issue here.

Of course, nobody has to take this update - but without a detailed reading of the release notes (and this issue), they'd have no signal that the update was not safe for them. They'd just silently lose the telemetry data.

[mattjohnsonpint]

Digging a little deeper, it appears that the reason there's no diagnostic listener support for .NET Standard is that it was intentionally removed in #535 because "... they are not available for .NET Standard 2.0." That is not exactly correct. They are not part of the standard, but they are indeed available - via the System.Diagnostics.DiagnosticSource NuGet package. M.D.S. already has this dependency, transitively through Microsoft.Data.SqlClient => Azure.Identity => Azure.Core => System.Diagnostics.DiagnosticSource.

Indeed, there are no compilation errors using the SqlDiagnosticListener.NetCoreApp.cs implementation in place for SqlDiagnosticListener.NetStandard.cs.

Furthermore, the existing implementation will also work on .NET Framework, which would address #1529. It only needs a package reference to System.Runtime.Loader added (which the netcore project already has).

Would you be open to a PR to enable SqlDiagnosticListener on .NET Standard and .NET Framework using the existing implementation for .NET Core? That would resolve my concern.

Thanks.

[lcheunglci]

Thanks @mattjohnsonpint for digging deeper into the root cause of the issue. I'll reopen this issue, and feel free to open a PR to enable SqlDiagnosticListener and you'll be able to verify that it builds from our pipeline.