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

Dependency injection support #1889

Merged
merged 19 commits into from
Mar 26, 2021

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Mar 9, 2021

This has been a hot topic lately so I took a stab at making it work. This isn't done, but enough of it is there to demonstrate the concept. Looking for feedback, please fire away!

Design Considerations

  • The main goal is for only OpenTelemetry.Extensions.Hosting to have a dependency on anything in Microsoft.Extensions.* suite. Turns out that dependency is already there so the main goal now is to keep .NET Framework users away from having to use IServiceProvider to build TracerProvider. You should only need that if you are using Hosting.

Public API Changes

SDK

    // TracerProviderBuilderSdk renamed TracerProviderBuilderBase and is now public so
    // Hosting library can use it as a base class. Tried to avoid this but lots of things with logic like:
    //   if (tracerProviderBuilder is TracerProviderBuilderSdk tracerProviderBuilderSdk)
    public abstract class TracerProviderBuilderBase : TracerProviderBuilder
    {
        // Already existed but is now public because the class is public
        public override TracerProviderBuilder AddInstrumentation<TInstrumentation>(
            Func<TInstrumentation> instrumentationFactory)
            where TInstrumentation : class

        // Already existed but is now public because the class is public
        public override TracerProviderBuilder AddSource(params string[] names)

        // TracerProviderBuilderHosting needs a way to register instrumentation without the generic
        protected TracerProviderBuilder AddInstrumentation(
            string instrumentationName,
            string instrumentationVersion,
            Func<object> instrumentationFactory)

        // Needed so hosting builder can call the base builder without using the extension which has a check
        // preventing it from executing for IDeferredTracerProviderBuilders
        protected TracerProvider Build()
    }

    // A new interface to enable libraries with an SDK-dependency to perform 
    // late-bound TracerProviderBuilder configuration once the IServiceProvider
    // is available.
    public interface IDeferredTracerProviderBuilder
    {
        IServiceCollection Services { get; }

        TracerProviderBuilder Configure(Action<IServiceProvider, TracerProviderBuilder> configure);
    }

Hosting

    // These extensions enable IServiceProvider registration scenarios
    public static class TracerProviderBuilderExtensions
    {
        public static TracerProviderBuilder AddInstrumentation<T>(this TracerProviderBuilder tracerProviderBuilder)
            where T : class

        public static TracerProviderBuilder AddProcessor<T>(this TracerProviderBuilder tracerProviderBuilder)
            where T : BaseProcessor<Activity>

        public static TracerProviderBuilder SetSampler<T>(this TracerProviderBuilder tracerProviderBuilder)
            where T : Sampler

        public static TracerProviderBuilder Configure(this TracerProviderBuilder tracerProviderBuilder, Action<IServiceProvider, TracerProviderBuilder> configure)

        public static TracerProvider Build(this TracerProviderBuilder tracerProviderBuilder)
    }

Usage Examples

Binding exporter options to IConfiguration (#101, #343):

    services.AddOpenTelemetryTracing((builder) => builder
        .AddJaegerExporter());
    services.Configure<JaegerExporterOptions>(this.Configuration.GetSection("Jaeger"));

Registering a Processor using DI:

    services.AddOpenTelemetryTracing((builder) => builder
        .AddProcessor<MyProcessor>());

    private class MyProcessor : BaseProcessor<Activity>
    {
        private readonly ILogger<MyProcessor> log;

        public MyProcessor(ILogger<MyProcessor> log)
        {
            this.log = log ?? throw new ArgumentNullException(nameof(log));
        }
    }

Extension method that also registers services (#894):

    services.AddOpenTelemetryTracing((builder) => builder
        .AddMyProcessor());

    public static class MyProcessorExtensions
    {
        public static TracerProviderBuilder AddMyProcessor(this TracerProviderBuilder tracerProviderBuilder)
        {
            if (tracerProviderBuilder is IDeferredTracerProviderBuilder deferredTracerProviderBuilder)
            {
                deferredTracerProviderBuilder.Services.AddHostedService<MyHostedService>();
                deferredTracerProviderBuilder.Services.AddSingleton<MyService>();
                deferredTracerProviderBuilder.AddProcessor<MyProcessor>();
            }

            return tracerProviderBuilder;
        }
    }

Implementation Examples

This is how JaegerExporter can use dependency injection to get IOptions<JaegerExporterOptions> from the IServiceProvider (if the application is using it).

    public static TracerProviderBuilder AddJaegerExporter(this TracerProviderBuilder builder, Action<JaegerExporterOptions> configure = null)
    {
        if (builder == null)
        {
            throw new ArgumentNullException(nameof(builder));
        }

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

        return AddJaegerExporter(builder, new JaegerExporterOptions(), configure);
    }

    private static TracerProviderBuilder AddJaegerExporter(TracerProviderBuilder builder, JaegerExporterOptions options, Action<JaegerExporterOptions> configure = null)
    {
        configure?.Invoke(options);

        var jaegerExporter = new JaegerExporter(options);

        if (options.ExportProcessorType == ExportProcessorType.Simple)
        {
            return builder.AddProcessor(new SimpleActivityExportProcessor(jaegerExporter));
        }
        else
        {
            return builder.AddProcessor(new BatchActivityExportProcessor(
                jaegerExporter,
                options.BatchExportProcessorOptions.MaxQueueSize,
                options.BatchExportProcessorOptions.ScheduledDelayMilliseconds,
                options.BatchExportProcessorOptions.ExporterTimeoutMilliseconds,
                options.BatchExportProcessorOptions.MaxExportBatchSize));
        }
    }

@ejsmith
Copy link
Contributor

ejsmith commented Mar 10, 2021

Looks interesting. Not seeing how I would go about registering services (IServicesCollection) during the build phase. I will check this branch out and take a look.

@CodeBlanch
Copy link
Member Author

@ejsmith

Looks interesting. Not seeing how I would go about registering services (IServicesCollection) during the build phase. I will check this branch out and take a look.

Ya, you're right, it only has the IServiceProvider. I'll keep messing with it and see if I can make that work too. Feel free to ping me on slack if you have any ideas. Or competing PR is also welcome. I feel like this is possible but it might take a couple of tries 😄

@CodeBlanch
Copy link
Member Author

@ejsmith I just pushed a new extension method called ConfigureOpenTelemetryTracing for your use case. Let me know what you think about that.

TracerProviderBuilder doesn't exist until IServiceProvider is ready. I think the pattern you are looking for is the builder lives during the IServiceCollection phase and then after Build you can no longer modify the services. I don't disagree with you, but all of the existing methods "adding" or "setting" things on the builder would need to be reworked to make that happen. What this new ConfigureOpenTelemetryTracing extension method does is allow you configure IServiceCollection and then register a callback for the "Build" phase.

Example...

services.AddOpenTelemetryTracing((builder) => builder
   .SetResourceBuilder(ResourceBuilder.CreateDefault().AddService(this.Configuration.GetValue<string>("Jaeger:ServiceName")));
   .AddJaegerExporter());
services.Configure<JaegerExporterOptions>(this.Configuration.GetSection("Jaeger"));
services.AddOpenTelemetryEventLogging(); // Pretend this is the new thing from your PR. 

AddOpenTelemetryEventLogging extension looks something like this...

public static IServiceCollection AddOpenTelemetryEventLogging(this IServiceCollection services) {
   services.AddHostedService<MyHostedService>();
   services.AddSingleton<MyService>();
   services.ConfigureOpenTelemetryTracing((sp, builder) => builder.AddProcessor<MyProcessor>()); // This will be called with the other stuff from above when the TracerProvider is being built up.
}

I know it isn't exactly what you were looking for, but it would work?

@ejsmith
Copy link
Contributor

ejsmith commented Mar 12, 2021

I created a PR to show what I am thinking for being able to separate the configuration and construction phases which I think would help with this PR. #1901

Comment on lines 41 to 49
if (builder is IDeferredTracerBuilder deferredTracerBuilder)
{
return deferredTracerBuilder.Configure((sp, builder) =>
{
AddZipkinExporter(builder, sp.GetOptions<ZipkinExporterOptions>(), configure);
});
}

return AddZipkinExporter(builder, new ZipkinExporterOptions(), configure);
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally don't like how this approach is causing all exporters and instrumentations to have to type check the builder and then have different code paths. IMO, it shouldn't be optional for these exporters and instrumentations to support separating configuration and construction phases. It should be mandatory that they register things with a deferred approach so that we can keep configuration and construction phases cleanly separated.

Copy link
Contributor

@ejsmith ejsmith Mar 14, 2021

Choose a reason for hiding this comment

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

I would personally prefer if we could force all of them to be registered with lambdas and not even make it an option to immediately new them up when configuring. Using a factory method approach for configuration is compatible with all targeted platforms without taking a dependency on Microsoft.Extensions.DependencyInjection for the core library while still allowing a DI approach to be implemented in a platform that supports it like the hosting package.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of everything on this PR, I'm the least happy with this bit. But I don't think it is possible to do it any other way 😭 It's not really a problem of separating configuration and construction, this is about the inversion of dependencies we've created for ourselves. To do it "properly" we would need this ctor: public ZipkinExporter(IOptions<ZipkinExporterOptions> options). But that of course requires a direct dependency.

{
private readonly List<InstrumentationFactory> instrumentationFactories = new List<InstrumentationFactory>();
private readonly List<Type> processorTypes = new List<Type>();
private readonly List<Action<IServiceProvider, TracerProviderBuilder>> configurationActions = new List<Action<IServiceProvider, TracerProviderBuilder>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I like having generic configuration actions like this.

@CodeBlanch
Copy link
Member Author

@ejsmith I'll leave some comments on #1901.

@CodeBlanch
Copy link
Member Author

Pushed some updates:

  • Switched to public abstract class TracerProviderBuilderBase so you can't new an instance. Previously version you could do new TracerProviderBuilderSdk but we want construction to happen through the static methods.
  • Since SDK already has a dependency on IOptions, I removed the reflection stuff. Made the IServiceProvider extensions internal.

@CodeBlanch
Copy link
Member Author

@alanwest @reyang @cijothomas @ejsmith I incorporated all the feedback I got from people (on PR and on Slack) please take a look and let me know if this is something worth completing (unit tests, documentation, etc.). I tried to make it as clean as I could with the smallest public API footprint possible while still providing the use cases and without forcing any users into IServiceProvider.

@ejsmith
Copy link
Contributor

ejsmith commented Mar 18, 2021

This is looking pretty good @CodeBlanch. I am going to try and play with the code soon, but I think it's a good approach.

@reyang
Copy link
Member

reyang commented Mar 18, 2021

@alanwest @reyang @cijothomas @ejsmith I incorporated all the feedback I got from people (on PR and on Slack) please take a look and let me know if this is something worth completing (unit tests, documentation, etc.). I tried to make it as clean as I could with the smallest public API footprint possible while still providing the use cases and without forcing any users into IServiceProvider.

The approach looks good to me 👍.
Instead of adding more to this PR (which is already a big one), I suggest that we put unit test / documentation in a follow up PR.

@CodeBlanch
Copy link
Member Author

Instead of adding more to this PR (which is already a big one), I suggest that we put unit test / documentation in a follow up PR.

Sounds good to me. There are a couple of broken tests I'll need to fix up. I'll do that but I'm going to leave this as a draft until @ejsmith has a chance to test out his options approach. We can go with whichever people like more.

if (tracerProviderBuilder is TracerProviderBuilderSdk tracerProviderBuilderSdk)
if (tracerProviderBuilder is IDeferredTracerProviderBuilder)
{
throw new NotSupportedException("DeferredTracerBuilder requires a ServiceProvider to build.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This can't be here because the Build method from the TracerProviderBuilderHosting class calls this method and then it blows up the build process.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops! Fixed it up by making the build on TracerProviderBuilderBase protected. I feel like we should keep this throw here to prevent anyone from accidentally using the hosting builder with the non-IServerProvider build extension?

@ejsmith
Copy link
Contributor

ejsmith commented Mar 18, 2021

Instead of adding more to this PR (which is already a big one), I suggest that we put unit test / documentation in a follow up PR.

Sounds good to me. There are a couple of broken tests I'll need to fix up. I'll do that but I'm going to leave this as a draft until @ejsmith has a chance to test out his options approach. We can go with whichever people like more.

This approach is good with me @CodeBlanch. I experimented with a pretty similar approach after you said you didn't really like the options being public. I also realized that we need need to use the builder immediately and we can have a hosting builder that lives by itself which is what you are doing. I think there are some flaws in the current public API that limit things, but I know that we can't change those now after it was marked stable with 1.0. So I think your approach is the best one.

@codecov
Copy link

codecov bot commented Mar 19, 2021

Codecov Report

Merging #1889 (0ce3374) into main (5f1acbf) will decrease coverage by 0.66%.
The diff coverage is 54.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1889      +/-   ##
==========================================
- Coverage   84.39%   83.73%   -0.67%     
==========================================
  Files         188      192       +4     
  Lines        6108     6159      +51     
==========================================
+ Hits         5155     5157       +2     
- Misses        953     1002      +49     
Impacted Files Coverage Δ
....Exporter.Jaeger/JaegerExporterHelperExtensions.cs 11.11% <0.00%> (-4.28%) ⬇️
....Exporter.Zipkin/ZipkinExporterHelperExtensions.cs 11.11% <0.00%> (-4.28%) ⬇️
...penTelemetry/Internal/ServiceProviderExtensions.cs 0.00% <0.00%> (ø)
...s.Hosting/Trace/TracerProviderBuilderExtensions.cs 17.64% <17.64%> (ø)
...ing/Implementation/TracerProviderBuilderHosting.cs 38.46% <38.46%> (ø)
...c/OpenTelemetry/Trace/TracerProviderBuilderBase.cs 85.48% <85.48%> (ø)
...Telemetry/Trace/TracerProviderBuilderExtensions.cs 95.00% <92.30%> (-5.00%) ⬇️
...c/OpenTelemetry.Api/Trace/TracerProviderBuilder.cs 100.00% <100.00%> (ø)
...ensions.Hosting/OpenTelemetryServicesExtensions.cs 64.70% <100.00%> (-10.91%) ⬇️
...rc/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs 100.00% <100.00%> (+10.16%) ⬆️
... and 9 more

@CodeBlanch CodeBlanch changed the title Draft: Dependency injection support Dependency injection support Mar 19, 2021
@CodeBlanch CodeBlanch marked this pull request as ready for review March 19, 2021 04:11
@CodeBlanch CodeBlanch requested a review from a team March 19, 2021 04:11
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.

4 participants