open-telemetry / opentelemetry-dotnet

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

Exception is thrown on non-standard IOC container due to use of Concrete classes. #5537

Closed PeterOscarsson closed 2 weeks ago

PeterOscarsson commented 5 months ago

OpenTelemetry.Exporter 1.8 net8.0

Upgrading from 1.7 to 1.8 without any changes to our codebase.

This code is throwing exception:

 public static void EnsureNoUseOtlpExporterRegistrations(this IServiceProvider serviceProvider)
 {
     var registrations = serviceProvider.GetServices<UseOtlpExporterRegistration>();
     if (registrations.Any())
     {
         throw new NotSupportedException("Signal-specific AddOtlpExporter methods and the cross-cutting UseOtlpExporter method being invoked on the same IServiceCollection is not supported.");
     }
 }

My guess is that as we are using a non-default ServiceProvider (IUnityContainer), we get a concrete class even though it is not registred in the service collection. Is it possible to refactor to use interfaces instead of concrete classes?

This is the only instance in all of the codebase we are using (asp.net mvc+ blazor, redis cache, Masstransit, ..) where this is a problem.

CodeBlanch commented 4 months ago

Hey @PeterOscarsson any chance you could give me a small repro app using Unity + 1.8 OtlpExporter I can use to debug?

PeterOscarsson commented 4 months ago

Sure, put the attached project in the OpenTelemetry catalog and put a breakpoint on EnsureNoUseOtlpExporterRegistrations in opentelemetry-dotnet\src\OpenTelemetry.Exporter.OpenTelemetryProtocol\Builder\OpenTelemetryBuilderServiceProviderExtensions.cs

Check with and without "UseUnityServiceProvider" in main Program.

OTelTest.zip

CodeBlanch commented 4 months ago

Thank you @PeterOscarsson! I opened this issue over in Unity: https://github.com/unitycontainer/microsoft-dependency-injection/issues/96

I'm not opposed to just switching to an interface but I figured it would be good to gather their thoughts on the differing behavior before we took action. Seems like a behavior they might want to fix to prevent other things from breaking when choosing to use Unity over the M.S.E.DI container 🤷

PeterOscarsson commented 4 months ago

@CodeBlanch ,Thats great, but I doubt you'll get an answer. Seems like its no longer activly maintained. Else I would have taken this up with them. We are investigating switching DI but we have a huge CodeBase that depends on UnityContainer and it will take a while to do that.

mechelewskim commented 3 months ago

Hey, we're affacted by this issue as well. We'll appreciate if you could use interface for this feature instead of concrete class to make it work with Unity. Thanks!

PeterOscarsson commented 3 months ago

@CodeBlanch , an update for Unity, is that there will be no answer from the maintainer as he passed away last christmas. :-(

Our company are currently investigating if we should switch DI container(not found a good enough yet) or fork the Unity one and continue using it. In the meantime we would be grateful for a fix to this "problem".