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

NotSupportedException is thrown during ConfigureBuilder #3753

Closed
hankovich opened this issue Oct 11, 2022 · 19 comments
Closed

NotSupportedException is thrown during ConfigureBuilder #3753

hankovich opened this issue Oct 11, 2022 · 19 comments
Labels
bug Something isn't working Stale Issues and pull requests which have been flagged for closing due to inactivity

Comments

@hankovich
Copy link

Bug Report

Packages:

<PackageReference Include="OpenTelemetry.Exporter.Console" Version="1.4.0-beta.1" />
<PackageReference Include="OpenTelemetry.Exporter.OpenTelemetryProtocol" Version="1.3.1" />
<PackageReference Include="OpenTelemetry.Extensions.Hosting" Version="1.0.0-rc9.7" />
<PackageReference Include="OpenTelemetry.Instrumentation.AspNetCore" Version="1.0.0-rc9.7" />
<PackageReference Include="OpenTelemetry.Instrumentation.ElasticsearchClient" Version="1.0.0-beta.3" />
<PackageReference Include="OpenTelemetry.Instrumentation.Hangfire" Version="1.0.0-beta.2" />
<PackageReference Include="OpenTelemetry.Instrumentation.Http" Version="1.0.0-rc9.7" />
<PackageReference Include="OpenTelemetry.Instrumentation.StackExchangeRedis" Version="1.0.0-rc9.7" />

Runtime version: .NET 7 RC 1

Symptom

I use a combination of AddOpenTelemetryTracing and ConfigureBuilder to emulate a missing overload mentioned in #3086 (I need an access to IServiceProvider to register tracing). It works fine and I can add all exporters and instrumentions. Hovewer, the approach fails if I pass Action<AspNetCoreInstrumentationOptions> to AddAspNetCoreInstrumentation.

What is the expected behavior?
No exceptions are thrown

What is the actual behavior?
NotSupporteException is thrown. I see it only in Debug, in Release it's swallowed, i.e. there're no logs, no errors and no externally visible exceptions. Open telemetry just disables itself (no traces are exported, the application continues handling requests).

Reproduce

services.AddOpenTelemetryTracing(builder =>
{
    builder.ConfigureBuilder(static (provider, nestedBuilder) =>
    {
        var tracingOptions = provider.GetRequiredService<IOptions<TracingOptions>>().Value;

        if (!tracingOptions.Enabled)
        {
            return;
        }

        nestedBuilder.AddConsoleExporter();

        var resourceBuilder = ResourceBuilder
            .CreateDefault()
            .AddService("name");

        nestedBuilder
            .AddSource("name")
            .SetResourceBuilder(resourceBuilder)
            .AddAspNetCoreInstrumentation(options => // .AddAspNetCoreInstrumentation() works fine
            {
                options.Filter = context => true;
            });
    });
});

As a workaround I can call .AddAspNetCoreInstrumentation() and add

services.Configure<AspNetCoreInstrumentationOptions>(options =>
{
    options.Filter = context => true;
});

Additional Context

System.NotSupportedException: 'Services cannot be configured after ServiceProvider has been created.'
   at OpenTelemetry.Trace.TracerProviderBuilderBase.ConfigureServices(Action`1 configure)
   at OpenTelemetry.Trace.TracerProviderBuilderExtensions.ConfigureServices(TracerProviderBuilder tracerProviderBuilder, Action`1 configure)
   at OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(TracerProviderBuilder builder, String name, Action`1 configureAspNetCoreInstrumentationOptions)
   at OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(TracerProviderBuilder builder, Action`1 configureAspNetCoreInstrumentationOptions)
   at Host.Tracing.Extensions.ServiceCollectionExtensions.<>c.<AddTracing>b__0_5(IServiceProvider provider, TracerProviderBuilder nestedBuilder) in C:\Users\Hello\source\repos\common\src\Host\Tracing\Extensions\ServiceCollectionExtensions.cs:line 52
   at OpenTelemetry.Trace.TracerProviderBuilderServiceCollectionHelper.<>c__DisplayClass0_0.<RegisterConfigureBuilderCallback>b__0(IServiceProvider sp, TracerProviderBuilderState state)
   at OpenTelemetry.Trace.TracerProviderBuilderServiceCollectionHelper.ConfigureTracerProviderBuilderStateCallbackRegistration.Configure(IServiceProvider serviceProvider, TracerProviderBuilderState state)
   at OpenTelemetry.Trace.TracerProviderBuilderServiceCollectionHelper.InvokeRegisteredConfigureStateCallbacks(IServiceProvider serviceProvider, TracerProviderBuilderState state)
   at OpenTelemetry.Trace.TracerProviderSdk..ctor(IServiceProvider serviceProvider, Boolean ownsServiceProvider)
   at OpenTelemetry.Trace.TracerProviderBuilderBase.<>c.<.ctor>b__4_0(IServiceProvider sp)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitRootCache(ServiceCallSite callSite, RuntimeResolverContext context)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite callSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.CreateServiceAccessor(Type serviceType)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(Type serviceType, ServiceProviderEngineScope serviceProviderEngineScope)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetService[T](IServiceProvider provider)
   at OpenTelemetry.Extensions.Hosting.Implementation.TelemetryHostedService.Initialize(IServiceProvider serviceProvider)
   at OpenTelemetry.Extensions.Hosting.Implementation.TelemetryHostedService.StartAsync(CancellationToken cancellationToken)
@hankovich hankovich added the bug Something isn't working label Oct 11, 2022
@cijothomas
Copy link
Member

@CodeBlanch Can you take a look?

@CodeBlanch
Copy link
Member

Sadly, this is working as designed! ConfigureBuilder fires after the IServiceProvider has been created from IServiceCollection. Services can't be added to the IServiceProvider once is has been created. The way the Options API works is configuration delegates are added to the IServiceCollection and then executed back when you access the final instance. That's why it is blowing up. AddAspNetCoreInstrumentation is trying to register a "service" (in this case configuration delegate) too late in the flow. Not sure what we could do about that.

The OP has hinted at a potential solution, but let me add it to the full snippet:

services.AddOpenTelemetryTracing(builder =>
{
    // Register delegate to configure options while the IServiceCollection is available...
    builder.ConfigureServices(services => {
        services.Configure<AspNetCoreInstrumentationOptions>(options => {
           options.Filter = context => true;
        });
    });
    // Do IServiceProvider work...
    builder.ConfigureBuilder(static (provider, nestedBuilder) =>
    {
        var tracingOptions = provider.GetRequiredService<IOptions<TracingOptions>>().Value;

        if (!tracingOptions.Enabled)
        {
            return;
        }

        nestedBuilder.AddConsoleExporter();

        var resourceBuilder = ResourceBuilder
            .CreateDefault()
            .AddService("name");

        nestedBuilder
            .AddSource("name")
            .SetResourceBuilder(resourceBuilder)
            .AddAspNetCoreInstrumentation();
    });
});

@hankovich It looks like you want to disable tracing based on your runtime configuration. This really isn't the use case ConfigureBuilder was intended to solve. Would something like this work instead?

if (builder.Configuration.GetValue<bool>("OpenTelemetry:Tracing:Enabled", true))
{
    builder.Services.AddOpenTelemetryTracing(options =>
    {
         // Configure only if enabled
    });
}

Just FYI we would like to have a built-in mechanism for disabling the SDK, but we're not there yet! We're kind of kicking around ideas for example: #3639

@julealgon
Copy link

Folks......... please, you gotta be careful with these random breaking changes...

I had code since May that was relying on GetServices to register extra services in the container, and suddenly (I actually don't know when precisely) that just doesn't work anymore?

I just spent nearly 2 entire days debugging why my traces were not showing up for a specific project. Doing all sorts of crazy tests, trying to eliminate as many factors as possible. I thought it was something in the Azure AppService environment that might have changed with the shift to .NET7, but no.... a breaking change on this library started to silently throw an exception that caused my traces to vanish completely.

Took me a very long time to finally be able to reproduce the issue locally, because it was related to a conditional call based on "Environment != 'Development'". I even set up an ubuntu distro locally using WSL to see if running in that environment would reproduce the problem I was seeing...

When I finally changed the environment to "Production" locally, I then immediately saw this exception. I thought "huh maybe this new ConfigureServices method from the newer version is not really equivalent to the old GetServices()"? But nay... switching back to GetServices() with a disposal warning suppression resulted in the exact same exception. On something that hasn't changed at all in several months besides package updates and .NET 6 to 7 migration 😭

To top it all of, you decide to fucking swallow exceptions like this...... why would you do that, that's an incredibly bad practice. This is a critical mistake (the exception message clearly says "you shouldn't do this"), by swallowing this exception you make it nearly impossible to detect the problem. I totally understand ignoring some configuration things, using the standard path if things are not setup, etc, but you can't just swallow critical exceptions like this.

So frustrating... I just can't believe I wasted so much time on something that would be trivially found if the exception was just thrown and the app crashed.

I've mentioned earlier how terrible I thought the EventSource-based custom diagnostics thing was (I still hope you'll abandon all of that at some point and migrate towards ILogger), but this here takes the cake.

@CodeBlanch
Copy link
Member

Wow that was spirited! Would you be able to provide some code which reproduces the problem? Something which works on 1.3.1 but not on 1.4.0. Happy to look into it.

@julealgon
Copy link

Wow that was spirited!

I'm sorry if that was a bit too harsh... I was just incredibly frustrated with the issue after spending so much time trying all sorts of crazy things.

Would you be able to provide some code which reproduces the problem? Something which works on 1.3.1 but not on 1.4.0. Happy to look into it.

When I went to try to reproduce the issue and find the exact version that caused a breaking change, I realized there was a bit more to the whole problem: turns out part of our functionality actually stopped working earlier than I suspected.

Take a look at this repo here (worker project):

Notes:

  • It uses OpenTelemetry.Extensions.Hosting v1.0.0-rc9.6
  • It conditionally registers stuff in the container using the Configure method in conjunction with GetServices on MeterProviderBuilder
  • When running the app, you can see that it emits activities as normal
  • However, registrations happening after GetServices are flat out ignored (this is the part I hadn't noticed in our main project), which mean the custom metric/counter do not show up (the decorator is never executed)
  • No exceptions are thrown anywhere here

If I take the exact same codebase, and update OpenTelemetry.Extensions.Hosting to v1.0.0-rc9.7:

  • I now get the NotSupportedException on the registrations after GetServices
  • The application still runs, but now traces are also gone (this is the key aspect)

So, code that was configuring metrics, suddenly makes all OTEL stop working completely.

This was made worse in our case because the issue would only occur when running the application in "Production" mode, as the IHostEnvironment check was using IsProduction to add the extra instrumentation.

For reference, I included the final solution (passing IHostEnvironment instead of resolving it) in a commented out method.

This is now the second time the OTEL library has made us waste more than 2 days in investigation work for weird defects. Whenever I find something in the web about issues, you guys always respond with "just enable the diagnostics and provide the logs". Please note that your custom diagnostics solution that relies on file system to both configure and save the logs, is not at all container/serverlss friendly: our application here is hosted in Azure AppService, which doesn't normally have you messing around with the file system.

I'll once again urge the team to reconsider this strategy of using a deactivated-by-default diagnostics solution that doesn't rely at all on ILogger and doesn't integrate seamlessly with modern applications. You are really making consumer's lives that much harder this way.

@CodeBlanch
Copy link
Member

@julealgon I discussed this with @alanwest and we agree that these types of issues should surface. I opened #4006 we'll see if the other maintainers agree 😄 For context, it isn't necessarily by choice that the exception was being swallowed. The OpenTelemetry Specification is super opinionated that SDKs should never crash the process once it is up and running. But I think in this case it is OK to throw because the process hasn't exactly started yet.

Using OpenTelemetry.Extensions.Hosting v1.0.0-rc9.6 (1.3 behavior)

  • Registrations happening after GetServices are flat out ignored (this is the part I hadn't noticed in our main project), which mean the custom metric/counter do not show up (the decorator is never executed)
  • No exceptions are thrown anywhere here

Using OpenTelemetry.Extensions.Hosting to v1.0.0-rc9.7 (1.4 behavior):

  • I now get the NotSupportedException on the registrations after GetServices

Agree with this! It used to silently fail and I fixed it recently so that it will correctly throw if you try to add a service after the IServiceProvider has been created. I think this is a good change because it points out to users when they make a mistake.

I'll once again urge the team to reconsider this strategy of using a deactivated-by-default diagnostics solution that doesn't rely at all on ILogger and doesn't integrate seamlessly with modern applications. You are really making consumer's lives that much harder this way.

This is a common ask! You can actually get the internal logs from AppService. But in other things, for example Functions, it is totally impossible. I think we will have a solution for this at some point. In the meantime you can use Macross.OpenTelemetry.Extensions if you really want to just dump the internal logging into ILogger. I made that to help some friends that were in the same boat as you: No access to the file system in prod 🤣

@alefcarlos
Copy link

Hey!

I have the same issue when I need to configure the AddOtlpExporter, for example I need to get the url from IConfiguration and then pass to otlpOptions:

        private static void ConfigureTracing(TracerProviderBuilder configure)
        {

            configure.AddHttpClientInstrumentation();
            configure.AddAspNetCoreInstrumentation();
            configure.ConfigureBuilder((sp, builder) =>
            {
                var configuration = sp.GetRequiredService<IConfiguration>();

                var exporter = configuration.GetValue<string>("OpenTelemetry:Traces:Exporter") ?? string.Empty;
                switch (exporter.ToLowerInvariant())
                {
                    case MetricsExporterResolver.Otlp:
                        // Adding the OtlpExporter creates a GrpcChannel.
                        // This switch must be set before creating a GrpcChannel/HttpClient when calling an insecure gRPC service.
                        // See: https://docs.microsoft.com/aspnet/core/grpc/troubleshoot#call-insecure-grpc-services-with-net-core-client
                        AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true);

                        builder.AddOtlpExporter(otlpOptions =>
                        {
                            otlpOptions.Endpoint = new Uri(configuration.GetValue<string>("OpenTelemetry:CollectorEndpoint"));
                        });
                        break;

                    default:
                        break;
                }
            });
        }

I saw in the examples retriving from WebApplicationBuilder.Configuration but this is not my case. Is there any workaround?

@julealgon
Copy link

The OpenTelemetry Specification is super opinionated that SDKs should never crash the process once it is up and running. But I think in this case it is OK to throw because the process hasn't exactly started yet.

I'm aware that the OTEL spec is very sensitive in this regard, but as you said, this type of exception is different: it is not an OTEL/collection issue, it is a preliminary setup problem that is part of the library and not of OTEL itself.

Agree with this! It used to silently fail and I fixed it recently so that it will correctly throw if you try to add a service after the IServiceProvider has been created. I think this is a good change because it points out to users when they make a mistake.

Agreed with having the exception always happen here. This would've allowed us to catch the problem in our case months earlier than we did.

This is a common ask! You can actually get the internal logs from AppService. But in other things, for example Functions, it is totally impossible. I think we will have a solution for this at some point.

Definitely looking forward to that.

In the meantime you can use Macross.OpenTelemetry.Extensions if you really want to just dump the internal logging into ILogger. I made that to help some friends that were in the same boat as you: No access to the file system in prod 🤣

This is absolutely fantastic! Thank you so much for sharing it. I'll for sure be using this in all of our services until official ILogger support is added.

@julealgon
Copy link

for example I need to get the url from IConfiguration and then pass to otlpOptions:

@alefcarlos OTEL already defines a well-known configuration setting that you can use to change the URL, OTEL_EXPORTER_OTLP_ENDPOINT. I'd strongly recommend relying on that instead of creating a separate configuration setting for it.

As for how to conditionally add the exporter based on configuration, you might have to pass the IConfiguration instead of attempting to resolve it from the container (which is what I ended up doing with IHostEnvironment as well).

@alefcarlos
Copy link

@julealgon, thank you !

But that used to work using 1.3.0 version :(

As for how to conditionally add the exporter based on configuration, you might have to pass the IConfiguration instead of attempting to resolve it from the container (which is what I ended up doing with IHostEnvironment as well).

Thats the exactly behaviour I have.

To solve that I changed my extension, now it uses WebApplicationBuilder

public static WebApplicationBuilder AddOpenTelemetryCommon(this WebApplicationBuilder appBuilder, string appName)
        {
            void ConfigureResource(ResourceBuilder r) => r.AddService(
                serviceName: appName,
                serviceVersion: Assembly.GetExecutingAssembly().GetName().Version?.ToString() ?? "unknown",
                serviceInstanceId: Environment.MachineName);

            void ConfigureTracing(TracerProviderBuilder configure)
            {
                var exporter = appBuilder.Configuration.GetValue<string>("OpenTelemetry:Traces:Exporter") ?? string.Empty;
                switch (exporter.ToLowerInvariant())
                {
                    case MetricsExporterResolver.Otlp:
                        // Adding the OtlpExporter creates a GrpcChannel.
                        // This switch must be set before creating a GrpcChannel/HttpClient when calling an insecure gRPC service.
                        // See: https://docs.microsoft.com/aspnet/core/grpc/troubleshoot#call-insecure-grpc-services-with-net-core-client
                        AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true);

                        configure.AddOtlpExporter(otlpOptions =>
                        {
                            otlpOptions.Endpoint = new Uri(appBuilder.Configuration.GetValue<string>("OpenTelemetry:CollectorEndpoint"));
                        });
                        break;

                    default:
                        break;
                }

                configure.AddHttpClientInstrumentation();
            }

            void ConfigureMetrics(MeterProviderBuilder configure)
            {
                var exporter = new MetricsExporterResolver(appBuilder.Configuration);

                configure.ConfigureServices(services =>
                {
                    services.AddSingleton(exporter);
                    services.AddTransient<IStartupFilter, OpenTelemetryPrometheusStartupFilter>();
                });

                switch (exporter.Value.ToLowerInvariant())
                {
                    case MetricsExporterResolver.Prometheus:
                        configure.AddPrometheusExporter();
                        break;

                    case MetricsExporterResolver.Otlp:
                        // Adding the OtlpExporter creates a GrpcChannel.
                        // This switch must be set before creating a GrpcChannel/HttpClient when calling an insecure gRPC service.
                        // See: https://docs.microsoft.com/aspnet/core/grpc/troubleshoot#call-insecure-grpc-services-with-net-core-client
                        AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true);

                        configure.AddOtlpExporter(otlpOptions =>
                        {
                            otlpOptions.Endpoint = new Uri(appBuilder.Configuration.GetValue<string>("OpenTelemetry:CollectorEndpoint"));
                        });
                        break;

                    default:
                        break;
                }

            }

            appBuilder.Services.AddOpenTelemetry()
                .ConfigureResource(ConfigureResource)
                .WithTracing(ConfigureTracing)
                .WithMetrics(ConfigureMetrics)
                .StartWithHost();

            return appBuilder;
        }

@CodeBlanch
Copy link
Member

@alefcarlos Thanks for sharing!

But that used to work using 1.3.0 version :(

Agree with this. The AddOtlpExporter extension changed a lot from 1.3.0 to 1.4.0. It now requires the IServiceCollection to do its work. Couple of reasons for this. 1) Tighter integration with Options API. If you pass a delegate to configure the options it should play nice with other actions that are registered (eg using services.Configure<OtlpExporterOptions>). To do that it has to register the delegate into services. 2) Tighter integration with Configuration API. Previously we loaded environment variables through the static class. Now we load them through IConfiguration. That also requires the IServiceCollection.

So this is a breaking behavioral change, but I think the new features/tighter integrations are worth it. I don't think most users will run into this.

The code above extending WebApplicationBuilder looks good to me. It is very close to what our AspNetCore example does. For dynamically choosing an exporter, this is a good solution.

You could get your original code up and running by avoiding the AddOtlpExporter extension and using builder.AddProcessor method with the public ctor exposed on the exporter classes... but I'm not going to provide that code because I think the above is a better solution.

@julealgon
Copy link

The AddOtlpExporter extension changed a lot from 1.3.0 to 1.4.0.

@CodeBlanch is there any particular reason why the team decided not to follow proper semantic versioning for these libraries? Minor version bumps shouldn't contain breaking API changes ideally, only deprecations at most.

Would certainly be nice if you were following semantic versioning as that gives more information about actual breaking changes to consumers by just looking at the version numbers.

@CodeBlanch
Copy link
Member

@julealgon I hear you. My thoughts on this...

Define breaking API change for me 😄

  • There shouldn't be any source breaks. You should be able to upgrade and compile. A lot of time went into making that possible! If you ran into one of these please reply or open an issue ASAP.

  • There shouldn't be any binary breaks either, meaning you can drop in a newer version at runtime, but there is one that I know about. We should probably fix that.

  • Behavior breaks, the goal is to avoid them, but you know the saying about omelets and broken eggs right? 😄 If we were to do a 2.0 we would have made a lot bigger changes which would have been very disruptive and might have caused people to stick on 1.3 (which would be bad for the growth of the ecosystem). A few breaking behavioral things so long as the impact is low is a lesser evil IMO. ConfigureBuilder is a really advanced API I expect only advanced users to be messing with so while these changes are annoying, I think the good outweighs the bad.

All that being said I'm sure I'm going to get flamed on this issue hard once 1.4 comes out 🤣

@maciejw
Copy link

maciejw commented Jan 20, 2023

Hello everyone.

It looks like this library has its own way of doing things, does anyone read aspnetcore source code or documentation here? because this library ignores all widely know conventions from dotnet ecosystem.

A summary:

  1. ConfigureServices should not start anything, this is only for DI configuration, registering ServiceDescriptors
  2. So we dont want to use builders that start listening on a socket during ConfigureServices, because its a configuration for DI to make composition root work. Aspnet will compose a graph and will give us an instance of what we ask later.
  3. Builders are used to configure instance of options and then register something in DI, builders are not the only way we can configure an app.
  4. Developers should have a choice, use a builder for static configuration in code, this works nice on presentations and in demo code, or register services through IServiceConfigration extension methods plus use IConfigureOptions for more advanced stuff
  5. If developers want to configure instance of your options later they can use IConfigureOptions<T> (using this we can inject stuff from DI, and configure something from IConfiguration)
  6. We do not want conditional registrations in DI, if we have to do this, you we use a factory that can do something conditionally, because we can inject option into it.
  7. We do all of this in order NOT to have untestable 8000 lines in ConfigureServices when we have an app that does something more then WeatherForecast from dotnet new webapi
  8. Classes that do something can be injected/resolved in Startup.Configure(ApplicationBuilder) method, IStartupFilter or IHostingService

If we forget about points I've mentioned we end up with problems described here.
This method from the title is a manifestation of how not to do stuff, but this is only the tip of the iceberg. The way configuration API of this library works is an abomination.

I know that we cannot change current API in a drastic way for now, but I hope we can do better in version 2.0, how can I help? together we could build something truly amazing :)

@CodeBlanch
Copy link
Member

Hey @maciejw I can't think of anywhere we are starting anything in a ConfigureServices flow can you share some code which reproduces what you are seeing?

@julealgon
Copy link

It looks like this library has its own way of doing things, does anyone read aspnetcore source code or documentation here? because this library ignores all widely know conventions from dotnet ecosystem.

A summary:

  1. ConfigureServices should not start anything, this is only for DI configuration, registering ServiceDescriptors

@maciejw , adding HostedServices to the ServicesCollection automatically starts them up when the application is run. This is how the hosting extensions work by definition.

What this library does is exactly that: it adds a hosted service that listens to the telemetry and then exports them.

  1. So we dont want to use builders that start listening on a socket during ConfigureServices, because its a configuration for DI to make composition root work. Aspnet will compose a graph and will give us an instance of what we ask later.

Again, it relies on a hosted service. You are just misunderstanding how it works fundamentally.

  1. Builders are used to configure instance of options and then register something in DI, builders are not the only way we can configure an app.

I don't quite follow this one. Can you clarify?

  1. Developers should have a choice, use a builder for static configuration in code, this works nice on presentations and in demo code, or register services through IServiceConfigration extension methods plus use IConfigureOptions for more advanced stuff

This should work just fine though. Did you run into any issues trying to move configuration into IConfigureOptions?

  1. If developers want to configure instance of your options later they can use IConfigureOptions<T> (using this we can inject stuff from DI, and configure something from IConfiguration)

My understanding is that this works as well.

  1. We do not want conditional registrations in DI, if we have to do this, you we use a factory that can do something conditionally, because we can inject option into it.

What conditional registration are you alluding to? In general I agree, but I'm not sure what you are referring to here in particular.

  1. We do all of this in order NOT to have untestable 8000 lines in ConfigureServices when we have an app that does something more then WeatherForecast from dotnet new webapi

This looks like an exaggeration... what aspects do you think would need to be testable exactly?

  1. Classes that do something can be injected/resolved in Startup.Configure(ApplicationBuilder) method, IStartupFilter or IHostingService

See first points. This relies on hosted services, the same way AspNetCore itself does. Similarly for AspNetCore, you don't "see" the hosted service being registered anywhere but it is done behind the scenes by one of the extensions or builders.

together we could build something truly amazing :)

🤔

@cijothomas
Copy link
Member

I'll once again urge the team to reconsider this strategy of using a deactivated-by-default diagnostics solution that doesn't rely at all on ILogger and doesn't integrate seamlessly with modern applications. You are really making consumer's lives that much harder this way

Linking to:
#3881

Copy link
Contributor

This issue was marked stale due to lack of activity and will be closed in 7 days. Commenting will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 25, 2024
Copy link
Contributor

github-actions bot commented Oct 3, 2024

Closed as inactive. Feel free to reopen if this issue is still a concern.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

No branches or pull requests

6 participants