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

[Repo] TracerProviderBuilder dependency injection docs updates #3869

Merged
merged 14 commits into from
Nov 10, 2022

Conversation

CodeBlanch
Copy link
Member

Changes

Added content for the "Guidance for library authors" section.

@CodeBlanch CodeBlanch requested a review from a team November 7, 2022 21:53
@@ -378,17 +378,15 @@ For the below examples imagine an exporter with this constructor:
```csharp
public class MyCustomExporter : BaseExporter<Activity>
{
public MyCustomExporter(
ILogger<MyCustomExporter> logger,
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I removed ILogger from this example. I was thinking that might lead people into trouble with endless log/export loops. @cijothomas @utpilla Should we add something in extending-the-sdk about how to log safely in an exporter? Maybe we mention that somewhere already I just checked briefly and didn't see anything.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/trace/extending-the-sdk has note about SuppressInstrumentationScope... That would help, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

To fix the loop, yes, totally helps! But if user was actually trying to write logs into their OTel stream, it will just eat them 😄 What I was thinking was more of an example that showed how to use EventSource to do it?

extension to register callbacks to configure options class instances instead of
invoking callbacks directly.

### IServiceCollection detached registration
Copy link
Member

@alanwest alanwest Nov 8, 2022

Choose a reason for hiding this comment

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

Is "detached registration" a term that is commonly used? Took me a bit to orient myself to what this section was about. Would you say that this scenario would be common for applications not using one of the host builders, i.e., the DI container is manually instantiated?

Copy link
Member Author

Choose a reason for hiding this comment

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

No "detached" is just what I have been calling it for lack of a better term 🤣 This is a feature users like @martinjt have asked for, being able to configure things with only a reference to IServiceCollection.

Would you say that this scenario would be common for applications not using one of the host builders, i.e., the DI container is manually instantiated?

I would expect the use case to be like...

var appBuilder = WebApplication.CreateBuilder(args);

appBuilder.Services.AddMyTelemetry(); // Configures resource + exporters based on IConfiguration.
appBuilder.Services.AddMyLibrary1(); // Configures dependencies, adds tracing sources & metric meters for library1.
appBuilder.Services.AddMyLibrary2(); // Configures dependencies, adds tracing sources & metric meters for library2.

appBuilder.Services.AddOpenTelemetryTracing();
appBuilder.Services.AddOpenTelemetryMetrics();

In the bootstrap the hosting extensions are used to start the providers but configuration is decoupled and owned by other components.

Copy link
Member

Choose a reason for hiding this comment

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

I see, maybe little bit of reorganization could help bring some more clarity.

## Guidance for library authors for providing extension methods to configure their custom components

Library authors are encouraged to provide extension methods on the
`TracerProviderBuilder` class for registering custom OpenTelemetry components.
Additionally, library authors may also consider providing extension methods
on `IServiceCollection`. Both of these patterns are described below.

### TracerProviderBuilder extension methods

Targeting `TracerProviderBuilder` will allow users to discover extensions using
IDE helpers such as `IntelliSense` (in Visual Studio or Visual Studio Code)
while configuring the builder(s) in their applications.

...

### IServiceCollection extension methods

In some scenarios, it may also be desirable to offer extension methods on
`IServiceCollection`. This can be the case when an author wishes to simplify
the configuration of numerous custom components. For example, consider the following:

... example of calling three methods here ...

...

The `ConfigureOpenTelemetryTracing` extension method allows this to be simplified.

... IServiceCollection example ...

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 just pushed big changes. Tried to incorporate 1) your feedback from above and 2) feedback from the SIG yesterday. Primarily I included in the IServiceCollection example @martinjt's idea to have the extension use options for turning on/off telemetry. Also adjusted the recommendations so each has a more defined use case.

LMK your thoughts!

Comment on lines 585 to 587
The benefit to using the `IServiceCollection` style is users only need to call a
single `AddMyLibrary` extension to configure the library itself and optionally
turn on OpenTelemetry integration.
Copy link
Member

Choose a reason for hiding this comment

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

This could be said about the TracerProviderBuilder style as well. I think the thing to highlight here about the IServiceCollection style is just what your example demonstrates - as an instrumentation author, I'm likely only taking an API dependency not an SDK dependency, so therefore the guidance you provide me is to offer an extension method on IServiceCollection making it easy to configure the instrumentation.

Copy link
Member

Choose a reason for hiding this comment

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

🤷 here's a stab at this

One of the main benefits of the IServiceCollection style is that it provides a way for library authors, taking only a dependency on the OpenTelemetry API, to offer an extension for configuring telemetry emitted from the library. This is commonly the case for instrumentation library authors.

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 tweaked this a bit to clarify my point and merged so I could keep moving on the configuration section. But feel free to modify further, I'm not at all sure it is perfect yet 😄

taking only a dependency on the OpenTelemetry API

At the moment, to use this stuff you MUST have an SDK reference. If you only have an API reference, you can do ConfigureBuilder types of things by using IDeferredTracerProviderBuilder. But there is nothing in API to give you access to the IServiceCollection because API doesn't have a Microsoft.Extensions.DependencyInjection.Abstractions reference (it can't see ISeviceCollection). That's what this PR was doing: #3663. If we add that DependencyInjection.Abstractions reference to API, we can move some of this fundamental stuff into API and enable this scenario with that lower reference.

Copy link
Member

Choose a reason for hiding this comment

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

At the moment, to use this stuff you MUST have an SDK reference.

Ha, yup. We've been talking about all this stuff for too long 😆. My brain wires are crossed between reality and things that have simply been proposed.

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

I think this doc is pretty solid. Still struggling a bit myself to frame the benefits of authoring extensions on IServiceCollection, but I think we can smooth things out more as we get user feedback.

@CodeBlanch CodeBlanch merged commit 165b6f4 into open-telemetry:main Nov 10, 2022
@CodeBlanch CodeBlanch deleted the docs-di-libraryguidance branch November 10, 2022 17:44
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.

5 participants