Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDK + Jaeger] Support loading environment variables from IConfiguration in Traces & Metrics #3720

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion build/Common.props
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,11 @@
<MicrosoftAspNetCoreHttpFeaturesPkgVer>[2.1.1,6.0)</MicrosoftAspNetCoreHttpFeaturesPkgVer>
<MicrosoftCodeAnalysisAnalyzersPkgVer>[3.3.3]</MicrosoftCodeAnalysisAnalyzersPkgVer>
<MicrosoftCodeCoveragePkgVer>[17.3.0]</MicrosoftCodeCoveragePkgVer>
<MicrosoftExtensionsConfigurationEnvironmentVariablesPkgVer>[3.1.0,)</MicrosoftExtensionsConfigurationEnvironmentVariablesPkgVer>
<MicrosoftExtensionsHostingAbstractionsPkgVer>[2.1.0,)</MicrosoftExtensionsHostingAbstractionsPkgVer>
<MicrosoftExtensionsLoggingPkgVer>[3.1.0,)</MicrosoftExtensionsLoggingPkgVer>
<MicrosoftExtensionsLoggingConfigurationPkgVer>[3.1.0,)</MicrosoftExtensionsLoggingConfigurationPkgVer>
<MicrosoftExtensionsOptionsPkgVer>[3.1.0,)</MicrosoftExtensionsOptionsPkgVer>
<MicrosoftExtensionsOptionsPkgVer>[5.0.0,)</MicrosoftExtensionsOptionsPkgVer>
Comment on lines 35 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes me wonder what the harm would be in bumping all the extension packages to [5.0.0,)?

Just poking around the dependencies of the Microsoft.Extensions.* packages themselves seems to suggest they're always bumped in unison.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an issue with that. I'm not going to do it on this PR but we could bring it up on SIG?

<MicrosoftNETFrameworkReferenceAssembliesPkgVer>[1.0.0,2.0)</MicrosoftNETFrameworkReferenceAssembliesPkgVer>
<MicrosoftSourceLinkGitHubPkgVer>[1.0.0,2.0)</MicrosoftSourceLinkGitHubPkgVer>
<OpenTracingPkgVer>[0.12.1,0.13)</OpenTracingPkgVer>
Expand Down
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

* Added support for loading environment variables from `IConfiguration` when
using the `AddJaegerExporter` extension
([#3720](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3720))

## 1.4.0-beta.1

Released 2022-Sep-29
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.RegisterOptionsFactory(configuration => new JaegerExporterOptions(configuration));
});

return builder.ConfigureBuilder((sp, builder) =>
{
Expand Down
39 changes: 24 additions & 15 deletions src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using System;
using System.Diagnostics;
using System.Net.Http;
using Microsoft.Extensions.Configuration;
using OpenTelemetry.Internal;
using OpenTelemetry.Trace;

Expand All @@ -43,37 +44,45 @@ public class JaegerExporterOptions

internal static readonly Func<HttpClient> DefaultHttpClientFactory = () => new HttpClient();

/// <summary>
/// Initializes a new instance of the <see cref="JaegerExporterOptions"/> class.
/// </summary>
public JaegerExporterOptions()
: this(new ConfigurationBuilder().AddEnvironmentVariables().Build())
{
if (EnvironmentVariableHelper.LoadString(OTelProtocolEnvVarKey, out string protocolEnvVar))
}

internal JaegerExporterOptions(IConfiguration configuration)
{
if (configuration.TryGetValue<JaegerExportProtocol>(
OTelProtocolEnvVarKey,
JaegerExporterProtocolParser.TryParse,
out var protocol))
{
if (JaegerExporterProtocolParser.TryParse(protocolEnvVar, out var protocol))
{
this.Protocol = protocol;
}
else
{
throw new FormatException($"{OTelProtocolEnvVarKey} environment variable has an invalid value: '{protocolEnvVar}'");
}
this.Protocol = protocol;
}

if (EnvironmentVariableHelper.LoadString(OTelAgentHostEnvVarKey, out string agentHostEnvVar))
if (configuration.TryGetStringValue(OTelAgentHostEnvVarKey, out var agentHost))
{
this.AgentHost = agentHostEnvVar;
this.AgentHost = agentHost;
}

if (EnvironmentVariableHelper.LoadNumeric(OTelAgentPortEnvVarKey, out int agentPortEnvVar))
if (configuration.TryGetIntValue(OTelAgentPortEnvVarKey, out var agentPort))
{
this.AgentPort = agentPortEnvVar;
this.AgentPort = agentPort;
}

if (EnvironmentVariableHelper.LoadString(OTelEndpointEnvVarKey, out string endpointEnvVar)
&& Uri.TryCreate(endpointEnvVar, UriKind.Absolute, out Uri endpoint))
if (configuration.TryGetUriValue(OTelEndpointEnvVarKey, out var endpoint))
{
this.Endpoint = endpoint;
}
}

/// <summary>
/// Gets or sets the <see cref="JaegerExportProtocol"/> to use when
/// communicating to Jaeger. Default value: <see
/// cref="JaegerExportProtocol.UdpCompactThrift"/>.
/// </summary>
public JaegerExportProtocol Protocol { get; set; } = JaegerExportProtocol.UdpCompactThrift;

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
<Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ActivityHelperExtensions.cs" Link="Includes\ActivityHelperExtensions.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\StatusHelper.cs" Link="Includes\StatusHelper.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\Guard.cs" Link="Includes\Guard.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\EnvironmentVariableHelper.cs" Link="Includes\EnvironmentVariableHelper.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\ConfigurationExtensions.cs" Link="Includes\ConfigurationExtensions.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\OpenTelemetrySdkEventSource.cs" Link="Includes\OpenTelemetrySdkEventSource.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\PooledList.cs" Link="Includes\PooledList.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\PeerServiceResolver.cs" Link="Includes\PeerServiceResolver.cs" />
Expand Down
4 changes: 4 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
`BatchLogRecordExportProcessor.OnEnd` is fired.
([#3731](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3731))

* Added support for loading environment variables from `IConfiguration` when
using `TracerProviderBuilder` or `MeterProviderBuilder`
([#3720](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3720))

## 1.4.0-beta.1

Released 2022-Sep-29
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// <copyright file="ProviderBuilderServiceCollectionExtensions.cs" company="OpenTelemetry Authors">
// 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.
// </copyright>

#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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No warning from the missing null forgiving operator?

Suggested change
services.AddOptions();
services!.AddOptions();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is internal and services is not decorated as nullable (NOT IServiceCollection? services) so really, it shouldn't warn. You would get a warning at the call site if you try to pass a null into it. For net6.0 & netstandard2.1 targets, it works fine. For net462 & netstandard2.0 it doesn't warn on this line but it does warn on the two de-references below this! That is all kinds of wrong 🤣 Nullable analysis just doesn't work well on anything before netstandard2.1. So a lot of these !s are there just to make the IDE happy for old targets. In contrib I did this. That turns off nullable warnings but then you have to have some newer target available to vet valid warnings. You get weirdness like this where it is adding targets just for analysis.


// 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<IConfiguration>(sp => new ConfigurationBuilder().AddEnvironmentVariables().Build());

return services!;
}
}
164 changes: 164 additions & 0 deletions src/OpenTelemetry/Internal/ConfigurationExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
// <copyright file="ConfigurationExtensions.cs" company="OpenTelemetry Authors">
// 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.
// </copyright>

#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<T>(
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<string?>(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}'");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a bug reminder #3690 😉

Of course, it can (and should) be addressed in a separate issue. Feel free to resolve this comment.

}

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<T>(
this IConfiguration configuration,
string key,
TryParseFunc<T> 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<T>(
this IServiceCollection services,
Func<IConfiguration, T> optionsFactoryFunc)
where T : class
{
Debug.Assert(services != null, "services was null");
Debug.Assert(optionsFactoryFunc != null, "optionsFactoryFunc was null");

services!.TryAddSingleton<IOptionsFactory<T>>(sp =>
{
return new DelegatingOptionsFactory<T>(
optionsFactoryFunc!,
sp.GetRequiredService<IConfiguration>(),
sp.GetServices<IConfigureOptions<T>>(),
sp.GetServices<IPostConfigureOptions<T>>(),
sp.GetServices<IValidateOptions<T>>());
});

return services!;
}

private sealed class DelegatingOptionsFactory<T> : OptionsFactory<T>
where T : class
{
private readonly Func<IConfiguration, T> optionsFactoryFunc;
private readonly IConfiguration configuration;

public DelegatingOptionsFactory(
Func<IConfiguration, T> optionsFactoryFunc,
IConfiguration configuration,
IEnumerable<IConfigureOptions<T>> setups,
IEnumerable<IPostConfigureOptions<T>> postConfigures,
IEnumerable<IValidateOptions<T>> 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);
}
}
19 changes: 19 additions & 0 deletions src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,25 @@ public static bool LoadNumeric(string envVarKey, out int result)
return false;
}

return LoadNumeric(envVarKey, value, out result);
}

/// <summary>
/// Reads an environment variable and parses is as a
/// <a href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#numeric-value">
/// numeric value</a> - a non-negative decimal integer.
/// </summary>
/// <param name="envVarKey">The name of the environment variable.</param>
/// <param name="value">The value of the environment variable.</param>
/// <param name="result">The parsed value of the environment variable.</param>
/// <returns>
/// Returns <c>true</c> when a non-empty value was read; otherwise, <c>false</c>.
/// </returns>
/// <exception cref="FormatException">
/// Thrown when failed to parse the non-empty value.
/// </exception>
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}'");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ internal MeterProviderBuilderBase(IServiceCollection services)
{
Guard.ThrowIfNull(services);

services.AddOptions();
services.AddOpenTelemetryProviderBuilderServices();
services.TryAddSingleton<MeterProvider>(sp => new MeterProviderSdk(sp, ownsServiceProvider: false));

this.services = services;
Expand All @@ -65,7 +65,7 @@ protected MeterProviderBuilderBase()
{
var services = new ServiceCollection();

services.AddOptions();
services.AddOpenTelemetryProviderBuilderServices();

this.services = services;
this.ownsServices = true;
Expand Down
Loading