-
Notifications
You must be signed in to change notification settings - Fork 772
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
Changes from 6 commits
81cc629
d4ccabf
1405478
b20705e
df5b8d3
142dbd2
305cfe5
cff5bc7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No warning from the missing null forgiving operator?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This class is internal and |
||||||
|
||||||
// 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!; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,8 +17,10 @@ | |
|
||
<ItemGroup> | ||
<PackageReference Include="System.Reflection.Emit.Lightweight" Version="$(SystemReflectionEmitLightweightPkgVer)" Condition="'$(TargetFramework)' != 'net6.0'" /> | ||
<PackageReference Include="Microsoft.Extensions.Configuration.EnvironmentVariables" Version="$(MicrosoftExtensionsConfigurationEnvironmentVariablesPkgVer)" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed to initialize IConfiguration from environment variables when no IConfiguration is found. No one wants the dependency but OTel spec calls for a lot of environment variable behavior so I don't think it is unreasonable for SDK to have this. Anyone using the AspNetCore "meta" reference already has this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm anticipating a future desire for an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Less filling, tastes great. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just want to call out that bringing more and more dependencies increases the chances of an assembly version conflict which is especially annoying for .NET Auto-Instrumentation 😭 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alanwest and I kicked around some ideas/options, but we haven't been able to come up with anything really great for avoiding this. When Auto-Instrumentation copies in references, does it do so blindly? Or do you somehow try to reason out what is there prior to doing the copy? |
||
<PackageReference Include="Microsoft.Extensions.Logging" Version="$(MicrosoftExtensionsLoggingPkgVer)" /> | ||
<PackageReference Include="Microsoft.Extensions.Logging.Configuration" Version="$(MicrosoftExtensionsLoggingConfigurationPkgVer)" /> | ||
<PackageReference Include="Microsoft.Extensions.Options" Version="$(MicrosoftExtensionsOptionsPkgVer)" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already had Microsoft.Extensions.Options through Microsoft.Extensions.Logging.Configuration but now we ask for >= 5.0.0. The reason for that is to gain access to OptionsFactory.CreateInstance which allows us to initialize the instance created for options using IConfiguration + the custom environment variables defined in OTel spec. |
||
</ItemGroup> | ||
|
||
<ItemGroup> | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?