PrometheusExporterOptions does not follow Options<T> pattern.

[tillig]

Bug Report

List of all OpenTelemetry NuGet packages and version that you are using (e.g. OpenTelemetry 1.0.2):

• OpenTelemetry 1.2.0-rc2
• OpenTelemetry.Exporter.Prometheus 1.2.0-rc2

Runtime version (e.g. net461, net48, netcoreapp3.1, net5.0 etc. You can find this information from the *.csproj file):

• net6.0

Symptom

The configuration of the Prometheus exporter with the MeterProviderBuilder appears to follow the Options<T> pattern where you can provide configuration for the PrometheusExporterOptions and later have those retrieved from the IServiceProvider, but the configuration action is not actually registered with the container or able to be reused. Further, nothing ever actually calls AddOptions<PrometheusExporterOptions> as part of the base dependency registration, so every time sp.GetOptions<PrometheusExporterOptions>() is executed here - in examples and in tests - it's always going to be a new instance of PrometheusExporterOptions. This is really hard to figure out and isn't documented anywhere if that's the expected behavior.

What is the expected behavior?

I expected to see:

• Something in here calling IServiceCollection.AddOptions<PrometheusExporterOptions>() to register the default options. Ostensibly in AddPrometheusExporter, but the structure of the MeterProviderBuilder doesn't currently support registering things with the calling IServiceCollection.
• The lambda passed into AddPrometheusExporter translated into some sort of services.Configure<PrometheusExporterOptions> call so it can be reused/resolved from the container.
• The UseOpenTelemetryScrapingEndpoint method able to use the configuration from AddPrometheusExporter.

What is the actual behavior?

• Nothing calls AddOptions<PrometheusExporterOptions>() so every instance will always be a new instance internal to OpenTelemetry.
• The lambda provided to AddPrometheusExporter only configures the exporter and is not used by UseOpenTelemetryScrapingEndpoint.
• If you resolve the IOptions<PrometheusExporterOptions> to try to get the options that have been configured, it's null.
• No documentation or examples showing use of AddOptions<PrometheusExporterOptions>() or encouraging use of that plus Configure<PrometheusExporterOptions>() instead of passing that lambda to AddPrometheusExporter.

Reproduce

Here are some unit tests showing the problem.

[Fact]
public void NoOptionsRegistered()
{
    var sp = new ServiceCollection()
        .AddOpenTelemetryMetrics(b =>
        {
            b.AddPrometheusExporter(opt =>
            {
                opt.StartHttpListener = false;
                opt.ScrapeEndpointPath = "/test-metrics";
            });
        })
        .BuildServiceProvider();
    var options = sp.GetService<IOptions<PrometheusExporterOptions>>();

    // Fails: options isn't in the collection.
    Assert.NotNull(options);
}

[Fact]
public void LambdaNotReused()
{
    var coll = new ServiceCollection();
    coll.AddOptions<PrometheusExporterOptions>();
    coll.AddOpenTelemetryMetrics(b =>
        {
            b.AddPrometheusExporter(opt =>
            {
                opt.StartHttpListener = false;
                opt.ScrapeEndpointPath = "/test-metrics";
            });
        });
    var sp = coll.BuildServiceProvider();
    var options = sp.GetService<IOptions<PrometheusExporterOptions>>().Value;

    // Fails: the fluent configuration isn't tied to the registered options,
    // which means when PrometheusExporterMiddleware does the resolve,
    // it _also_ doesn't have access.
    Assert.Equal("/test-metrics", options.ScrapeEndpointPath);
}

[Fact]
public void WorksButNotIntuitive()
{
    var coll = new ServiceCollection();
    coll.AddOptions<PrometheusExporterOptions>()
        .Configure(opt =>
        {
            opt.StartHttpListener = false;
            opt.ScrapeEndpointPath = "/test-metrics";
        });
    coll.AddOpenTelemetryMetrics(b =>
        {
            // DON'T PASS A CONFIG LAMBDA or it will be different
            // than what UseOpenTelemetryScrapingEndpoint uses.
            b.AddPrometheusExporter();
        });
    var sp = coll.BuildServiceProvider();
    var options = sp.GetService<IOptions<PrometheusExporterOptions>>().Value;

    // Succeeds, but not super intuitive that this is how it works.
    Assert.Equal("/test-metrics", options.ScrapeEndpointPath);
}

Additional Context

Most "builder" implementations in the core frameworks have a Services property so things attached to the builder can register things with the owning IServiceCollection - like IMvcBuilder.Services or IHealthChecksBuilder.Services. You'd have to add that to the MeterProviderBuilder to make this work.

[tillig]

I've been working on bridging this in my local solution and one way this could be addressed is with IConfigureOptions<T>.

First, the actual configuration (from IConfiguration) could be in an IConfigureOptions<T> like this one I've worked out that mirrors all the stuff for the OtlpExporterOptions:

public class OpenTelemetryExporterConfigurator : IConfigureOptions<OtlpExporterOptions>
{
    private readonly IConfiguration _configuration;

    public OpenTelemetryExporterConfigurator()
      : this(new ConfigurationBuilder().AddEnvironmentVariables().Build())
    {
    }

    public OpenTelemetryExporterConfigurator(IConfiguration configuration)
    {
        this._configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
    }

    public void Configure(OtlpExporterOptions options)
    {
        if (options == null)
        {
            throw new ArgumentNullException(nameof(options));
        }

        if (this._configuration.TryGetValue<Uri>(OpenTelemetryEnvironmentVariable.OpenTelemetryExporterEndpoint, out var endpoint))
        {
            options.Endpoint = endpoint;
        }

        if (this._configuration.TryGetValue<string>(OpenTelemetryEnvironmentVariable.OpenTelemetryExporterHeaders, out var headers))
        {
            options.Headers = headers;
        }

        if (this._configuration.TryGetValue<int>(OpenTelemetryEnvironmentVariable.OpenTelemetryExporterTimeout, out var timeout))
        {
            options.TimeoutMilliseconds = timeout;
        }

        if (this._configuration.TryGetValue<string>(OpenTelemetryEnvironmentVariable.OpenTelemetryExporterProtocol, out var protocol))
        {
            options.Protocol = protocol?.Trim() switch
            {
                "grpc" => OtlpExportProtocol.Grpc,
                "http/protobuf" => OtlpExportProtocol.HttpProtobuf,
                _ => throw new FormatException($"{OpenTelemetryEnvironmentVariable.OpenTelemetryExporterProtocol} environment variable has an invalid value: '{protocol}'"),
            };
        }
    }
}

Then you can have the extension method handle the non-deferred/direct read from config...

public static MeterProviderBuilder AddOtlpExporter(
    this MeterProviderBuilder builder,
    Action<OtlpExporterOptions, MetricReaderOptions> configureExporterAndMetricReader)
{
    Guard.ThrowIfNull(builder, nameof(builder));

    if (builder is IDeferredMeterProviderBuilder deferredMeterProviderBuilder)
    {
        return deferredMeterProviderBuilder.Configure((sp, builder) =>
        {
            AddOtlpExporter(builder, sp.GetOptions<OtlpExporterOptions>(), sp.GetOptions<MetricReaderOptions>(), null, configureExporterAndMetricReader, sp);
        });
    }

    var configurator = new OpenTelemetryExporterConfigurator();
    var options = new OtlpExporterOptions();
    configurator.Configur(options);
    return AddOtlpExporter(builder, options, new MetricReaderOptions(), null, configureExporterAndMetricReader, serviceProvider: null);
}

This would, of course, require that somewhere along the way someone has to do something like:

services.AddOptions<OtlpExporterOptions>();
services.AddTransient<IConfigureOptions<OtlpExporterOptions>, OpenTelemetryExporterConfigurator>();

Then if someone registers their own IConfiguration it'll get injected properly and config will come from there; or if they don't, the default constructor on the configurator gets used and it pulls directly from the environment like before.

The services.AddOptions<T> bit could be added automatically if the MeterProviderBuilder (and TracerProviderBuilder got a Services collection added to it like the MvcBuilder.

public static MeterProviderBuilder AddOtlpExporter(
    this MeterProviderBuilder builder,
    Action<OtlpExporterOptions, MetricReaderOptions> configureExporterAndMetricReader)
{
    Guard.ThrowIfNull(builder, nameof(builder));

    if (builder is IDeferredMeterProviderBuilder deferredMeterProviderBuilder)
    {
        // Add the options setup automatically when the exporter is added.
        builder.Services.AddOptions<OtlpExporterOptions>();
        builder.Services.AddTransient<IConfigureOptions<OtlpExporterOptions>, OpenTelemetryExporterConfigurator>();
        return deferredMeterProviderBuilder.Configure((sp, builder) =>
        {
            AddOtlpExporter(builder, sp.GetOptions<OtlpExporterOptions>(), sp.GetOptions<MetricReaderOptions>(), null, configureExporterAndMetricReader, sp);
        });
    }

    var configurator = new OpenTelemetryExporterConfigurator();
    var options = new OtlpExporterOptions();
    configurator.Configur(options);
    return AddOtlpExporter(builder, options, new MetricReaderOptions(), null, configureExporterAndMetricReader, serviceProvider: null);
}

[tillig]

It appears that IDeferredMeterProvider may be able to pretty easily be updated to have a Services property - MeterProviderBuilderHosting already has a Services property, this would just let folks hook into it. Same with TracerProviderBuilderHosting.

Unclear if there's any appetite for that. It would make it more like other builders and add some flexibility at the cost of exposing that Services property.

[CodeBlanch]

@tillig Hey I'll take a look at this as part of my work on #3533.

[CodeBlanch]

@tillig I just ran the tests above on my branch with #3780 and they all pass now! I'm going to close this issue. Feel free to do your own testing and re-open if needed.

Here are a couple explanations for why it is working now:

• The first test NoOptionsRegistered was getting null for the options because previously we didn't specifically initialize the options pipeline. This is now done here.

• The second test LambdaNotReused if you look at the 1.3.x branch we used to do this which executes the lambda directly. It doesn't tie into the options pipeline, essentially. The new version registers the lambda with options so it all ties in as expected!

[tillig]

Sounds great! Thanks! 🎉

[tillig]

Got this integrated into our local solution, verified it works perfectly. Thanks again!