Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Listen to Microsoft.Data.Sqlclient 2.0+ events as well as those from … #2034

Merged
merged 8 commits into from
Nov 26, 2020

Conversation

ulfaxelssoncab
Copy link
Contributor

…System.Data(.SqlClient).

Fix Issue #2032 .

Changes

Se title.

Checklist

  • I ran Unit Tests locally.

No, the dependencies of Mds (net46) would require creating an entirely new test project for this and I am not comfortable enough with this codebase to do that.

Sidenote, why is there SqlClientDiagnosticSourceListenerTests.cs and SqlClientDiagnosticSourceListenerTestsCopy.cs?

  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue if available.

No, that would be a bit premature...

For significant contributions please make sure you have completed the following items:

  • Design discussion issue #
  • Changes in public surface reviewed

The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.

Notes for authors:

  • FxCop and other analyzers will fail the build. To see these errors yourself, compile localy using the Release configuration.

Notes for reviewers:

  • We support comment build triggers
    • /AzurePipelines run will queue all builds
    • /AzurePipelines run <pipeline-name> will queue a specific build

@cijothomas
Copy link
Contributor

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@ulfaxelssoncab
Copy link
Contributor Author

/AzurePipelines run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2034 in repo microsoft/ApplicationInsights-dotnet

@zx48
Copy link

zx48 commented Oct 9, 2020

Hi, what's the blocker on getting this PR in and issue #2032 fixed?

@ulfaxelssoncab
Copy link
Contributor Author

I (as the author of the code in this PR) did not add any tests to actually validate that this fixes the issue. That would involve creating a new test project that is compatible with the requirements of MDS or upgrading the current project to Net Fwk 4.6.

While I could figure out what was wrong and add a fix that compiles, doing the test thing would be going deeper into the rabbit hole than I can do right now.

So I left the PR as a draft (so non-merge:able) here hoping that would at least help someone else fix the issue faster.

@TimothyMothra TimothyMothra added this to the 2.17 milestone Nov 3, 2020
@cijothomas
Copy link
Contributor

Sorry for the delay - Will review this and include this in 2.17 release, as 2.16 is a light release with only major change is just the version bump of DiagnosticSource nuget version.

@cijothomas
Copy link
Contributor

We should be able to merge this as-is. And as a follow up add actual tests for validation.

@ulfaxelssoncab ulfaxelssoncab marked this pull request as ready for review November 23, 2020 16:18
@ulfaxelssoncab
Copy link
Contributor Author

OK, I have changed it from draft to Open/Ready for review.

@TimothyMothra
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@TimothyMothra TimothyMothra merged commit 14de4d1 into microsoft:develop Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants