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.

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 😃

🤔

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 😃

@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 🤣

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

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.

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: https://github.com/open-telemetry/opentelemetry-dotnet/issues/3881

@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;
        }

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

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.

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