open-telemetry / opentelemetry-dotnet

The OpenTelemetry .NET Client
https://opentelemetry.io
Apache License 2.0
3.22k stars 764 forks source link

Ability to register DI services inside of TracerProviderBuilder #894

Closed ejsmith closed 3 years ago

ejsmith commented 4 years ago

I am working on implementing an in memory exporter that will keep the last X or most expensive X traces in memory and then show them in an API endpoint in your app. The problem is that I want to register my exporter as a singleton in the DI container, but I don't have access to the ServicesCollection when I am calling AddProcessorPipeline inside of the TracerProviderBuilder. It is pretty common practice when creating fluent builder APIs like this one to allow access to the ServicesCollection. Is there any chance this ability can be added?

cijothomas commented 4 years ago

Would this help - https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry.Extensions.Hosting/OpenTelemetryServicesExtensions.cs#L59

ejsmith commented 4 years ago

No that doesn't help because I'm creating a new exporter that has a registration method that is an extension method on top of TracerProviderBuilder which does not have access to the ServicesCollection. I want to register my exporter as a singleton inside of the ServicesCollection.

cijothomas commented 4 years ago

Sorry I am not sure if I understand what you are trying to achieve. The below shows adding a custom exporter. And you can have access to IServiceProvider here, and retrieve any registered instance from there and pass it to custom Exporter.

// services.AddSomethingiwant().
services.AddOpenTelemetry((sp, builder) =>
            {
                // var fromServiceCollection = sp.GetService(); //retrieve somethingiwant
                builder.SetSampler(new AlwaysOnSampler());
                builder.AddProcessorPipeline((pipeline) => pipeline.SetExporter(new MyExporter(fromServiceCollection)));
            });

Let me know if this helps.

ejsmith commented 4 years ago

@cijothomas I'm not getting a service, I want to add a service to the collection. sp in your example is the IServiceProvider. I want access to the IServicesCollection to be able to add a new registration to the DI.

        public static TracerProviderBuilder UseInMemoryExporter(this TracerProviderBuilder builder, Action<InMemoryExporterOptions> configure = null, Action<ActivityProcessorPipelineBuilder> processorConfigure = null)
        {
            if (builder == null)
            {
                throw new ArgumentNullException(nameof(builder));
            }

            return builder.AddProcessorPipeline(pipeline =>
            {
                var exporterOptions = new InMemoryExporterOptions();
                configure?.Invoke(exporterOptions);

                var inMemoryExporter = new InMemoryExporter(exporterOptions);
                processorConfigure?.Invoke(pipeline);

                // I want to add this instance of the InMemoryExporter as a singleton to the DI

                pipeline.SetExporter(inMemoryExporter );
            });
        }

So I can do this:

            services.AddOpenTelemetry((sp, builder) => builder
                .AddRequestInstrumentation()
                .UseInMemoryExporter());

And then later in my app get access to that singleton instance of the InMemoryExporter

cijothomas commented 4 years ago

Thanks for clarifying. (I knew I understood some part wrongly :D)

Can you elaborate on the use case for application retrieving the InMemoryExporter? To dispose on shutdown? or do something else or does the app intend to the same instance of the exporter for other pipeline?

The following can achieve this, but defeats the purpose of extension method. Once I get the use case, we can try to come up with a better solution.

var exporterOptions = new InMemoryExporterOptions() {configure it here};
var memoryExporter = new InMemoryExporter(exporterOptions);
services.Add(memoryExporter);
services.AddOpenTelemetry((builder) =>
            {
                builder.AddProcessorPipeline((pipeline) => pipeline.SetExporter(memoryExporter));
            });

// I can retrieve exporter from DI
ejsmith commented 4 years ago

@cijothomas I am building an in memory exporter that collects and saves the most recent and most expensive traces and has the ability to show those traces by registering API endpionts. In order to get access to those to show from the application's API, I need to be able to get access to the singleton instance.

The example you show is what I am currently doing, but it's generally bad to create the instances during the app startup / registration phase and also makes it a lot more involved to setup than I'd like it to be ideally.

I feel like it's a pretty useful thing especially locally to have an easy way to collect and view traces without needed to have to figure out and setup something like zipkin, or jaeger or elastic APM. Eventually, I think it would be cool to create a nice HTML UI for viewing those traces but currently I am just showing them in a simple text format.

cijothomas commented 4 years ago

Okay. Understood the use case now. We need to see where and how to make this work nicely. I will get back to this after beta release. Meanwhile if you have proposals, please do share.

Also, please also look at ZPages exporter. Its doing in-memory "self contained" viewing ability.

ejsmith commented 4 years ago

@cijothomas sounds good. Looking forward to the beta release! Thank you for all the hard work. The more I dig into the library the more I am really impressed with what you guys have built. The code is very impressive.

I didn't know about ZPages. That looks interesting. I will check it out.

ejsmith commented 3 years ago

@cijothomas most builder patterns for the generic host have a IServiceCollection property on the builder object. The problem in OpenTelemetry is that the core packages support net452 which Microsoft.Extensions.DependencyInjection does not support. Would you consider a patch that added a IServiceCollection property on the builder, but only for the netstandard 2.0 or .net 4.6.1 targets?

CodeBlanch commented 3 years ago

@ejsmith Some thoughts...

It receives an externally managed collection for its data. You could basically do the same thing to achieve what you are looking for...

            MyState state = new MyState();
            services.AddSingleton(state);
            services.AddOpenTelemetry(builder => builder
                .AddRequestInstrumentation()
                .UseStateAwareExporter(state));

Not great because it is leaking some implementation details, but we could also wrap it up in an extension to hide that away:

            services.AddOpenTelemetry(builder => builder
                .AddRequestInstrumentation()
                .UseStateAwareExporter(services));

        public static TracerProviderBuilder AddMyStateAwareExporter(this TracerProviderBuilder builder, IServiceCollection services)
        {
            MyState state = new MyState();
            services.AddSingleton<MyState>(state);
            return builder.UseStateAwareExporter(state);
        }
ejsmith commented 3 years ago

Yes, I'm aware of the work around and I had to use that. But I'd like to try and figure out some way to make the TracerProviderBuilder support DI in both service registration as well as being able to get instrumentations be dependency injected. What if the root builder just had like a generic dictionary of items you could add to it? Or maybe we could create some sort of derived builder that is only used in the extensions hosting package and extension methods.

CodeBlanch commented 3 years ago

What if the root builder just had like a generic dictionary of items you could add to it? Or maybe we could create some sort of derived builder that is only used in the extensions hosting package and extension methods.

I'm not opposed to either thing! The only key requirement (for me) is that the dependency is localized to the hosting library (or we create a new assembly for this).