Support IConfiguration for configuration settings

[tillig]

Feature Request

Allow the OpenTelemetry environment variables/settings to also be provided by IConfiguration instead of directly from the environment.

Is your feature request related to a problem?

The OpenTelemetry spec has a set of conventional environment variables to configure various aspects of the system. The current implementation of the libraries here read those values directly from the environment.

It's a common pattern in .NET Core to include environment variables in IConfiguration. This allows for merging some default values from JSON with environment settings to make a complete set of values. Being able to provide this merged configuration to the systems that read settings rather than having them read directly from environment would allow for a wider variety of configuration opportunities.

Further, it's much easier to set up tests (e.g., ASP.NET TestHost integration tests) if you don't have to change environment variables.

Describe the solution you'd like:

I'd like to be able to pass IConfiguration into anywhere configuration values are read from the environment. For example, in the JaegerConfigurationOptions class the constructor reads directly from the environment to set things up. Having a constructor that takes IConfiguration would be helpful and would allow for shared logic to read the keys. Something like...

public JaegerExporterOptions()
{
  var envConfig = new ConfigurationBuilder().AddEnvironmentVariables().Build();
  this.InitializeFromConfiguration(envConfig);
}

public JaegerExporterOptions(IConfiguration configuration)
{
  this.InitializeFromConfiguration(configuration);
}

private void InitializeFromConfiguration(IConfiguration configuration)
{
  // Common config handling for either IConfiguration OR environment variables.
  if (ConfigurationHelper.LoadString(configuration, OTelAgentHostEnvVarKey, out string agentHostEnvVar))
  {
    this.AgentHost = agentHostEnvVar;
  }
}

Describe alternatives you've considered.

The only alternative is to build all the duplicate parsing logic myself since our systems hold things like "service name" in JSON config rather than environment variables.

[cijothomas]

https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Extensions.Hosting I think this is covered when using OTel.Extensions.Hosting package. (JaegerExporter currently support this, but not really documented in its readme file).

Example: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/examples/AspNetCore/Startup.cs#L73

Please see if Extensions.Hosting package addresses this. (Note that this package is not yet stable.)

[tillig]

Unfortunately, it doesn't seem that does what I'm hoping. It does look like it nicely can attach things to the hosting environment, but it doesn't connect anything to IConfiguration. It would actually need to duplicate all the hardcoded environment reading in every location and point to IConfiguration instead.

A good way to find out if it can be connected to IConfiguration is to look for uses of EnvironmentVariableHelper, which directly reads from the environment only.

[tillig]

I do see the calls to services.Configure<T>(config) and that's not quite the same. It implies that the configuration is in the same object format as the thing being configured, like:

{
  "Jaeger": {
    "ServiceName": "jaeger-test",
    "AgentHost": "localhost",
    "AgentPort": 6831,
    "Endpoint": "http://localhost:14268",
    "Protocol": "UdpCompactThrift"
  }
}

Sort of like a JSON serialized version of the object.

However, that doesn't merge in what might be specified in the environment.

Imagine appsettings.json like:

{
  "OTEL_EXPORTER_JAEGER_AGENT_HOST": "localhost"
}

Then appsettings.Production.json might be:

{
  "OTEL_EXPORTER_JAEGER_AGENT_HOST": "production-jaeger"
}

Finally, at deployment, a common set of environment variables may be added to all pods going to a given cluster.

OTEL_EXPORTER_JAEGER_AGENT_HOST = cluster-jaeger

It may be that the JSON files aren't included so it's only that one override environment variable. It may be that the environment variable isn't provided but only the JSON files.

The scenario is that they all use the same, standardized variable names (for environment and for config) so:

• You can use a common deployment template with environment overrides for all services in all languages should you need to override.
• You don't have to tie your config to an object model.

[codeaphex]

Maybe I'm not getting your specific question but I'm relying on the WebApplicationBuilder to read the configs from the usual providers:

builder.Services.AddOpenTelemetryTracing(traceProvider =>
            {
                traceProvider
                    .AddSource(OpenTelemetryExtensions.ServiceName)
                    .SetResourceBuilder(
                        ResourceBuilder.CreateDefault()
                            .AddService(serviceName: OpenTelemetryExtensions.ServiceName, serviceVersion: OpenTelemetryExtensions.ServiceVersion))
                    .AddHttpClientInstrumentation()
                    .AddAspNetCoreInstrumentation()
                    .AddJaegerExporter(exporter =>
                    {
                        exporter.AgentHost = builder.Configuration["JaegerConfiguration:AgentHost"];
                        exporter.AgentPort = Convert.ToInt32(builder.Configuration["JaegerConfiguration:AgentPort"]);
                    });
            });```

[tillig]

@codeaphex That part where you're manually reading info and passing it to the Jaeger exporter? There is already code to read that directly from the environment. Since config can include environment variables, it seems reasonable to allow those settings to come from config, not just environment. If that was done, you wouldn't need to do that manual wire-up at all; it'd happen for you, just like it would right now if you used environment variables explicitly.

[CodeBlanch]

@cijothomas This really doesn't have anything to do with the hosting library.

You can bind IConfiguration to exporter options today like this...

builder.Services.Configure<JaegerExporterOptions>(builder.Configuration.GetSection("Jaeger"));
builder.Services.Configure<OtlpExporterOptions>(builder.Configuration.GetSection("Otlp"));
builder.Services.Configure<ZipkinExporterOptions>(builder.Configuration.GetSection("Zipkin"));

There are a bunch of ways to do it. Essentially all the SDK exporters will ask for their options class instance through the Options API and that's where the integration happens allowing this to work.

The issue is how that binding happens.

If you set an environment variable like this JAEGER__AGENTHOST and you used the config above, everything would work fine. The "Jaeger" section is mapped to JaegerExporterOptions and "AGENTHOST" will match the property.

The problem is the OTel standard key "OTEL_EXPORTER_JAEGER_AGENT_HOST" will map somewhere very specific using the .NET Options/Configuration pathing structure and where it maps probably doesn't match the user's binding.

Essentially the issue is the OTel spec environment variable keys are not friendly to work well with .NET IConfiguration.

@tillig Am I understanding this correctly?

[tillig]

@CodeBlanch

Essentially the issue is the OTel spec environment variable keys are not friendly to work well with .NET IConfiguration.

Am I understanding this correctly?

Well, sort of. It's almost the inverse of that.

• I want to use the standard OTel environment variables.
• I want to get the values from IConfiguration and not just the environment.
• I do not want to use a .NET-specific object model to bind configuration to.

The OTel standard environment variable works just fine with configuration. It does not get mapped weird because it doesn't have double-underscores anywhere. I put an example earlier showing how configuration could look.

I'll put it here again for clarity:

I want to be able to have a default in appsettings.json so the dev environment "just works" for folks.

{
  "OTEL_EXPORTER_JAEGER_AGENT_HOST": "localhost"
}

I want to be able to override that default for my production environment, so appsettings.Production.json would have that change. This is still not an environment variable, but it's the standard OTel key.

{
  "OTEL_EXPORTER_JAEGER_AGENT_HOST": "production-jaeger"
}

There may be a need in a deployment to override that. That's how configuration hierarchy works! Instead of using a .NET specific override, I can be language-agnostic by inserting an environment variable in the Kubernetes pod definition on a global level as part of my CI/CD pipeline. I won't have to know it's a .NET container being deployed.

OTEL_EXPORTER_JAEGER_AGENT_HOST = cluster-jaeger

.NET IConfiguration defaults have this fallback already built in:

• appsettings.json
• appsettings.EnvironmentName.json
• Environment Variables

The real key is I don't want the .NET specific stuff leaking out to the environment variables. Being language agnostic and using the standard keys helps unify things across polyglot systems.

I also don't want folks confused where ".NET configuration uses one set of keys but the environment variable override is totally different." There's already a standard, I want to use the standard - in configuration and in the environment.

Finally, from a testability standpoint, it's pretty hard to set up unit tests for things that are glued to the environment. I see some of the hoops that get jumped through to make that happen and it's more like integration tests than unit tests. With IConfiguration as the lowest common denominator, it's easy to set up tests:

var data = new Dictionary<string, string>
{
  { "OTEL_EXPORTER_JAEGER_AGENT_HOST", "value" },
};
var config = new ConfigurationBuilder().AddInMemoryCollection(data).Build();

No set/update/reset of variables, no need to ensure process isolation due to environment corruption, etc.

And, again, the default fallback already includes/supports environment, so even if folks don't have JSON files, the IConfiguration could still just be environment-based. For the folks that provide config - great. For the folks who don't, you can always internally do something like:

var config = new ConfigurationBuilder.AddEnvironmentVariables().Build();

Then unify on reading environment from config rather than literally directly from the hardcoded environment. It'd also give the added benefit, internally, of not having to manually support things like LoadNumeric because the config system already has Get<int>() and similar conversion/parsing built in.

[CodeBlanch]

@tillig

• I want to use the standard OTel environment variables.
• I want to get the values from IConfiguration and not just the environment.

Couldn't you just use different environment variables that play nice with your particular configuration layout? For example, instead of OTEL_EXPORTER_JAEGER_AGENT_HOST wouldn't something like JAEGER__AGENTHOST give you what you need?

[tillig]

@CodeBlanch I could, but that breaks this:

The real key is I don't want the .NET specific stuff leaking out to the environment variables. Being language agnostic and using the standard keys helps unify things across polyglot systems.

We have a standard deployment template that works across things that aren't .NET. I don't want .NET specific stuff leaking out when there is already a documented standard.

[CodeBlanch]

@tillig

We have a standard deployment template that works across things that aren't .NET

Would having some duplicates in there that are .NET specific cause harm to the other platforms?

[tillig]

I'm more worried about confusion and precedent.

"Why is it configured twice? I'm a Java person, let me PR this unnecessary duplicate out of there."

"These two values can diverge. Which takes precedence? On which platforms?"

"We need to update this value. I'm not a .NET person and only put a PR in to update the one that every other platform uses. This other non-.NET dev approved it. Uh oh, diverging values!"

"Hey, the .NET folks get their own language specific variables jammed in here, let's all put our own stuff in there!"

Think in terms of hundreds of devs in an org, not all of them super-senior folks. I'm trying to make things as standard and "pit of success" as possible. If it was two or three services, or if I could trust folks to be more diligent in these areas, I'd probably care a lot less. Removing duplicates, having standard options/places to set things, using a standard I can point out in the OTel docs and having that "just work" with .NET config and not be different/special - that's the big value.

[CodeBlanch]

I'm more worried about confusion and precedent.

For sure any kind of shared-single-template-to-rule-them-all setup is going to suffer these kind of human issues. But are there any technical blockers? Just trying to prioritize this and understand the need.

Here's another workaround I messed with this morning...

// In startup
builder.Services.ConfigureOptions<ConfigureJaegerExporterOptions>();

// Somewhere
internal sealed class ConfigureJaegerExporterOptions : IConfigureOptions<JaegerExporterOptions>
{
    private readonly IConfiguration configuration;

    public ConfigureJaegerExporterOptions(IConfiguration configuration)
    {
        this.configuration = configuration;
    }

    public void Configure(JaegerExporterOptions options)
    {
        options.AgentHost = this.configuration.GetValue<string>("OTEL_EXPORTER_JAEGER_AGENT_HOST");

        // TODO: Map other standard OTEL envvar keys onto options instance
    }
}

What that workaround does is just apply the IConfiguration keys for the standard OTel stuff onto the options. That will run after the ctor (where envvars are parsed) so it gives you a chance to use the "override" behavior of the options API.

Could OTel .NET take these environment variables from IConfiguration automatically?

The challenge here is how does that work for .NET Framework? Is it the common case that .NET Framework users have IConfiguration hooked up or not? In my personal experience I haven't seen it hooked up ever. So we need a solution that works in both cases and doesn't force IConfiguation usage. I'm thinking there might be a way to make it work once the exporters can see the service collection (#3533) which would allow them to register something like an IConfigureOptions in order to participate in the options pipeline the way it was designed. I'm going to come back to this after that is available!

[tillig]

Are there technical blockers? No, I have extension methods and configurators that already basically do what your workaround/experimentation is doing:

/// <summary>
/// Environment variable keys defined by the OpenTelemetry specification:
/// <c>https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md</c>.
/// </summary>
public static class OpenTelemetryEnvironmentVariable
{
    /// <summary>
    /// Specifies the Jaeger transport protocol (<c>http/thrift.binary</c>, <c>grpc</c>, <c>udp/thrift.compact</c>, <c>udp/thrift.binary</c>).
    /// </summary>
    public const string JaegerExporterProtocol = "OTEL_EXPORTER_JAEGER_PROTOCOL";

    /// <summary>
    /// Hostname of the Jaeger agent when using UDP (<c>udp/thrift.compact</c> or <c>udp/thrift.binary</c>) transports.
    /// </summary>
    public const string JaegerExporterAgentHost = "OTEL_EXPORTER_JAEGER_AGENT_HOST";

    /// <summary>
    /// Port of the Jaeger agent when using UDP (<c>udp/thrift.compact</c> or <c>udp/thrift.binary</c>) transports.
    /// </summary>
    public const string JaegerExporterAgentPort = "OTEL_EXPORTER_JAEGER_AGENT_PORT";

    /// <summary>
    /// Full URL of the Jaeger HTTP or gRPC endpoint.
    /// </summary>
    public const string JaegerExporterEndpoint = "OTEL_EXPORTER_JAEGER_ENDPOINT";
}

/// <summary>
/// Sets up <see cref="JaegerExporterOptions"/> based on environment variables
/// or configuration keys defined by the OpenTelemetry SDK:
/// <c>https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md</c>.
/// </summary>
/// <seealso cref="OpenTelemetryEnvironmentVariable"/>
public class JaegerExporterConfigurator : IConfigureOptions<JaegerExporterOptions>
{
    private readonly IConfiguration _configuration;

    /// <summary>
    /// Initializes a new instance of the <see cref="JaegerExporterConfigurator"/> class.
    /// </summary>
    /// <param name="configuration">
    /// The <see cref="IConfiguration"/> with the environment variable values
    /// for the Jaeger exporter.
    /// </param>
    public JaegerExporterConfigurator(IConfiguration configuration)
    {
        this._configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
    }

    /// <inheritdoc/>
    public void Configure(JaegerExporterOptions options)
    {
        ArgumentNullException.ThrowIfNull(options);

        if (this._configuration.TryGetValue<string>(OpenTelemetryEnvironmentVariable.JaegerExporterProtocol, out var protocol))
        {
            options.Protocol = JaegerExportProtocolMap.Map(protocol);
        }

        if (this._configuration.TryGetValue<string>(OpenTelemetryEnvironmentVariable.JaegerExporterAgentHost, out var agentHost))
        {
            options.AgentHost = agentHost;
        }

        if (this._configuration.TryGetValue<int>(OpenTelemetryEnvironmentVariable.JaegerExporterAgentPort, out var agentPort))
        {
            options.AgentPort = agentPort;
        }

        if (this._configuration.TryGetValue<Uri>(OpenTelemetryEnvironmentVariable.JaegerExporterEndpoint, out var endpoint))
        {
            if (endpoint == null || !endpoint.IsAbsoluteUri)
            {
                throw new InvalidOperationException($"Jaeger endpoint must be expressed as an absolute URI. Value '{endpoint}' is not absolute.");
            }

            options.Endpoint = endpoint;
        }
    }
}

The challenges I ran into were:

• There's no existing constants class with the OTel variables. That isn't awesome, given the values are in here somewhere but aren't consumable.
• The JaegerExporterOptions constructor goes looking in the environment up front which isn't the end of the world, but it means I can't explicitly exclude the environment during testing situations.

For the .NET Framework folks who don't necessarily have IConfiguration, that's OK - you can still use that internally:

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

  public JaegerExporterOptions(IConfiguration config)
  {
    this.Protocol = config["OTEL_EXPORTER_JAEGER_PROTOCOL"].Get<string>();
  }
}

Obviously I've simplified some error checking there but the concept is that Microsoft.Extensions.Configuration does support .NET 4.6.2+ so it's an option for any reasonably supported .NET desktop framework. There's no need to hardcode right to the environment.

(I also haven't tried this from a usage perspective. It may be that as a constructor parameter it's not the best; but I also wonder if having the constructor do the initialization directly rather than using a factory method or something is all that great. JaegerExporterOptions.FromEnvironment() maybe?)

A reasonable first step that would help is to have publicly accessible constants classes for environment variable names defined by the OTel spec. They're used internally, they're documented part of the spec, it doesn't seem terribly unreasonable to have them as part of the public API.

I think the IConfigureOptions<T> is also a good solution. Showing that in the Jaeger (and other) examples instead of the Configure<T>(config["section"]) option would get people using the standard API.

Finally, I think removing the hard tie to the environment in constructors would be good. If there needs to be a way to get stuff from the environment "by default," a static factory method might be a better way to go.

public static JaegerOptions FromEnvironment()
{
  var config = new ConfigurationBuilder().AddEnvironmentVariables().Build();
  return FromConfiguration(config);
}

public static JaegerOptions FromConfiguration(IConfiguration config)
{
  var options = new JaegerOptions();
  var configurator = new ConfigureJaegerExporterOptions(config);
  configurator.Configure(options);
  return options;
}

Then people could pick from var options = JaegerOptions.FromEnvironment() or var options = JaegerOptions.FromConfiguration(myConfig) or, if they're doing the whole thing with DI and the options framework, use the IConfigureOptions<JaegerOptions> mechanism.

It doesn't tie anyone to the environment, it doesn't use custom .NET config settings, it still all works for .NET 4.6.2+, it doesn't require anyone actually be using IConfiguration at all.

[CodeBlanch]

(I also haven't tried this from a usage perspective. It may be that as a constructor parameter it's not the best; but I also wonder if having the constructor do the initialization directly rather than using a factory method or something is all that great. JaegerExporterOptions.FromEnvironment() maybe?)

None of that works sadly 😢 Options (meaning the Options API) is very strict and requires a public parameter-less constructor which is invoked internally by the pipeline. You can take over the factory used internally, but I think there are easier ways to make this work. I'll try after #3533 and report back what I find!

[tillig]

This works perfectly as of 1.4.0-beta.3. Thanks!