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

Ability to register DI services inside of TracerProviderBuilder #894

Closed
ejsmith opened this issue Jul 23, 2020 · 12 comments
Closed

Ability to register DI services inside of TracerProviderBuilder #894

ejsmith opened this issue Jul 23, 2020 · 12 comments
Labels
enhancement New feature or request

Comments

@ejsmith
Copy link
Contributor

ejsmith commented Jul 23, 2020

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?

@ejsmith ejsmith added the enhancement New feature or request label Jul 23, 2020
@ejsmith
Copy link
Contributor Author

ejsmith commented Jul 23, 2020

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
Copy link
Member

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
Copy link
Contributor Author

ejsmith commented Jul 23, 2020

@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
Copy link
Member

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
Copy link
Contributor Author

ejsmith commented Jul 23, 2020

@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
Copy link
Member

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
Copy link
Contributor Author

ejsmith commented Jul 23, 2020

@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
Copy link
Contributor Author

ejsmith commented Mar 5, 2021

@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
Copy link
Member

@ejsmith Some thoughts...

  • Are you saying add IServiceCollection on TracerBuilder? I don't think forcing a Microsoft.Extensions.DependencyInjection.Abstractions dependency on everyone will be that well-received. For example, I'm using OpenTelemetry .NET in a .NET Framework 4.8 application using Unity for dependency injection. If TracerProviderBuilder suddenly needs IServiceCollection what do I do?

  • If you are saying somehow expose IServiceCollection on TracerBuilder only when OpenTelemetry.Extensions.Hosting is used, I'm all for that, it would be a great addition!

  • I think what @cijothomas is saying is there is a way to make it work with the current API. Look at the ctor for InMemoryExporter:

public InMemoryExporter(ICollection<T> exportedItems)

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
Copy link
Contributor Author

ejsmith commented Mar 5, 2021

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
Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants