Skip to content

Merge | Port XEvents activity ID correlation to .NET Core #3373

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

Merged
merged 3 commits into from
May 29, 2025

Conversation

edwardneal
Copy link
Contributor

Description

If tracing was enabled, the netfx version of SqlClient would send the current activity ID to the remote database server. This functionality was part of TdsParser and would be covered by the merge, but it lacked test coverage. I've ported the feature to netcore and added a test.

As a result, it's now possible to enable tracing, then link the activity ID of a SqlCommand to the attach_activity_id_xfer value of the appropriate server-side XEvent. The XEvents can then be correlated further within SQL Server. We gain end-to-end trace correlation. In the future, this could be used to link OpenTelemetry spans into SQL Server XEvents.

A fuller description of the feature (albeit one which is described in terms of SQL Server Native Client) is documented here.

Issues

Relates to #1261.

Testing

Added one new functional test. This test enables tracing and XEvents, then runs a simple query or stored procedure and compares the activity ID recorded by the XEvents trace to the activity ID recorded in the SqlClient tracing.

@edwardneal edwardneal requested a review from a team as a code owner May 23, 2025 21:48
@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski
paulmedynski previously approved these changes May 26, 2025
Also add comment to TdsParser referencing MS-TDS specification
@edwardneal edwardneal requested a review from paulmedynski May 26, 2025 11:43
@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

codecov bot commented May 26, 2025

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 62.30%. Comparing base (b8948f2) to head (9bdf134).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 66.66% 2 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3373      +/-   ##
==========================================
- Coverage   67.04%   62.30%   -4.75%     
==========================================
  Files         300      292       -8     
  Lines       65376    65229     -147     
==========================================
- Hits        43831    40639    -3192     
- Misses      21545    24590    +3045     
Flag Coverage Δ
addons ?
netcore 66.69% <66.66%> (-5.51%) ⬇️
netfx 60.92% <75.00%> (-4.27%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cheenamalhotra cheenamalhotra requested a review from a team May 27, 2025 17:26
@cheenamalhotra cheenamalhotra added this to the 6.1-preview2 milestone May 27, 2025
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, great changes, I appreciate your efforts on these, @edwardneal !!

@benrr101 benrr101 merged commit b68f44a into dotnet:main May 29, 2025
237 checks passed
@edwardneal edwardneal deleted the merge/xevents-tracing branch May 29, 2025 19:25
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