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

[sqlclient] Make SqlClientInstrumentation a singleton #2305

Merged

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Nov 7, 2024

In an upcoming PR we will be adding metric instrumentation. That instrumentation will need to leverage the same DiagnosticSourceListener that is used for traces. This PR makes SqlClientInstrumentation a singleton because multiple instances of the instrumentation would create duplicate activities.

@alanwest alanwest requested a review from a team as a code owner November 7, 2024 00:35
@github-actions github-actions bot added the comp:instrumentation.sqlclient Things related to OpenTelemetry.Instrumentation.SqlClient label Nov 7, 2024
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

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

Project coverage is 86.36%. Comparing base (71655ce) to head (3e11aac).
Report is 587 commits behind head on main.

Files with missing lines Patch % Lines
...ent/Implementation/SqlEventSourceListener.netfx.cs 77.77% 2 Missing ⚠️
...ient/Implementation/SqlClientDiagnosticListener.cs 91.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2305       +/-   ##
===========================================
+ Coverage   73.91%   86.36%   +12.44%     
===========================================
  Files         267        8      -259     
  Lines        9615      330     -9285     
===========================================
- Hits         7107      285     -6822     
+ Misses       2508       45     -2463     
Flag Coverage Δ
unittests-Instrumentation.SqlClient 86.36% <91.66%> (?)

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

Files with missing lines Coverage Δ
...trumentation.SqlClient/SqlClientInstrumentation.cs 90.90% <100.00%> (ø)
...ation.SqlClient/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
...ient/Implementation/SqlClientDiagnosticListener.cs 83.72% <91.66%> (ø)
...ent/Implementation/SqlEventSourceListener.netfx.cs 70.42% <77.77%> (ø)

... and 268 files with indirect coverage changes

@Kielek
Copy link
Contributor

Kielek commented Nov 7, 2024

@alanwest, I do not think that metrics should relay on activities. Please consider sampling scenario. It might lead to dropping measurements.

As I remember we have tried this approach in AspNetCore, then changed to separate listeners.

@alanwest
Copy link
Member Author

alanwest commented Nov 7, 2024

I do not think that metrics should relay on activities. Please consider sampling scenario. It might lead to dropping measurements.

As I remember we have tried this approach in AspNetCore, then changed to separate listeners.

Metrics will not rely on activities. A single listener will be needed though because when both tracing and metrics are enabled, the duration of the span and metrics must be the same. A stopwatch will be used when only metrics are enabled or the span is sampled.

Aspnet core was a bit different as the span is always created by the native instrumentation.

@CodeBlanch CodeBlanch changed the title Make SqlClientInstrumentation a singleton [sqlclient] Make SqlClientInstrumentation a singleton Nov 7, 2024
Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@Kielek Kielek merged commit 4d7161b into open-telemetry:main Nov 7, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.sqlclient Things related to OpenTelemetry.Instrumentation.SqlClient
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants