[DependencyInjection] Introduce new package and refactor SDK

[CodeBlanch]

Relates to #3663

Overview

We have been discussing the public API additions for 1.4.0. One concern is that instrumentation libraries with only API references won't be able to participate fully in the library patterns. One option was #3663. This PR is another option which is to add another package which instrumentation could depend on without consuming the full SDK. Currently named OpenTelemetry.Extensions.DependencyInjection (itself depends on API + Microsoft.Extensions.DependencyInjection.Abstractions).

Changes

Refactors the following APIs from SDK into OpenTelemetry.Extensions.DependencyInjection:

• MeterProviderBuilder + TracerProviderBuilder AddInstrumentation extensions
• MeterProviderBuilder + TracerProviderBuilder ConfigureServices & ConfigureBuilder extensions
• IServiceCollection ConfigureOpenTelemetryTracing & ConfigureOpenTelemetryMetrics extensions

Example usage

Here is a diff showing how this helps solve the problem.

New startup pattern

There is a new pattern being deployed here. We used to do this:

var appBuilder = WebApplication.CreateBuilder(args);

appBuilder.Services.AddOpenTelemetryTracing(builder => builder.AddConsoleExporter());
appBuilder.Services.AddOpenTelemetryMetrics(builder => builder.AddConsoleExporter());

The new style is this:

var appBuilder = WebApplication.CreateBuilder(args);

appBuilder.Services.AddOpenTelemetry()
    .WithTracing(builder => builder.AddConsoleExporter())
    .WithMetrics(builder => builder.AddConsoleExporter())
    .StartWithHost();

The reason for that is something needs to invoke the SDK so it can patch everything up to pick up the IConfigureTracerProviderBuilder & IConfigureMeterProviderBuilder registrations in the IServiceCollection (that's what OpenTelemetry.DependencyInjection does). @alanwest and I kicked around a bunch of ideas and this is the best we could come up with.

One thing nice it does as a side effect is give us a spot to do cross-cutting things. Like this:

var appBuilder = WebApplication.CreateBuilder(args);

appBuilder.Services.AddOpenTelemetry()
    .ConfigureResource(r => r.AddService(serviceName: "MyOtelService"))
    .WithTracing(builder => builder.AddConsoleExporter())
    .WithMetrics(builder => builder.AddConsoleExporter())
    .StartWithHost();

PS: We can do logging there too, I'm just waiting on the spec to go stable before messing with it. I will update main-logs if/when this goes in.

Public API Changes

OpenTelemetry.Extensions.DependencyInjection

namespace OpenTelemetry.Trace
{
   // This is the API a builder must implement for everything to work. SDK does this now.
   public interface ITracerProviderBuilder : IDeferredTracerProviderBuilder
   {
       TracerProvider? Provider { get; }
       TracerProviderBuilder ConfigureServices(Action<IServiceCollection> configure);
       TracerProviderBuilder ConfigureBuilder(Action<IServiceProvider, TracerProviderBuilder> configure);
   }

   // These extensions used to live in SDK. They are now in DependencyInjection so that libraries may use them without SDK reference.
   public static class OpenTelemetryDependencyInjectionTracerProviderBuilderExtensions
   {
       public static TracerProviderBuilder AddInstrumentation<T>(this TracerProviderBuilder tracerProviderBuilder) where T : class;
       public static TracerProviderBuilder AddInstrumentation<T>(this TracerProviderBuilder tracerProviderBuilder, T instrumentation) where T : class;
       public static TracerProviderBuilder AddInstrumentation<T>(this TracerProviderBuilder tracerProviderBuilder, Func<IServiceProvider, T> instrumentationFactory) where T : class;
       public static TracerProviderBuilder AddInstrumentation<T>(this TracerProviderBuilder tracerProviderBuilder, Func<IServiceProvider, TracerProvider, T> instrumentationFactory) where T : class;
       public static TracerProviderBuilder ConfigureServices(this TracerProviderBuilder tracerProviderBuilder, Action<IServiceCollection> configure);
       public static TracerProviderBuilder ConfigureBuilder(this TracerProviderBuilder tracerProviderBuilder, Action<IServiceProvider, TracerProviderBuilder> configure);
   }

   // This extension enables the "detached" configuration.
   // Libraries may use this (or IConfigureTracerProviderBuilder directly) to register actions into the IServiceCollection which SDK will consume.
   // Options API Configure extension and IConfigureOptions<T> work the same way.
   public static class OpenTelemetryDependencyInjectionTracingServiceCollectionExtensions
   {
       public static IServiceCollection ConfigureOpenTelemetryTracerProvider(this IServiceCollection services, Action<IServiceProvider, TracerProviderBuilder> configure);
   }

   public interface IConfigureTracerProviderBuilder
   {
       void ConfigureBuilder(IServiceProvider serviceProvider, TracerProviderBuilder tracerProviderBuilder);
   }
}

namespace OpenTelemetry.Metrics
{
   public interface IMeterProviderBuilder : IDeferredMeterProviderBuilder
  {
       MeterProvider? Provider { get; }
       MeterProviderBuilder ConfigureServices(Action<IServiceCollection> configure);
       MeterProviderBuilder ConfigureBuilder(Action<IServiceProvider, MeterProviderBuilder> configure);
   }

   public static class OpenTelemetryDependencyInjectionMeterProviderBuilderExtensions
   {
       public static MeterProviderBuilder AddInstrumentation<T>(this MeterProviderBuilder meterProviderBuilder) where T : class;
       public static MeterProviderBuilder AddInstrumentation<T>(this MeterProviderBuilder meterProviderBuilder, T instrumentation) where T : class;
       public static MeterProviderBuilder AddInstrumentation<T>(this MeterProviderBuilder meterProviderBuilder, Func<IServiceProvider, T> instrumentationFactory) where T : class;
       public static MeterProviderBuilder AddInstrumentation<T>(this MeterProviderBuilder meterProviderBuilder, Func<IServiceProvider, MeterProvider, T> instrumentationFactory) where T : class;
       public static MeterProviderBuilder ConfigureServices(this MeterProviderBuilder meterProviderBuilder, Action<IServiceCollection> configure);
       public static MeterProviderBuilder ConfigureBuilder(this MeterProviderBuilder meterProviderBuilder, Action<IServiceProvider, MeterProviderBuilder> configure);
   }

   public static class OpenTelemetryDependencyInjectionMetricsServiceCollectionExtensions
   {
       public static IServiceCollection ConfigureOpenTelemetryMeterProvider(this IServiceCollection services, Action<IServiceProvider, MeterProviderBuilder> configure);
   }

   public interface IConfigureMeterProviderBuilder
   {
       void ConfigureBuilder(IServiceProvider serviceProvider, MeterProviderBuilder meterProviderBuilder);
   }
}

OpenTelemetry

namespace OpenTelemetry
{
   public static class OpenTelemetryServiceCollectionExtensions
   {
      // New entry point for adding OpenTelemetry SDK into an IServiceCollection
      public static OpenTelemetryBuilder AddOpenTelemetry(this IServiceCollection services);
   }

   public class OpenTelemetryBuilder
   {
       public IServiceCollection Services { get; }
       public OpenTelemetryBuilder ConfigureResource(Action<ResourceBuilder> configure);
       public OpenTelemetryBuilder WithMetrics();
       public OpenTelemetryBuilder WithMetrics(Action<MeterProviderBuilder> configure);
       public OpenTelemetryBuilder WithTracing();
       public OpenTelemetryBuilder WithTracing(Action<TracerProviderBuilder> configure);
   }
}

OpenTelemetry.Extensions.Hosting

namespace OpenTelemetry
{
   public static class OpenTelemetryBuilderHostingExtensions
   {
       // New extension for registering the IHostedService. Replaces AddOpenTelemetryTracing & AddOpenTelemetryMetrics
       public static OpenTelemetryBuilder StartWithHost(this OpenTelemetryBuilder builder);
   }
}

namespace Microsoft.Extensions.DependencyInjection
{
   public static class OpenTelemetryServicesExtensions
   {
+      [Obsolete("Use the AddOpenTelemetry().WithTracing(configure).StartWithHost() pattern instead. This method will be removed in a future version.")]
       public static IServiceCollection AddOpenTelemetryTracing(this IServiceCollection services);
+      [Obsolete("Use the AddOpenTelemetry().WithTracing(configure).StartWithHost() pattern instead. This method will be removed in a future version.")]
       public static IServiceCollection AddOpenTelemetryTracing(this IServiceCollection services, Action<TracerProviderBuilder> configure);
+      [Obsolete("Use the AddOpenTelemetry().WithMetrics(configure).StartWithHost() pattern instead. This method will be removed in a future version.")]
       public static IServiceCollection AddOpenTelemetryMetrics(this IServiceCollection services);
+      [Obsolete("Use the AddOpenTelemetry().WithMetrics(configure).StartWithHost() pattern instead. This method will be removed in a future version.")]
       public static IServiceCollection AddOpenTelemetryMetrics(this IServiceCollection services, Action<MeterProviderBuilder> configure);
   }
}

TODOs

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

[github-actions]

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.

[alanwest]

I think this PR is good and I think it is the approach we should move forward with.

Though, just putting some of my thought process into words...

I had waffled a lot on whether it just makes sense to adopt #3663 and have OpenTelemetry.Api just take a dependency on Microsoft.Extensions.DependencyInjection.Abstractions.

I think the main question is: who is the target audience for OpenTelemetry.Api and does OpenTelemetry.DependencyInjection have the exact same audience?

As it stands OpenTelemetry.Api serves two (maybe) distinct audiences.

Instrumentation authors

OpenTelemetry.Api enables instrumentation authors to:

• Use the OpenTelemetry shim API rather than DiagnosticSource directly.
• Use features of the OpenTelemetry API not yet provided by DiagnosticSource.

Instrumentation library authors

OpenTelemetry.Api enables instrumentation library authors to:

• Everything instrumentation authors do, plus
• Configure telemetry providers e.g., TracerProvider, MeterProvider.

So, the main distinction between an instrumentation author and an instrumentation library author is that the latter is concerned with configuration of emitted telemetry. Examples of configuration include enabling telemetry, filtering telemetry, hooks to further enrich telemetry, sanitizing telemetry, etc.

If we had it to do over again, would MeterProviderBuilder and TracerProviderBuilder not be in OpenTelemetry.Api? If this were the case then I think a separate package definitely makes sense.

But since MeterProviderBuilder and TracerProviderBuild are already in the API then it raises the question whether to continue shoving additional configuration related concerns into the API. I think the answer is still no and a separate package still makes sense. That said, I think the "instrumentation author" audience is theoretical, and in practice there are really only going to be "instrumentation library authors".

[codecov]

Codecov Report

Merging #3923 (1b8c894) into main (f0f5158) will decrease coverage by 0.14%.
The diff coverage is 89.66%.

❗ Current head 1b8c894 differs from pull request most recent head d95757e. Consider uploading reports for the commit d95757e to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3923      +/-   ##
==========================================
- Coverage   85.50%   85.36%   -0.15%     
==========================================
  Files         288      289       +1     
  Lines       11080    11219     +139     
==========================================
+ Hits         9474     9577     +103     
- Misses       1606     1642      +36     

Impacted Files	Coverage Δ
...ensions.Hosting/OpenTelemetryServicesExtensions.cs	0.00% <0.00%> (-100.00%)	⬇️
...rc/OpenTelemetry/Trace/TracerProviderExtensions.cs	68.00% <ø> (ø)
src/OpenTelemetry/OpenTelemetryBuilder.cs	63.63% <63.63%> (ø)
...lemetry/Trace/Builder/TracerProviderBuilderBase.cs	78.37% <80.88%> (-12.32%)	⬇️
.../Metrics/Builder/MeterProviderBuilderExtensions.cs	80.80% <82.50%> (-1.25%)	⬇️
...lemetry/Metrics/Builder/MeterProviderBuilderSdk.cs	89.65% <89.79%> (-3.21%)	⬇️
...emetry/Metrics/Builder/MeterProviderBuilderBase.cs	94.54% <94.33%> (-1.58%)	⬇️
...elemetry/Trace/Builder/TracerProviderBuilderSdk.cs	94.64% <94.64%> (-5.36%)	⬇️
...pendencyInjectionMeterProviderBuilderExtensions.cs	100.00% <100.00%> (ø)
...encyInjectionMetricsServiceCollectionExtensions.cs	100.00% <100.00%> (ø)
... and 114 more

[utpilla]

Should we call this project OpenTelemetry.Extensions.DependencyInjection?

[alanwest]

Related to #3469 (specifically the section titled "Integrations for specific application frameworks or libraries")

I agree, I like OpenTelemetry.Extensions.DependencyInjection. The package is mostly extensions involving IServiceCollection and I'm pretty sure we don't see a future where it would deviate from this. There are a few extensions from IServiceProvider as well, but I think this is ok.

[CodeBlanch]

Updated with rename to: OpenTelemetry.Extensions.DependencyInjection

[alanwest]

Normally the namespace would match the namespace of the thing being extended. In this case IServiceCollection, so Microsoft.Extensions.DependencyInjection.

In speaking with @CodeBlanch offline, he said he intentionally chose OpenTelemetry.Metrics here because methods like ConfigureOpenTelemetryMeterProvider are for more advanced use cases and he wanted to make it a little less likely these extensions would pop up in intellisense for more common use cases.

I think this is good reasoning, however, what if we took it a bit further and named it OpenTelemetry.Metrics.AdvancedExtensions (ugh... naming is hard)? Users would have to be pretty intentional that they do in fact want to use them.

[utpilla]

.NET team has a guidance on naming: https://learn.microsoft.com/en-us/dotnet/core/extensions/options-library-authors#namespace-guidance, according to which they discourage people from using Microsoft.Extensions.DependencyInjection as the namespace.

OpenTelemetry.Metrics.AdvancedExtensions is good or we could have OpenTelemetry.AdvancedExtensions.Metrics 😀

[utpilla]

Or they could just not use ConfigureOpenTelemetryMeterProvider after reading the documentation which would explicitly say it's for library authors?

[CodeBlanch]

Just to add a little more to the discussion, in addition to changing the namespaces of these methods I also...

• Changed the names. They used to be "ConfigureOpenTelemetryTracing/Metrics" and now they are "ConfigureOpenTelemetryTracerProvider/MeterProvider". I did that to intentionally scare away casual users. Hopefully they don't know what those things are and shy away from these methods in favor of the more friendly AddOpenTelemetry in their intellisense.

• Changed the signatures. They used to receive a simple delegate like Action<TracerProviderBuilder> now they receive the service provider as well like Action<IServiceProvider, TracerProviderBuilder> the goal there was to also scare away casual users looking for something easy 👻

PS: I am not opposed to moving them again to some sub namespaces, just wanted to point out these changes.

[alanwest]

they discourage people from using Microsoft.Extensions.DependencyInjection as the namespace.

Interesting! I thought I've read guidance stating the opposite before. Though maybe this is an exception?

In that case, we probably want to reconsider our use of Microsoft.Extensions.DependencyInjection namespace for all the other classes in this PR?

Though what namespace should StartWithHost be in? Either OpenTelemetry.Extensions.DependencyInjection or OpenTelemetry.Extensions.Hosting? If we did OpenTelemetry.Extensions.Hosting then I think it would be very hard to find.

[alanwest]

Hmm... ok so maybe I was mistaken, this PR does not add anything to the Microsoft.Extensions.DependencyInjection namespace

[CodeBlanch]

Hmm... ok so maybe I was mistaken, this PR does not add anything to the Microsoft.Extensions.DependencyInjection namespace

I was chatting with @utpilla offline about this. It was not a goal of this work to be compliant with the guidance, but it just kind of happened 🤣 The reason was we now have AddOpenTelemetry which returns OpenTelemetryBuilder. Extensions like StartWithHost target OpenTelemetryBuilder. To make it work nicely for users, everything should be in the same namespace. So it was either put everything in Microsoft.Extensions.DependencyInjection or everything in OpenTelemetry. The later felt more appropriate to me so that's what landed on the PR.

The only thing left in the MEDI namespace should be the hosting extensions which are now marked Obsolete.

[bravecobra]

Is the extra required StartWithHost() call documented somewhere as a breaking change? It took me a couple of hours to figure out why my metrics and traces weren't coming through after upgrading the packages from 1.3.x to 1.4.x-rcx. It also doesn't mention it in the changelog of 1.4.0-rc.1 as a breaking change either which threw me off.
I'm sure more people will run into this issue, so mentioning it as breaking change in the changelog will be welcome as it prevents tracing and metrics from coming through by simply upgrading the package versions.
I'm sure the docs and their example code will be updated as well once 1.4.x ships.

[CodeBlanch]

@bravecobra I'm not sure what you mean by breaking because StartWithHost isn't required. Can you share more detail about what you were doing before and what you tried to do on the new version?

StartWithHost does the same thing as AddOpenTelemetryTracing or AddOpenTelemetryMetrics (registers an IHostedService).

Old style:

appBuilder.Services.AddOpenTelemetryTracing(/* tracing configuration code */);
appBuilder.Services.AddOpenTelemetryMetrics(/* metrics configuration code */);

New style:

appBuilder.Services.AddOpenTelemetry()
   .WithTracing(/* tracing configuration code */)
   .WithMetrics(/* metrics configuration code */)
   .StartWithHost();

I'm sure the docs and their example code will be updated as well once 1.4.x ships.

Should already be done (in this repo). You see somewhere I missed?

• Hosting README

• AspNetCore Example

The other examples and docs as well as tests should be smiliar. Also the obsolete warning has a hint.

If you are talking about the open telemetry site, I'm not sure how to update that. Need to look into it 😄

[bravecobra]

Well, it seems I ran into problems, assuming that the OtlpExporterOptions were distinct per exporter (traces, metrics, etc..). This is no longer the case but it worked in 1.3.x.

Dependency injection wise each AddOtlpExporter() call overrides the previous registered OtlpExporterOptions as the default name of the registration is an empty string. As I was setting the endpoints for both metrics and traces, only the last registered version survived in DI. The only way to be able to set the endpoints (including the trailing path (v1/metrics and /v1/traces)), is to name the OtlpExportOptions.

The easiest way to reproduce this, is with the AspNetCore example project, but exporting through HTTP (4318) instead of gRPC (4317) to an otel collector in Docker. It'll start throwing exceptions for the traces, since the metrics options are the last to be registered and the endpoint are set.

If you don't know that those options are shared between traces and metrics unless you name them given the endpoints are being set, you'll run into issues easily.

So in short, if you have multiple AddOtlpExporter() calls and you are setting the endpoints, you need to name the OtlpExportOptions as well, because if you don't, there is only 1 registered in DI and it'll be the wrong one for one of them. Secondly, the extra paths are only appended if you don't touch the endpoints, again something is counter-intuitive, but that was already the case in the 1.3.x.

I had to check out this project to debug the issue as my code (https://github.com/bravecobra/emojivoto-dotnet) looked very similar to the examples project assuming that the examples project was working. It didn't when using Http.

I assume lots of people will run into similar issues.

[CodeBlanch]

@bravecobra Thanks for that write up! Same issue as #4043 I think. We should definitely do something about this.

/cc @alanwest

[bravecobra]

Yes, that seems to be exactly the same case I encountered.