Skip to content

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

Merged
TimothyMothra merged 8 commits intomicrosoft:developfrom
ulfaxelssoncab:develop
Nov 26, 2020
Merged

Listen to Microsoft.Data.Sqlclient 2.0+ events as well as those from …#2034
TimothyMothra merged 8 commits intomicrosoft:developfrom
ulfaxelssoncab:develop

Conversation

@ulfaxelssoncab
Copy link
Copy Markdown
Contributor

@ulfaxelssoncab ulfaxelssoncab commented Sep 8, 2020

…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
Copy Markdown
Contributor

cijothomas commented Sep 15, 2020

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Sep 15, 2020

Azure Pipelines successfully started running 3 pipeline(s).

@ulfaxelssoncab
Copy link
Copy Markdown
Contributor Author

ulfaxelssoncab commented Sep 26, 2020

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Sep 26, 2020

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

@zx48 zx48 mentioned this pull request Oct 8, 2020
Closed
@zx48
Copy link
Copy Markdown

zx48 commented Oct 9, 2020

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

@ulfaxelssoncab
Copy link
Copy Markdown
Contributor Author

ulfaxelssoncab commented Oct 9, 2020

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
Copy Markdown
Contributor

cijothomas commented Nov 9, 2020

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
Copy Markdown
Contributor

cijothomas commented Nov 23, 2020

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
Copy Markdown
Contributor Author

ulfaxelssoncab commented Nov 23, 2020

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

@TimothyMothra
Copy link
Copy Markdown

TimothyMothra commented Nov 25, 2020

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Nov 25, 2020

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

Reviewers

1 more reviewer

@TimothyMothra TimothyMothra TimothyMothra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@TimothyMothra TimothyMothra

Labels

None yet

Projects

None yet

Milestone

2.17

Development

Successfully merging this pull request may close these issues.

4 participants

@ulfaxelssoncab @cijothomas @zx48 @TimothyMothra