opentelemetry-dotnet icon indicating copy to clipboard operation
opentelemetry-dotnet copied to clipboard

[Tracing] Improve dependency injection support in tracing build-up using SDK

Open CodeBlanch opened this issue 3 years ago • 1 comments

Fixes #1215 - Multiple registrations no longer allowed Relates to #3086 - Adds a mechanism to do this (see scenarios) Relates to #1642 - Moves all of the dependency bits into SDK, only thing in "hosting" is the IHostedService. May or may not help with the name 😄 Relates to #2980 - Makes it possible to solve this Relates to #2971 - Makes it possible to solve this

Changes

  • Moves dependency injection features based on ServiceCollection from hosting library directly into SDK so they are universally available. This is similar to what was done on #3504 for logs.

  • Enabled a few more detached scenarios.

  • We decided on the SIG meeting 8/2/2022 that multiple providers registered into the same service collection using the AddOpenTelemetryTracing extension would NOT be supported. This PR enforces that.

Public API

namespace OpenTelemetry.Trace
{
   public abstract class TracerProviderBuilderBase
   {
       // This ctor is used by the hosting builder to pass in the external services the builder is bound to
+      protected TracerProviderBuilderBase(IServiceCollection services) {}
       // This property is used by the hosting builder to set the external service provider once it is available
+      protected IServiceProvider? ServiceProvider { get; set; }
   }

   public static class TracerProviderBuilderExtensions
   {
       // All of these existed previously in Hosting, now it is part of the SDK and be used anywhere SDK is referenced
+      public static TracerProviderBuilder AddInstrumentation<T>(this TracerProviderBuilder tracerProviderBuilder) {}
+      public static TracerProviderBuilder AddProcessor<T>(this TracerProviderBuilder tracerProviderBuilder) where T : BaseProcessor<Activity> {}
+      public static TracerProviderBuilder ConfigureBuilder(this TracerProviderBuilder tracerProviderBuilder, Action<IServiceProvider, TracerProviderBuilder> configure) {}
+      public static TracerProviderBuilder ConfigureServices(this TracerProviderBuilder tracerProviderBuilder, Action<IServiceCollection> configure) {}
+      public static TracerProviderBuilder SetSampler<T>(this TracerProviderBuilder tracerProviderBuilder) where T : Sampler {}
       // These are provided so no one breaks upgrading:
+      [Obsolete] public static TracerProviderBuilder Configure(this TracerProviderBuilder tracerProviderBuilder, Action<IServiceProvider, TracerProviderBuilder> configure) {}
+      [Obsolete] public static IServiceCollection? GetServices(this TracerProviderBuilder tracerProviderBuilder) {}

        // These are new
+       public static TracerProviderBuilder AddExporter(this TracerProviderBuilder tracerProviderBuilder, ExportProcessorType exportProcessorType, BaseExporter<Activity> exporter) {}
+       public static TracerProviderBuilder AddExporter(this TracerProviderBuilder tracerProviderBuilder, ExportProcessorType exportProcessorType, BaseExporter<Activity> exporter, Action<ExportActivityProcessorOptions> configure) {}
+       public static TracerProviderBuilder AddExporter<T>(this TracerProviderBuilder tracerProviderBuilder, ExportProcessorType exportProcessorType) where T : BaseExporter<Activity> {}
+       public static TracerProviderBuilder AddExporter<T>(this TracerProviderBuilder tracerProviderBuilder, ExportProcessorType exportProcessorType, Action<ExportActivityProcessorOptions> configure) where T : BaseExporter<Activity> {}
   }

    // This class is similar to MetricReaderOptions and is used by AddExporter extensions
+   public class ExportActivityProcessorOptions
+   {
+       public ExportProcessorType ExportProcessorType { get; set; }
+       public BatchExportActivityProcessorOptions BatchExportProcessorOptions { get; set; }
+   }
}

Existing scenarios made available in SDK

All of this stuff you could already do using the hosting library. But now you can do it just with the SDK. That makes it more universal. Notice below the root is Sdk.CreateTracerProviderBuilder previously that builder had zero support for any kind dependency injection. Now it has the same surface as the builder returned by the AddOpenTelemetryTracing API. All of this will work for .NET Framework as well as .NET/Core.

using var tracerProvider = Sdk.CreateTracerProviderBuilder()
    // Scenario 1: Register services...
    .ConfigureServices(services =>
    {
        services.AddSingleton<MyCustomExporter>();
    })
    // Scenario 2: Add a processor retrieved through DI...
    .AddProcessor<MyCustomProcessor>()
    // Scenario 3: Add a sampler retrieved through DI...
    .SetSampler<MyCustomSampler>()
    // Scenario 4: Configure the builder once the service provider is available...
    .ConfigureBuilder((sp, builder) =>
    {
        builder.AddProcessor(new SimpleActivityExportProcessor(sp.GetRequiredService<MyCustomExporter>()));
    })

The hidden value being created here is really for library/extension authors. Now I can build a library with this extension...

public static TracerProviderBuilder AddMyLibraryFeature(this TracerProviderBuilder builder)
{
   return builder
      .ConfigureServices(services =>
      {
           services.AddSingleton<MyDependencyA>();
           services.AddSingleton<MyDependencyB>();
      })
      .AddProcessor<MyCustomProcessor>();
}

...and it will work equally well for .NET Framework users as it will .NET/Core users, with only a reference to SDK.

It also allows us to clean up some of the strange registration code we have. For example this...

public static TracerProviderBuilder AddJaegerExporter(this TracerProviderBuilder builder, Action<JaegerExporterOptions> configure = null)
{
    Guard.ThrowIfNull(builder);

    if (builder is IDeferredTracerProviderBuilder deferredTracerProviderBuilder)
    {
        return deferredTracerProviderBuilder.Configure((sp, builder) =>
        {
            AddJaegerExporter(builder, sp.GetOptions<JaegerExporterOptions>(), configure, sp);
        });
    }

    return AddJaegerExporter(builder, new JaegerExporterOptions(), configure, serviceProvider: null);
}

...becomes this...

public static TracerProviderBuilder AddJaegerExporter(this TracerProviderBuilder builder, Action<JaegerExporterOptions> configure = null)
{
    Guard.ThrowIfNull(builder);

    if (configure != null)
    {
        builder.ConfigureServices(services => services.Configure(configure));
    }

    return builder.ConfigureBuilder((sp, builder) =>
    {
        var options = sp.GetRequiredService<IOptions<JaegerExporterOptions>>().Value;

        AddJaegerExporter(builder, options, sp);
    });
}

New scenarios enabled

Register an exporter with the builder

This can be done either by providing the exporter instance or using dependency injection.

builder.AddExporter<MyExporter>(ExportProcessorType.Batch);

The BatchExportActivityProcessorOptions used (when batching is configured) is retrieved through the options API. Users will be able to bind BatchExportActivityProcessorOptions to IConfiguration and manage it through the configuration pipeline (JSON, envvars, command-line, whatever they happen to have configured). There is a bit more work to smooth out how environment variables are retrieved (which is what #2980 is about) but I'll tackle that separately.

Register a processor through services which is automatically configured

builder.Services.AddOpenTelemetryTracing();
builder.Services.AddSingleton<BaseProcessor<Activity>, MyCustomProcessor>();

Register a sampler through services which is automatically configured

builder.Services.AddOpenTelemetryTracing();
builder.Services.AddSingleton<Sampler, MyCustomSampler>();

Note: A NotSupportedException is thrown during build if a sampler is set directly using a SetSampler call and also through services.

Swiss-army-knife detached configuration method

Some users like @MartinJT have asked for a way to configure things completely detached from the builder. You can now do this:

services.AddSingleton<Action<IServiceProvider, TracerProviderBuilder>>(MyBuilderConfigurationMethod);

The builder will now look for any actions registered and call them during build-up.

That feature can be used to make all kinds of registration extensions off of IServiceCollection. For example @MartinJT has asked for a way to globally register an exporter. Using that extension point, users could do this:

public static IServiceCollection AddOpenTelemetrySimpleExporter<T>(this IServiceCollection services)
    where T : BaseExporter<Activity>
{
    return services
       .AddSingleton<T>()
       .AddSingleton<Action<IServiceProvider, TracerProviderBuilder>>((sp, builder) =>
       {
           builder.AddProcessor(new SimpleActivityExportProcessor(sp.GetRequiredService<T>()));
       });
}

Maybe we should have such extensions in the SDK itself. Not going to tackle that here!

#3086 is asking for an AddOpenTelemetryTracing extension which exposes the final IServiceProvider. I'm hesitant to do that because I think it would be confusing to expose these two methods:

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

There are huge non-trivial behavioral differences between those two methods which the signatures beguile.

However using this extension point, it would be simple for users to make such an extension exist if they so-desire:

public static IServiceCollection AddOpenTelemetryTracing(this IServiceCollection services, Action<IServiceProvider, TracerProviderBuilder> configure)
{
    return services
       .AddOpenTelemetryTracing()
       .AddSingleton<Action<IServiceProvider, TracerProviderBuilder>>(configure);
}

TODOs

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

CodeBlanch avatar Aug 03 '22 22:08 CodeBlanch

Codecov Report

Merging #3533 (c91f02d) into main (8700d5d) will increase coverage by 0.31%. The diff coverage is 92.95%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3533      +/-   ##
==========================================
+ Coverage   87.02%   87.34%   +0.31%     
==========================================
  Files         277      280       +3     
  Lines        9953    10056     +103     
==========================================
+ Hits         8662     8783     +121     
+ Misses       1291     1273      -18     
Impacted Files Coverage Δ
...s.Hosting/Trace/TracerProviderBuilderExtensions.cs 0.00% <0.00%> (-73.92%) :arrow_down:
...rc/OpenTelemetry/Trace/TracerProviderExtensions.cs 68.00% <ø> (ø)
...s.Hosting/Implementation/TelemetryHostedService.cs 73.33% <75.00%> (+4.10%) :arrow_up:
...racerProviderBuilderServiceCollectionExtensions.cs 85.71% <85.71%> (ø)
...lemetry/Trace/Builder/TracerProviderBuilderBase.cs 91.81% <91.81%> (ø)
...emetry/Trace/Builder/TracerProviderBuilderState.cs 94.73% <94.73%> (ø)
...y/Trace/Builder/TracerProviderBuilderExtensions.cs 97.56% <97.56%> (ø)
...ensions.Hosting/OpenTelemetryServicesExtensions.cs 77.77% <100.00%> (+6.34%) :arrow_up:
src/OpenTelemetry/BaseExportProcessor.cs 86.95% <100.00%> (+0.59%) :arrow_up:
src/OpenTelemetry/BatchExportProcessor.cs 84.11% <100.00%> (+1.86%) :arrow_up:
... and 23 more

codecov[bot] avatar Aug 04 '22 17:08 codecov[bot]

@cijothomas

We need to document/add some examples demonstrating the new capabilities ( i.e everything from PR desc. into docs/examples) later, so people can easily leverage these.

Totally! Once I am done getting traces/metrics/logs all squared away I'll take a stab at spinning up some documentation.

CodeBlanch avatar Aug 29 '22 20:08 CodeBlanch