From 81cc6295df631154c7e0d3d5e1336e10a4ad2c12 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 3 Oct 2022 10:13:07 -0700 Subject: [PATCH 1/7] Support retrieval of environment variables through IConfiguration in SDK. --- build/Common.props | 1 + src/OpenTelemetry/CHANGELOG.md | 4 ++ ...viderBuilderServiceCollectionExtensions.cs | 42 ++++++++++++++ .../Internal/EnvironmentVariableHelper.cs | 19 +++++++ .../Builder/MeterProviderBuilderBase.cs | 4 +- src/OpenTelemetry/OpenTelemetry.csproj | 1 + .../Builder/TracerProviderBuilderBase.cs | 4 +- .../HostingMeterExtensionTests.cs | 46 +++++++++++++++- .../HostingTracerExtensionTests.cs | 45 +++++++++++++++ .../MeterProviderBuilderExtensionsTests.cs | 55 +++++++++++++++++++ .../TracerProviderBuilderExtensionsTest.cs | 55 +++++++++++++++++++ 11 files changed, 271 insertions(+), 5 deletions(-) create mode 100644 src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs diff --git a/build/Common.props b/build/Common.props index deed26cd9ca..96e224950b9 100644 --- a/build/Common.props +++ b/build/Common.props @@ -31,6 +31,7 @@ [2.1.1,6.0) [3.3.3] [17.3.0] + [3.1.0,) [2.1.0,) [3.1.0,) [3.1.0,) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 58afdc44a34..16358254728 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +* Added support for loading environment variables from `IConfiguration` when + using `TracerProviderBuilder` or `MeterProviderBuilder` + ([#XXXX](https://github.com/open-telemetry/opentelemetry-dotnet/pull/XXXX)) + ## 1.4.0-beta.1 Released 2022-Sep-29 diff --git a/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs b/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs new file mode 100644 index 00000000000..058abfb76ac --- /dev/null +++ b/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs @@ -0,0 +1,42 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +#nullable enable + +using System.Diagnostics; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection.Extensions; + +namespace Microsoft.Extensions.DependencyInjection; + +internal static class ProviderBuilderServiceCollectionExtensions +{ + public static IServiceCollection AddOpenTelemetryProviderBuilderServices(this IServiceCollection services) + { + Debug.Assert(services != null, "services was null"); + + services.AddOptions(); + + // Note: When using a host builder IConfiguration is automatically + // registered and this registration will no-op. This only runs for + // Sdk.Create* style or when manually creating a ServiceCollection. The + // point of this registration is to make IConfiguration available in + // those cases. + services.TryAddSingleton(sp => new ConfigurationBuilder().AddEnvironmentVariables().Build()); + + return services; + } +} diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index f6ea9783590..6a745e4ad11 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -76,6 +76,25 @@ public static bool LoadNumeric(string envVarKey, out int result) return false; } + return LoadNumeric(envVarKey, value, out result); + } + + /// + /// Reads an environment variable and parses is as a + /// + /// numeric value - a non-negative decimal integer. + /// + /// The name of the environment variable. + /// The value of the environment variable. + /// The parsed value of the environment variable. + /// + /// Returns true when a non-empty value was read; otherwise, false. + /// + /// + /// Thrown when failed to parse the non-empty value. + /// + public static bool LoadNumeric(string envVarKey, string value, out int result) + { if (!int.TryParse(value, NumberStyles.None, CultureInfo.InvariantCulture, out result)) { throw new FormatException($"{envVarKey} environment variable has an invalid value: '{value}'"); diff --git a/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs b/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs index 186926a5799..80482d7b5cd 100644 --- a/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs +++ b/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs @@ -52,7 +52,7 @@ internal MeterProviderBuilderBase(IServiceCollection services) { Guard.ThrowIfNull(services); - services.AddOptions(); + services.AddOpenTelemetryProviderBuilderServices(); services.TryAddSingleton(sp => new MeterProviderSdk(sp, ownsServiceProvider: false)); this.services = services; @@ -65,7 +65,7 @@ protected MeterProviderBuilderBase() { var services = new ServiceCollection(); - services.AddOptions(); + services.AddOpenTelemetryProviderBuilderServices(); this.services = services; this.ownsServices = true; diff --git a/src/OpenTelemetry/OpenTelemetry.csproj b/src/OpenTelemetry/OpenTelemetry.csproj index 542fcc9e407..b7f6e7fa3f7 100644 --- a/src/OpenTelemetry/OpenTelemetry.csproj +++ b/src/OpenTelemetry/OpenTelemetry.csproj @@ -19,6 +19,7 @@ + diff --git a/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderBase.cs b/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderBase.cs index a747395308c..223f07e798c 100644 --- a/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderBase.cs +++ b/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderBase.cs @@ -51,7 +51,7 @@ internal TracerProviderBuilderBase(IServiceCollection services) { Guard.ThrowIfNull(services); - services.AddOptions(); + services.AddOpenTelemetryProviderBuilderServices(); services.TryAddSingleton(sp => new TracerProviderSdk(sp, ownsServiceProvider: false)); this.services = services; @@ -64,7 +64,7 @@ protected TracerProviderBuilderBase() { var services = new ServiceCollection(); - services.AddOptions(); + services.AddOpenTelemetryProviderBuilderServices(); this.services = services; this.ownsServices = true; diff --git a/test/OpenTelemetry.Extensions.Hosting.Tests/HostingMeterExtensionTests.cs b/test/OpenTelemetry.Extensions.Hosting.Tests/HostingMeterExtensionTests.cs index f4acf44b4e2..4553ae33926 100644 --- a/test/OpenTelemetry.Extensions.Hosting.Tests/HostingMeterExtensionTests.cs +++ b/test/OpenTelemetry.Extensions.Hosting.Tests/HostingMeterExtensionTests.cs @@ -15,13 +15,14 @@ // using System; +using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using OpenTelemetry.Metrics; using Xunit; -using static OpenTelemetry.Extensions.Hosting.Tests.HostingTracerExtensionTests; namespace OpenTelemetry.Extensions.Hosting.Tests { @@ -195,6 +196,49 @@ public void AddOpenTelemetryMetrics_MultipleCallsConfigureSingleProvider() Assert.Single(providers); } + [Fact] + public async Task AddOpenTelemetryMetrics_HostConfigurationHonoredTest() + { + bool configureBuilderCalled = false; + + var builder = new HostBuilder() + .ConfigureAppConfiguration(builder => + { + builder.AddInMemoryCollection(new Dictionary + { + ["TEST_KEY"] = "TEST_KEY_VALUE", + }); + }) + .ConfigureServices(services => + { + services.AddOpenTelemetryMetrics(builder => + { + builder.ConfigureBuilder((sp, builder) => + { + configureBuilderCalled = true; + + var configuration = sp.GetRequiredService(); + + var testKeyValue = configuration.GetValue("TEST_KEY", null); + + Assert.Equal("TEST_KEY_VALUE", testKeyValue); + }); + }); + }); + + var host = builder.Build(); + + Assert.False(configureBuilderCalled); + + await host.StartAsync(); + + Assert.True(configureBuilderCalled); + + await host.StopAsync(); + + host.Dispose(); + } + private static MeterProviderBuilder AddMyFeature(MeterProviderBuilder meterProviderBuilder) { meterProviderBuilder.ConfigureServices(services => services.AddSingleton()); diff --git a/test/OpenTelemetry.Extensions.Hosting.Tests/HostingTracerExtensionTests.cs b/test/OpenTelemetry.Extensions.Hosting.Tests/HostingTracerExtensionTests.cs index 131e9038139..22432e3f4a7 100644 --- a/test/OpenTelemetry.Extensions.Hosting.Tests/HostingTracerExtensionTests.cs +++ b/test/OpenTelemetry.Extensions.Hosting.Tests/HostingTracerExtensionTests.cs @@ -15,9 +15,11 @@ // using System; +using System.Collections.Generic; using System.Diagnostics; using System.Linq; using System.Threading.Tasks; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using OpenTelemetry.Trace; @@ -198,6 +200,49 @@ public void AddOpenTelemetryTracing_MultipleCallsConfigureSingleProvider() Assert.Single(providers); } + [Fact] + public async Task AddOpenTelemetryTracing_HostConfigurationHonoredTest() + { + bool configureBuilderCalled = false; + + var builder = new HostBuilder() + .ConfigureAppConfiguration(builder => + { + builder.AddInMemoryCollection(new Dictionary + { + ["TEST_KEY"] = "TEST_KEY_VALUE", + }); + }) + .ConfigureServices(services => + { + services.AddOpenTelemetryTracing(builder => + { + builder.ConfigureBuilder((sp, builder) => + { + configureBuilderCalled = true; + + var configuration = sp.GetRequiredService(); + + var testKeyValue = configuration.GetValue("TEST_KEY", null); + + Assert.Equal("TEST_KEY_VALUE", testKeyValue); + }); + }); + }); + + var host = builder.Build(); + + Assert.False(configureBuilderCalled); + + await host.StartAsync(); + + Assert.True(configureBuilderCalled); + + await host.StopAsync(); + + host.Dispose(); + } + private static TracerProviderBuilder AddMyFeature(TracerProviderBuilder tracerProviderBuilder) { tracerProviderBuilder.ConfigureServices(services => services.AddSingleton()); diff --git a/test/OpenTelemetry.Tests/Metrics/MeterProviderBuilderExtensionsTests.cs b/test/OpenTelemetry.Tests/Metrics/MeterProviderBuilderExtensionsTests.cs index b4c96f2cc7c..7691f22f349 100644 --- a/test/OpenTelemetry.Tests/Metrics/MeterProviderBuilderExtensionsTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MeterProviderBuilderExtensionsTests.cs @@ -17,6 +17,7 @@ using System; using System.Collections.Generic; using System.Linq; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; using OpenTelemetry.Resources; @@ -199,6 +200,60 @@ public void SetAndConfigureResourceTest() Assert.Contains(provider.Resource.Attributes, kvp => kvp.Key == "key2" && (string)kvp.Value == "value2"); } + [Fact] + public void ConfigureBuilderIConfigurationAvailableTest() + { + Environment.SetEnvironmentVariable("TEST_KEY", "TEST_KEY_VALUE"); + + bool configureBuilderCalled = false; + + using var provider = Sdk.CreateMeterProviderBuilder() + .ConfigureBuilder((sp, builder) => + { + var configuration = sp.GetRequiredService(); + + configureBuilderCalled = true; + + var testKeyValue = configuration.GetValue("TEST_KEY", null); + + Assert.Equal("TEST_KEY_VALUE", testKeyValue); + }) + .Build(); + + Assert.True(configureBuilderCalled); + + Environment.SetEnvironmentVariable("TEST_KEY", null); + } + + [Fact] + public void ConfigureBuilderIConfigurationModifiableTest() + { + bool configureBuilderCalled = false; + + using var provider = Sdk.CreateMeterProviderBuilder() + .ConfigureServices(services => + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { ["TEST_KEY_2"] = "TEST_KEY_2_VALUE" }) + .Build(); + + services.AddSingleton(configuration); + }) + .ConfigureBuilder((sp, builder) => + { + var configuration = sp.GetRequiredService(); + + configureBuilderCalled = true; + + var testKey2Value = configuration.GetValue("TEST_KEY_2", null); + + Assert.Equal("TEST_KEY_2_VALUE", testKey2Value); + }) + .Build(); + + Assert.True(configureBuilderCalled); + } + private static void RunBuilderServiceLifecycleTest( MeterProviderBuilder builder, Func buildFunc, diff --git a/test/OpenTelemetry.Tests/Trace/TracerProviderBuilderExtensionsTest.cs b/test/OpenTelemetry.Tests/Trace/TracerProviderBuilderExtensionsTest.cs index 383e691d358..34cc5a8e3ec 100644 --- a/test/OpenTelemetry.Tests/Trace/TracerProviderBuilderExtensionsTest.cs +++ b/test/OpenTelemetry.Tests/Trace/TracerProviderBuilderExtensionsTest.cs @@ -18,6 +18,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; using OpenTelemetry.Resources; @@ -393,6 +394,60 @@ public void AddExporterNamedOptionsTest() Assert.Equal(1, namedOptionsConfigureInvocations); } + [Fact] + public void ConfigureBuilderIConfigurationAvailableTest() + { + Environment.SetEnvironmentVariable("TEST_KEY", "TEST_KEY_VALUE"); + + bool configureBuilderCalled = false; + + using var provider = Sdk.CreateTracerProviderBuilder() + .ConfigureBuilder((sp, builder) => + { + var configuration = sp.GetRequiredService(); + + configureBuilderCalled = true; + + var testKeyValue = configuration.GetValue("TEST_KEY", null); + + Assert.Equal("TEST_KEY_VALUE", testKeyValue); + }) + .Build(); + + Assert.True(configureBuilderCalled); + + Environment.SetEnvironmentVariable("TEST_KEY", null); + } + + [Fact] + public void ConfigureBuilderIConfigurationModifiableTest() + { + bool configureBuilderCalled = false; + + using var provider = Sdk.CreateTracerProviderBuilder() + .ConfigureServices(services => + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { ["TEST_KEY_2"] = "TEST_KEY_2_VALUE" }) + .Build(); + + services.AddSingleton(configuration); + }) + .ConfigureBuilder((sp, builder) => + { + var configuration = sp.GetRequiredService(); + + configureBuilderCalled = true; + + var testKey2Value = configuration.GetValue("TEST_KEY_2", null); + + Assert.Equal("TEST_KEY_2_VALUE", testKey2Value); + }) + .Build(); + + Assert.True(configureBuilderCalled); + } + private static void RunBuilderServiceLifecycleTest( TracerProviderBuilder builder, Func buildFunc, From d4ccabf63f1cdfddb51d811fbe61fa29a3188c6d Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 3 Oct 2022 10:13:42 -0700 Subject: [PATCH 2/7] Update Jaeger to load environment variables through IConfiguration. --- .../CHANGELOG.md | 4 ++ .../JaegerExporterHelperExtensions.cs | 11 +++- .../JaegerExporterOptions.cs | 65 +++++++++++++++---- .../JaegerExporterOptionsTests.cs | 25 +++++++ 4 files changed, 91 insertions(+), 14 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md index 041b2df43eb..735a80aeeae 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md +++ b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +* Added support for loading environment variables from `IConfiguration` when + using the `AddJaegerExporter` extension + ([#XXXX](https://github.com/open-telemetry/opentelemetry-dotnet/pull/XXXX)) + ## 1.4.0-beta.1 Released 2022-Sep-29 diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs index c9da743562d..ead9212750d 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs @@ -62,10 +62,15 @@ public static TracerProviderBuilder AddJaegerExporter( name ??= Options.DefaultName; - if (configure != null) + builder.ConfigureServices(services => { - builder.ConfigureServices(services => services.Configure(name, configure)); - } + if (configure != null) + { + services.Configure(name, configure); + } + + services.AddSingleton, JaegerExporterOptions.JaegerExporterOptionsFactory>(); + }); return builder.ConfigureBuilder((sp, builder) => { diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs index b992048d338..5d537ad4d58 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs @@ -17,6 +17,8 @@ using System; using System.Diagnostics; using System.Net.Http; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Options; using OpenTelemetry.Internal; using OpenTelemetry.Trace; @@ -43,37 +45,63 @@ public class JaegerExporterOptions internal static readonly Func DefaultHttpClientFactory = () => new HttpClient(); + /// + /// Initializes a new instance of the class. + /// public JaegerExporterOptions() + : this(new ConfigurationBuilder().AddEnvironmentVariables().Build()) + { + } + + internal JaegerExporterOptions(IConfiguration configuration) { - if (EnvironmentVariableHelper.LoadString(OTelProtocolEnvVarKey, out string protocolEnvVar)) + var protocol = configuration.GetValue(OTelProtocolEnvVarKey, null); + if (!string.IsNullOrEmpty(protocol)) { - if (JaegerExporterProtocolParser.TryParse(protocolEnvVar, out var protocol)) + if (JaegerExporterProtocolParser.TryParse(protocol, out var parsedProtocol)) { - this.Protocol = protocol; + this.Protocol = parsedProtocol; } else { - throw new FormatException($"{OTelProtocolEnvVarKey} environment variable has an invalid value: '{protocolEnvVar}'"); + throw new FormatException($"{OTelProtocolEnvVarKey} environment variable has an invalid value: '{protocol}'"); } } - if (EnvironmentVariableHelper.LoadString(OTelAgentHostEnvVarKey, out string agentHostEnvVar)) + var agentHost = configuration.GetValue(OTelAgentHostEnvVarKey, null); + if (!string.IsNullOrEmpty(agentHost)) { - this.AgentHost = agentHostEnvVar; + this.AgentHost = agentHost; } - if (EnvironmentVariableHelper.LoadNumeric(OTelAgentPortEnvVarKey, out int agentPortEnvVar)) + var agentPort = configuration.GetValue(OTelAgentPortEnvVarKey, null); + if (!string.IsNullOrEmpty(agentPort)) { - this.AgentPort = agentPortEnvVar; + if (EnvironmentVariableHelper.LoadNumeric(OTelAgentPortEnvVarKey, agentPort, out int parsedAgentPort)) + { + this.AgentPort = parsedAgentPort; + } } - if (EnvironmentVariableHelper.LoadString(OTelEndpointEnvVarKey, out string endpointEnvVar) - && Uri.TryCreate(endpointEnvVar, UriKind.Absolute, out Uri endpoint)) + var endpoint = configuration.GetValue(OTelEndpointEnvVarKey, null); + if (!string.IsNullOrEmpty(endpoint)) { - this.Endpoint = endpoint; + if (Uri.TryCreate(endpoint, UriKind.Absolute, out Uri parsedEndpoint)) + { + this.Endpoint = parsedEndpoint; + } + else + { + throw new FormatException($"{OTelEndpointEnvVarKey} environment variable has an invalid value: '{endpoint}'"); + } } } + /// + /// Gets or sets the to use when + /// communicating to Jaeger. Default value: . + /// public JaegerExportProtocol Protocol { get; set; } = JaegerExportProtocol.UdpCompactThrift; /// @@ -129,5 +157,20 @@ public JaegerExporterOptions() /// /// public Func HttpClientFactory { get; set; } = DefaultHttpClientFactory; + + internal sealed class JaegerExporterOptionsFactory : IOptionsFactory + { + private readonly IConfiguration configuration; + + public JaegerExporterOptionsFactory(IConfiguration configuration) + { + Debug.Assert(configuration != null, "configuration was null"); + + this.configuration = configuration; + } + + public JaegerExporterOptions Create(string name) + => new(this.configuration); + } } } diff --git a/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs b/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs index 33143faf16f..42b0548be66 100644 --- a/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs +++ b/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs @@ -15,6 +15,8 @@ // using System; +using System.Collections.Generic; +using Microsoft.Extensions.Configuration; using Xunit; namespace OpenTelemetry.Exporter.Jaeger.Tests @@ -93,6 +95,29 @@ public void JaegerExporterOptions_EnvironmentVariableNames() Assert.Equal("OTEL_EXPORTER_JAEGER_ENDPOINT", JaegerExporterOptions.OTelEndpointEnvVarKey); } + [Fact] + public void JaegerExporterOptions_FromConfigurationTest() + { + var values = new Dictionary() + { + [JaegerExporterOptions.OTelProtocolEnvVarKey] = "http/thrift.binary", + [JaegerExporterOptions.OTelAgentHostEnvVarKey] = "jaeger-host", + [JaegerExporterOptions.OTelAgentPortEnvVarKey] = "123", + [JaegerExporterOptions.OTelEndpointEnvVarKey] = "http://custom-endpoint:12345", + }; + + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(values) + .Build(); + + var options = new JaegerExporterOptions(configuration); + + Assert.Equal("jaeger-host", options.AgentHost); + Assert.Equal(123, options.AgentPort); + Assert.Equal(JaegerExportProtocol.HttpBinaryThrift, options.Protocol); + Assert.Equal(new Uri("http://custom-endpoint:12345"), options.Endpoint); + } + private static void ClearEnvVars() { Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelProtocolEnvVarKey, null); From 1405478941dfa1f295e089db00c9f07d1773dc77 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 3 Oct 2022 10:26:51 -0700 Subject: [PATCH 3/7] Warning fix. --- .../Builder/ProviderBuilderServiceCollectionExtensions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs b/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs index 058abfb76ac..276174d5d19 100644 --- a/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs +++ b/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs @@ -37,6 +37,6 @@ public static IServiceCollection AddOpenTelemetryProviderBuilderServices(this IS // those cases. services.TryAddSingleton(sp => new ConfigurationBuilder().AddEnvironmentVariables().Build()); - return services; + return services!; } } From b20705e75f8ae209b56e912f9a62ef2393607ebc Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 3 Oct 2022 10:27:26 -0700 Subject: [PATCH 4/7] CHANGELOG patch. --- src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md | 2 +- src/OpenTelemetry/CHANGELOG.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md index 735a80aeeae..f60481649c0 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md +++ b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md @@ -4,7 +4,7 @@ * Added support for loading environment variables from `IConfiguration` when using the `AddJaegerExporter` extension - ([#XXXX](https://github.com/open-telemetry/opentelemetry-dotnet/pull/XXXX)) + ([#3720](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3720)) ## 1.4.0-beta.1 diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 16358254728..8291eb4978e 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -4,7 +4,7 @@ * Added support for loading environment variables from `IConfiguration` when using `TracerProviderBuilder` or `MeterProviderBuilder` - ([#XXXX](https://github.com/open-telemetry/opentelemetry-dotnet/pull/XXXX)) + ([#3720](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3720)) ## 1.4.0-beta.1 From df5b8d363948ffca72f5936d3f4650c5a92ba9f6 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 3 Oct 2022 10:52:36 -0700 Subject: [PATCH 5/7] Bug fixes. --- build/Common.props | 2 +- .../JaegerExporterHelperExtensions.cs | 3 ++- .../JaegerExporterOptions.cs | 12 +++++++++--- src/OpenTelemetry/OpenTelemetry.csproj | 3 ++- 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/build/Common.props b/build/Common.props index 96e224950b9..e7a089f2b96 100644 --- a/build/Common.props +++ b/build/Common.props @@ -35,7 +35,7 @@ [2.1.0,) [3.1.0,) [3.1.0,) - [3.1.0,) + [5.0.0,) [1.0.0,2.0) [1.0.0,2.0) [0.12.1,0.13) diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs index ead9212750d..c315463bde0 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs @@ -18,6 +18,7 @@ using System.Net.Http; using System.Reflection; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Options; using OpenTelemetry.Exporter; using OpenTelemetry.Internal; @@ -69,7 +70,7 @@ public static TracerProviderBuilder AddJaegerExporter( services.Configure(name, configure); } - services.AddSingleton, JaegerExporterOptions.JaegerExporterOptionsFactory>(); + services.TryAddSingleton, JaegerExporterOptions.JaegerExporterOptionsFactory>(); }); return builder.ConfigureBuilder((sp, builder) => diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs index 5d537ad4d58..c3e39eda436 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs @@ -15,6 +15,7 @@ // using System; +using System.Collections.Generic; using System.Diagnostics; using System.Net.Http; using Microsoft.Extensions.Configuration; @@ -158,18 +159,23 @@ internal JaegerExporterOptions(IConfiguration configuration) /// public Func HttpClientFactory { get; set; } = DefaultHttpClientFactory; - internal sealed class JaegerExporterOptionsFactory : IOptionsFactory + internal sealed class JaegerExporterOptionsFactory : OptionsFactory { private readonly IConfiguration configuration; - public JaegerExporterOptionsFactory(IConfiguration configuration) + public JaegerExporterOptionsFactory( + IConfiguration configuration, + IEnumerable> setups, + IEnumerable> postConfigures, + IEnumerable> validations) + : base(setups, postConfigures, validations) { Debug.Assert(configuration != null, "configuration was null"); this.configuration = configuration; } - public JaegerExporterOptions Create(string name) + protected override JaegerExporterOptions CreateInstance(string name) => new(this.configuration); } } diff --git a/src/OpenTelemetry/OpenTelemetry.csproj b/src/OpenTelemetry/OpenTelemetry.csproj index b7f6e7fa3f7..c05c686b45b 100644 --- a/src/OpenTelemetry/OpenTelemetry.csproj +++ b/src/OpenTelemetry/OpenTelemetry.csproj @@ -17,9 +17,10 @@ + - + From 142dbd2f893d769ba6c47cee69fd97161b3ffe92 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 3 Oct 2022 11:09:15 -0700 Subject: [PATCH 6/7] Warning cleanup. --- .../Builder/ProviderBuilderServiceCollectionExtensions.cs | 2 +- .../Builder/MeterProviderBuilderServiceCollectionHelper.cs | 4 ++-- .../Builder/TracerProviderBuilderServiceCollectionHelper.cs | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs b/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs index 276174d5d19..ab136d3a4f0 100644 --- a/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs +++ b/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs @@ -35,7 +35,7 @@ public static IServiceCollection AddOpenTelemetryProviderBuilderServices(this IS // Sdk.Create* style or when manually creating a ServiceCollection. The // point of this registration is to make IConfiguration available in // those cases. - services.TryAddSingleton(sp => new ConfigurationBuilder().AddEnvironmentVariables().Build()); + services!.TryAddSingleton(sp => new ConfigurationBuilder().AddEnvironmentVariables().Build()); return services!; } diff --git a/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderServiceCollectionHelper.cs b/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderServiceCollectionHelper.cs index e53f412828d..779e7f017f6 100644 --- a/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderServiceCollectionHelper.cs +++ b/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderServiceCollectionHelper.cs @@ -42,7 +42,7 @@ internal static IServiceCollection RegisterConfigureStateCallback( Debug.Assert(services != null, "services was null"); Debug.Assert(configure != null, "configure was null"); - return services.AddSingleton(new ConfigureMeterProviderBuilderStateCallbackRegistration(configure!)); + return services!.AddSingleton(new ConfigureMeterProviderBuilderStateCallbackRegistration(configure!)); } internal static void InvokeRegisteredConfigureStateCallbacks( @@ -52,7 +52,7 @@ internal static void InvokeRegisteredConfigureStateCallbacks( Debug.Assert(serviceProvider != null, "serviceProvider was null"); Debug.Assert(state != null, "state was null"); - var callbackRegistrations = serviceProvider.GetServices(); + var callbackRegistrations = serviceProvider!.GetServices(); foreach (var callbackRegistration in callbackRegistrations) { diff --git a/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderServiceCollectionHelper.cs b/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderServiceCollectionHelper.cs index ae68e0881b5..5de52f1a495 100644 --- a/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderServiceCollectionHelper.cs +++ b/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderServiceCollectionHelper.cs @@ -42,7 +42,7 @@ internal static IServiceCollection RegisterConfigureStateCallback( Debug.Assert(services != null, "services was null"); Debug.Assert(configure != null, "configure was null"); - return services.AddSingleton(new ConfigureTracerProviderBuilderStateCallbackRegistration(configure!)); + return services!.AddSingleton(new ConfigureTracerProviderBuilderStateCallbackRegistration(configure!)); } internal static void InvokeRegisteredConfigureStateCallbacks( @@ -52,7 +52,7 @@ internal static void InvokeRegisteredConfigureStateCallbacks( Debug.Assert(serviceProvider != null, "serviceProvider was null"); Debug.Assert(state != null, "state was null"); - var callbackRegistrations = serviceProvider.GetServices(); + var callbackRegistrations = serviceProvider!.GetServices(); foreach (var callbackRegistration in callbackRegistrations) { From 305cfe5e3ce46ab53f74e89f6a84ed78f6d7e726 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 11 Oct 2022 18:10:41 -0700 Subject: [PATCH 7/7] Code review. --- .../JaegerExporterHelperExtensions.cs | 3 +- .../JaegerExporterOptions.cs | 60 ++----- .../JaegerExporterProtocolParser.cs | 2 +- .../OpenTelemetry.Exporter.Jaeger.csproj | 2 +- .../Internal/ConfigurationExtensions.cs | 164 ++++++++++++++++++ 5 files changed, 177 insertions(+), 54 deletions(-) create mode 100644 src/OpenTelemetry/Internal/ConfigurationExtensions.cs diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs index c315463bde0..6a6bd83efc0 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs @@ -18,7 +18,6 @@ using System.Net.Http; using System.Reflection; using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Options; using OpenTelemetry.Exporter; using OpenTelemetry.Internal; @@ -70,7 +69,7 @@ public static TracerProviderBuilder AddJaegerExporter( services.Configure(name, configure); } - services.TryAddSingleton, JaegerExporterOptions.JaegerExporterOptionsFactory>(); + services.RegisterOptionsFactory(configuration => new JaegerExporterOptions(configuration)); }); return builder.ConfigureBuilder((sp, builder) => diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs index c3e39eda436..f2946388b90 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs @@ -15,11 +15,9 @@ // using System; -using System.Collections.Generic; using System.Diagnostics; using System.Net.Http; using Microsoft.Extensions.Configuration; -using Microsoft.Extensions.Options; using OpenTelemetry.Internal; using OpenTelemetry.Trace; @@ -56,45 +54,27 @@ public JaegerExporterOptions() internal JaegerExporterOptions(IConfiguration configuration) { - var protocol = configuration.GetValue(OTelProtocolEnvVarKey, null); - if (!string.IsNullOrEmpty(protocol)) + if (configuration.TryGetValue( + OTelProtocolEnvVarKey, + JaegerExporterProtocolParser.TryParse, + out var protocol)) { - if (JaegerExporterProtocolParser.TryParse(protocol, out var parsedProtocol)) - { - this.Protocol = parsedProtocol; - } - else - { - throw new FormatException($"{OTelProtocolEnvVarKey} environment variable has an invalid value: '{protocol}'"); - } + this.Protocol = protocol; } - var agentHost = configuration.GetValue(OTelAgentHostEnvVarKey, null); - if (!string.IsNullOrEmpty(agentHost)) + if (configuration.TryGetStringValue(OTelAgentHostEnvVarKey, out var agentHost)) { this.AgentHost = agentHost; } - var agentPort = configuration.GetValue(OTelAgentPortEnvVarKey, null); - if (!string.IsNullOrEmpty(agentPort)) + if (configuration.TryGetIntValue(OTelAgentPortEnvVarKey, out var agentPort)) { - if (EnvironmentVariableHelper.LoadNumeric(OTelAgentPortEnvVarKey, agentPort, out int parsedAgentPort)) - { - this.AgentPort = parsedAgentPort; - } + this.AgentPort = agentPort; } - var endpoint = configuration.GetValue(OTelEndpointEnvVarKey, null); - if (!string.IsNullOrEmpty(endpoint)) + if (configuration.TryGetUriValue(OTelEndpointEnvVarKey, out var endpoint)) { - if (Uri.TryCreate(endpoint, UriKind.Absolute, out Uri parsedEndpoint)) - { - this.Endpoint = parsedEndpoint; - } - else - { - throw new FormatException($"{OTelEndpointEnvVarKey} environment variable has an invalid value: '{endpoint}'"); - } + this.Endpoint = endpoint; } } @@ -158,25 +138,5 @@ internal JaegerExporterOptions(IConfiguration configuration) /// /// public Func HttpClientFactory { get; set; } = DefaultHttpClientFactory; - - internal sealed class JaegerExporterOptionsFactory : OptionsFactory - { - private readonly IConfiguration configuration; - - public JaegerExporterOptionsFactory( - IConfiguration configuration, - IEnumerable> setups, - IEnumerable> postConfigures, - IEnumerable> validations) - : base(setups, postConfigures, validations) - { - Debug.Assert(configuration != null, "configuration was null"); - - this.configuration = configuration; - } - - protected override JaegerExporterOptions CreateInstance(string name) - => new(this.configuration); - } } } diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterProtocolParser.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterProtocolParser.cs index 52fb109350f..2a770135541 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterProtocolParser.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterProtocolParser.cs @@ -20,7 +20,7 @@ internal static class JaegerExporterProtocolParser { public static bool TryParse(string value, out JaegerExportProtocol result) { - switch (value?.Trim()) + switch (value.Trim().ToLower()) { case "udp/thrift.compact": result = JaegerExportProtocol.UdpCompactThrift; diff --git a/src/OpenTelemetry.Exporter.Jaeger/OpenTelemetry.Exporter.Jaeger.csproj b/src/OpenTelemetry.Exporter.Jaeger/OpenTelemetry.Exporter.Jaeger.csproj index abd411f0df3..2720fd1a2b2 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/OpenTelemetry.Exporter.Jaeger.csproj +++ b/src/OpenTelemetry.Exporter.Jaeger/OpenTelemetry.Exporter.Jaeger.csproj @@ -19,7 +19,7 @@ - + diff --git a/src/OpenTelemetry/Internal/ConfigurationExtensions.cs b/src/OpenTelemetry/Internal/ConfigurationExtensions.cs new file mode 100644 index 00000000000..dc22bbf673c --- /dev/null +++ b/src/OpenTelemetry/Internal/ConfigurationExtensions.cs @@ -0,0 +1,164 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +#nullable enable + +using System; +using System.Collections.Generic; +using System.Diagnostics; +#if !NETFRAMEWORK && !NETSTANDARD2_0 +using System.Diagnostics.CodeAnalysis; +#endif +using System.Globalization; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Options; + +namespace OpenTelemetry.Internal; + +internal static class ConfigurationExtensions +{ + public delegate bool TryParseFunc( + string value, +#if !NETFRAMEWORK && !NETSTANDARD2_0 + [NotNullWhen(true)] +#endif + out T? parsedValue); + + public static bool TryGetStringValue( + this IConfiguration configuration, + string key, +#if !NETFRAMEWORK && !NETSTANDARD2_0 + [NotNullWhen(true)] +#endif + out string? value) + { + value = configuration.GetValue(key, null); + + return !string.IsNullOrWhiteSpace(value); + } + + public static bool TryGetUriValue( + this IConfiguration configuration, + string key, +#if !NETFRAMEWORK && !NETSTANDARD2_0 + [NotNullWhen(true)] +#endif + out Uri? value) + { + if (!configuration.TryGetStringValue(key, out var stringValue)) + { + value = null; + return false; + } + + if (!Uri.TryCreate(stringValue, UriKind.Absolute, out value)) + { + throw new FormatException($"{key} environment variable has an invalid value: '{stringValue}'"); + } + + return true; + } + + public static bool TryGetIntValue( + this IConfiguration configuration, + string key, + out int value) + { + if (!configuration.TryGetStringValue(key, out var stringValue)) + { + value = default; + return false; + } + + if (!int.TryParse(stringValue, NumberStyles.None, CultureInfo.InvariantCulture, out value)) + { + throw new FormatException($"{key} environment variable has an invalid value: '{stringValue}'"); + } + + return true; + } + + public static bool TryGetValue( + this IConfiguration configuration, + string key, + TryParseFunc tryParseFunc, +#if !NETFRAMEWORK && !NETSTANDARD2_0 + [NotNullWhen(true)] +#endif + out T? value) + { + if (!configuration.TryGetStringValue(key, out var stringValue)) + { + value = default; + return false; + } + + if (!tryParseFunc(stringValue!, out value)) + { + throw new FormatException($"{key} environment variable has an invalid value: '{stringValue}'"); + } + + return true; + } + + public static IServiceCollection RegisterOptionsFactory( + this IServiceCollection services, + Func optionsFactoryFunc) + where T : class + { + Debug.Assert(services != null, "services was null"); + Debug.Assert(optionsFactoryFunc != null, "optionsFactoryFunc was null"); + + services!.TryAddSingleton>(sp => + { + return new DelegatingOptionsFactory( + optionsFactoryFunc!, + sp.GetRequiredService(), + sp.GetServices>(), + sp.GetServices>(), + sp.GetServices>()); + }); + + return services!; + } + + private sealed class DelegatingOptionsFactory : OptionsFactory + where T : class + { + private readonly Func optionsFactoryFunc; + private readonly IConfiguration configuration; + + public DelegatingOptionsFactory( + Func optionsFactoryFunc, + IConfiguration configuration, + IEnumerable> setups, + IEnumerable> postConfigures, + IEnumerable> validations) + : base(setups, postConfigures, validations) + { + Debug.Assert(optionsFactoryFunc != null, "optionsFactoryFunc was null"); + Debug.Assert(configuration != null, "configuration was null"); + + this.optionsFactoryFunc = optionsFactoryFunc!; + this.configuration = configuration!; + } + + protected override T CreateInstance(string name) + => this.optionsFactoryFunc(this.configuration); + } +}