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

[Traces] Support DI scenarios in instrumentation without SDK dependency + named options in SqlClient instrumentation #3663

Closed
wants to merge 2 commits into from

Conversation

CodeBlanch
Copy link
Member

Changes

#3533 doesn't tackle IDeferredTracerProviderBuilder which is needed for instrumentation libraries which do not have a dependency on SDK. Such libraries are limited in what they can participate in because without IServiceCollection they can't register anything.

What this PR does is expose IServiceCollection to API and shows how OpenTelemetry.Instrumentation.SqlClient can then use the same patterns as everything else.

To do this API needs a Microsoft.Extensions.DependencyInjection.Abstractions reference. Is that a big deal? 🤷‍♀️ That reference comes with SDK regardless, so most users are going to get it one way or another.

Opening as a draft to gather feedback.

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Merging #3663 (7a4300f) into main (ef46e82) will decrease coverage by 0.16%.
The diff coverage is 77.27%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3663      +/-   ##
==========================================
- Coverage   87.61%   87.45%   -0.17%     
==========================================
  Files         283      283              
  Lines       10270    10282      +12     
==========================================
- Hits         8998     8992       -6     
- Misses       1272     1290      +18     
Impacted Files Coverage Δ
...c/OpenTelemetry.Api/Trace/TracerProviderBuilder.cs 33.33% <0.00%> (-66.67%) ⬇️
...y/Trace/Builder/TracerProviderBuilderExtensions.cs 97.29% <ø> (-0.38%) ⬇️
...lemetry/Trace/Builder/TracerProviderBuilderBase.cs 90.65% <85.71%> (-0.78%) ⬇️
...ation.SqlClient/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 35.71% <0.00%> (-42.86%) ⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 36.36% <0.00%> (-40.91%) ⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs 75.00% <0.00%> (-10.00%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 91.75% <0.00%> (-4.40%) ⬇️
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 86.66% <0.00%> (-2.23%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 96.87% <0.00%> (-0.79%) ⬇️
... and 10 more

Comment on lines +72 to +73
.AddSqlClientInstrumentation()
.AddSqlClientInstrumentation("Instrumentation2", configureSqlClientInstrumentationOptions: null)
Copy link
Member

Choose a reason for hiding this comment

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

I think the goofy thing about adding instrumentation twice is that there are a couple strange side effects that could occur.

With the SqlClient instrumentation I think what would happen is that multiple activities would be started

// SqlClient does not create an Activity. So the activity coming in here will be null or the root span.
activity = SqlActivitySourceHelper.ActivitySource.StartActivity(
SqlActivitySourceHelper.ActivityName,
ActivityKind.Client,
default(ActivityContext),
SqlActivitySourceHelper.CreationTags);

If this pattern were applied to the ASP.NET Core instrumentation I think the side effect might be different. I think there'd only be one activity (the one created by ASP.NET Core), but the instrumentation would be invoked twice... maybe duplicating work that shouldn't be duplicated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good points! Just to clarify, it was always possible before to register instrumentation(s) multiple times. All I'm doing here is supporting binding named options to it. We should look at this as a separate effort?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, definitely a separate effort to consider how and if we should prevent multiple calls to add instrumentation.

Though, if we did prevent multiple calls then would there value in supporting named options for instrumentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a well thought out answer 😄 Really all I'm doing on these PRs is establishing anytime you ask for options, pass a name (either user-supplied or default). IMO it is a good pattern that allows the users to have control if they happen to need it without a lot of effort to do it.

Why would they need it? Off the top of my head...

  • Using named options always could just be a preference thing for large complex applications?

  • Imagine some application with 2 tracer providers. That application has a single IConfiguration which needs to apply to different providers. Naming helps in this case in a big way. More clear with exporters for sure. Instrumentation... depends on what the behavior is for calling "AddInstrumentation" across two providers. What the user probably wants to do in that case is create one activity, listen on both providers? But then we also have filter/enrich things to worry about. Kind of undefined at the moment.

  • Reaching here, but thinking about the OTel config file thing. Let's say we read in OTel standard config file and expose via Options/IConfiguration keys so that SDK can pick it up it. Having the name available everywhere might be useful as a disambiguation/collision avoidance mechanism?

@@ -11,5 +11,6 @@

<ItemGroup>
<PackageReference Include="System.Diagnostics.DiagnosticSource" Version="$(SystemDiagnosticSourcePkgVer)" />
<PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="$(MicrosoftExtensionsDependencyInjectionAbstractionsPkgVer)" />
Copy link
Member

Choose a reason for hiding this comment

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

Since some of our other instrumentation packages reference the SDK, should we just consider having the SqlClient instrumentation reference it too?

Copy link
Member

Choose a reason for hiding this comment

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

it is a bug that Instrumentations depend on SDK. We'll need to fix that sometime in the future. The main reason was to do the SupressInstrumentation stuff, which was only in SDK, not in API...

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you guys feel about what this PR is doing?

  • Adding Microsoft.Extensions.DependencyInjection.Abstractions to API allows it to see IServiceCollection
  • Adding ConfigureServices & ConfigureBuilder to TracerProviderBuilder allows all the fundamental DI features to work with only API reference + allows us to sunset the IDeferredTracerProviderBuilder 🌤️

Copy link
Member

Choose a reason for hiding this comment

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

it is a bug that Instrumentations depend on SDK

😆 that was my thought too, but wanted to test the waters.

In some sense, I've thought about the dependencies of the SDK project on DI, Configuration, Options, etc to be a kind of known bug/issue. As I recall, it was unintentional, but the ship sailed.

How do you guys feel about what this PR is doing?

I feel ok about this especially given the unintended stance we fell into on the SDK project. Though, I think it warrants a discussion beyond the scope of just this PR. I think we should attempt to define boundaries of what kind of things are OK for the API and SDK to depend on going forward. It would help this discussion and the one here #3639 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

SDK depending on additional things is one thing. Its less concerning.
API can be used by library authors to instrument their libraries. (in case of .NET, a lot of libraries just need DS dependency, but some would need OTel API as well). Adding more dependency to API require a stronger justification...

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 23, 2022
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants