Added diagnostics to sqlclient

[trstringer]

This pull request includes changes to SqlClient in order to provide a mechanism for facilitating diagnostics collection in the library for all Execute*() operations (sync and async).

Tests have been created in order to test the functionality as well.

This functionality is proposed for merging as a facilitator of client-side diagnostics and logging to collect relevant data retrieval information for monitoring, baselining, and troubleshooting.

[saurabh500]

@tstringer all tests are failing. Can you take a look?

[trstringer]

@saurabh500 without further investigation, I'm assuming that the SqlClient tests are failing largely due to the fact that they require a connection. I took the connection string from the ExceptionTests (https://github.com/tstringer/corefx/blob/sqlclient-diagnostics/src/System.Data.SqlClient/tests/FunctionalTests/ExceptionTest.cs#L14) and used it in my tests: https://github.com/tstringer/corefx/blob/sqlclient-diagnostics/src/System.Data.SqlClient/tests/FunctionalTests/DiagnosticTest.cs#L13 for reference. Do you know if that is a connect-able source? Any recommendation on these connection strings to actually connect to a valid data source?

[trstringer]

@saurabh500 yep, looks like that's the cause: System.Data.SqlClient.SqlException : A network-related or instance-specific error occurred while establishing a connection to SQL Server. The server was not found or was not accessible. Verify that the instance name is correct and that SQL Server is configured to allow remote connections.

Let me know what connection string I can use in my tests for the CI builds! Tests' connection string can be found here, it appears this is invalid

[nbilling]

The exception tests don't expect the connection string to work since they all get into one error state or another before the connection string actually gets used.

We already discussed the unit test thing and it would require a SqlConnection mock that doesn't currently exist in corefx. Our decision was to go without any unit tests and to either change those tests to manual ones or omit them. Possibly I forgot to pass this on to you @tstringer ? My bad if so.

[avanderhoorn]

@saurabh500 if the decision is to omit these... do we just want them comments out/ignored for now so that they are there when you do get a mock connection eventually?

[trstringer]

@nbilling @avanderhoorn thanks for the input, that sounds like the way to go here. @saurabh500 how would you like me to proceed here? Would you like for me to make the test attributes [Fact(Skip = "Skipping due to data source dependency")]? Let me know and I'll make those changes asap! 😄

[saurabh500]

The connection string for exception test is definitely not expected to work :)

For SqlClient we don't have any tests connecting to the server in Github. We do run tests internally for SqlClient using on premise and Azure Sql servers to make sure things are not broken. It is not a good solution and needs work.

@tstringer You have a few options.

1. You can disable the tests. Check in a Readme on how to execute these tests locally so that we can verify the functionality. We do something similar for our ManualTests folder.
2. You can use localdb to run the tests on Windows to get some coverage. EF team uses LocalDB for some of their tests. That would work as well.

Option 2 would be preferred option. You could start with Option 1, open an issue to migrate to Option 2 and send another PR with Option 2 incorporated into the tests.

Long term, we will consider open sourcing TDS server and setting up Azure Sql / Sql in a VM infra for testing.

[saurabh500]

Is this dependency necessary? We are trying to move away from System.Collection.NonGeneric

[trstringer]

@saurabh500 That dependency is unnecessary, and I have removed it (and tested post-removal) in commit 23de00d

[trstringer]

@saurabh500 I have changed the diagnostics tests to be run manually, and this change is reflected in commit 5de34d8

[nbilling]

Should the test be moved from functional to manual directory?

[trstringer]

@nbilling I had a quick phone conversation with @saurabh500 and the intent here is to get through this PR like this, and then I'll create an issue and subsequent PR that makes these tests hit localdb so they are functional at least on Windows. So this is a temporary fix for these tests at the moment that will be resolved shortly.

[nbilling]

Fair enough.

[justinvp]

Is there a reason these need to be static readonly fields, or could they be constants? e.g.:

public const string DiagnosticListenerName = "SqlClientDiagnosticListener";

private const string SqlClientPrefix = "System.Data.SqlClient.";
public const string SqlBeforeExecuteCommand = SqlClientPrefix + nameof(WriteCommandBefore);
public const string SqlAfterExecuteCommand = SqlClientPrefix + nameof(WriteCommandAfter);
public const string SqlErrorExecuteCommand = SqlClientPrefix + nameof(WriteCommandError);

[trstringer]

@justinvp I have made these changes and they are reflected in commit 319736b Thanks! 😄

[saurabh500]

@tstringer I went through the Async changes and it looks good to me. 👍
The only open item right now is the [CallerMemberName] comment. You are good to merge after that.
Do you have permissions or do you need us to merge?

[trstringer]

@saurabh500 Great! I have made the changes to incorporate [CallerMemberName] and now all of the requested changes should be reflected in the commit. Of note is that there were merge conflicts this morning, so I did a pull from upstream master and rebased this branch. I also squashed the commits into a single commit.

To answer your question directly, unfortunately no I don't have write access to this repository, so I would need you to merge this pull request.

Thanks! 😄

[avanderhoorn]

Thanks so much to everyone involved for making this happen!!! This was a gigantic effort and will make a big difference. Thanks again to @tstringer making the code happen, @saurabh500 & @YoungGah for being open to getting this code in and taking the time to review, @nbilling for your second pair of eyes, and @justinvp @stephentoub for your input/reviews!

[trstringer]

Huge thanks to the entire team, especially for the code reviews!! It was a pleasure working with everybody, and I look forward to further contributions and interactions with everyone involved here!