-
Notifications
You must be signed in to change notification settings - Fork 355
[SqlClient] Stop Activity.Current if present #3041
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3041 +/- ##
==========================================
+ Coverage 69.78% 69.89% +0.11%
==========================================
Files 420 410 -10
Lines 16294 16242 -52
==========================================
- Hits 11371 11353 -18
+ Misses 4923 4889 -34
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where SQL activities were not being properly stopped on .NET Framework when using a global activity listener with metrics-only configuration. The change ensures that any current SQL activity is stopped regardless of whether tracing or metrics are being collected.
- Separates activity stopping logic from activity modification logic in the event handler
- Adds comprehensive test coverage for the metrics-only scenario with global activity listener
- Updates changelog to document the bug fix
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| SqlEventSourceListener.netfx.cs | Refactors OnEndExecute method to ensure SQL activities are stopped even when only metrics are enabled |
| SqlClientIntegrationTests.cs | Adds test to verify activities are properly stopped in metrics-only scenarios and minor code cleanup |
| CHANGELOG.md | Documents the bug fix for activity stopping issue |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientIntegrationTests.cs
Show resolved
Hide resolved
Ensure any current activity is stopped if only metrics is enabled but a global activity listener is registered. Resolves open-telemetry#3033.
Add PR number and fix lint warning.
Only stop if it's out activity.
Refactor method to only have one point of return.
- Add integration test for open-telemetry#3033. - Add missing assertion. - Use expression bodied method.
Fix entries being added to previous release.
a6012e3 to
6b47a9c
Compare
Fix bad merge conflict resolution with main.
Fixes #3033
Changes
Ensure any current activity is stopped if only metrics is enabled but a global activity listener is registered.
I've manually verified the original issue is resolved and that .NET isn't affected by the same issue.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)