Add overload to AddOpenTelemetryTracing which has IServiceProvider parameter

[adnang]

To consume other dependencies when setting up tracing for a Host, the README at the documentation for https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Extensions.Hosting/README.md suggests to use the TracerProviderBuilder.Configure method which accepts an Action with IServiceProvider parameter.

This is quite confusing and easy to miss for newcomers, since its not clear from tools like intelliisense that this option is available.
Also, you can recursively call this .Configure() method with a new Action - which has unclear behaviour from just looking at it:

services.AddOpenTelemetryTracing(builder =>
{
    builder.Configure((sp, b) => b.Configure((sp2, b2) => ...))

This design is also not consistent with other libraries extensions to IServiceCollection, which generally have an overload of the top-level method to provide access to the IServiceProvider.

Could we remove .Configure() from the public API in a future version, and instead have an overload:

public static IServiceCollection AddOpenTelemetryTracing(
    this IServiceCollection services, 
    Action<IServiceProvider, TracerProviderBuilder> configure)

which does the same thing as .Configure(Action<IServiceProvider, TracerProviderBuilder>) under the hood

[CodeBlanch]

@adnang

Also, you can recursively call this .Configure() method with a new Action

builder.Configure((sp, b) => b.Configure((sp2, b2) => ...))

Totally supported and works fine! Code link:

opentelemetry-dotnet/src/OpenTelemetry.Extensions.Hosting/Implementation/TracerProviderBuilderHosting.cs

Lines 52 to 57 in de44411

	// Note: Not using a foreach loop because additional actions can be
	// added during each call.
	for (int i = 0; i < this.configurationActions.Count; i++)
	{
	this.configurationActions[i](serviceProvider, this);
	}

Could we remove .Configure() from the public API in a future version

I didn't dig into this fully, but I don't think so. The reason is we need it is for library extensibility. Imagine in your startup you want to do something like this...

builder.AddCustomInstrumentationLibrary();

That extension could be authored like this:

public static TracerProviderBuilder AddCustomInstrumentationLibrary(this TracerProviderBuilder builder)
{
        (tracerProviderBuilder.GetServices()
            ?? throw new NotSupportedException(
                "CustomInstrumentationLibrary requires a hosting TracerProviderBuilder instance."))
            .AddHostedService<CustomInstrumentationHostedService>()
            .AddSingleton<CustomInstrumentationService>();

        return tracerProviderBuilder.Configure((sp, tracerProviderBuilder) =>
        {
            tracerProviderBuilder.AddInstrumentation(sp.GetRequiredService<CustomInstrumentationService>());
        });
}

Basically we are two-phase. Libraries are able to register services they need and also act on the final services when they are available.

Could we remove .Configure() from the public API in a future version, and instead have an overload:

public static IServiceCollection AddOpenTelemetryTracing(
    this IServiceCollection services, 
    Action<IServiceProvider, TracerProviderBuilder> configure)

I would love to find a way to do this, and I am open to suggestions/contributions, but given the above, I don't know if it is possible. Using the proposed extension with that library code...

services.AddOpenTelemetryTracing((sp, builder) =>
   // This is now broken. Because IServiceProvider has already been created at this point and services are closed. 
   // Essentially AddCustomInstrumentationLibrary can't register the services it depends on.
   builder.AddCustomInstrumentationLibrary()
);

[github-actions]

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

[github-actions]

Closed as inactive. Feel free to reopen if this issue is still a concern.