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

CodeBlanch · 2022-10-03T17:23:59Z

Relates to #2980
Relates to #3639

Changes

Update SDK so that IConfiguration is always available in tracing & metrics builders.
Update Jaeger to load environment variables using IConfiguration.

Details

When using a host builder (generic host or web host builder) IConfiguration is there by default and users may customize it.
When using "Sdk.Create" methods or an empty ServiceCollection we now create an IConfiguration which includes environment variables by default. Users may customize it if they wish.

TODOs

Appropriate CHANGELOG.md updated for non-trivial changes

…SDK.

CodeBlanch · 2022-10-03T18:12:04Z

src/OpenTelemetry/OpenTelemetry.csproj

@@ -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)" />


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.

I'm anticipating a future desire for an OpenTelemetry .NET Lite package... let's hope not 😆

Less filling, tastes great.

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 😭

Reference: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/blob/main/docs/troubleshooting.md#assembly-version-conflicts

@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?

CodeBlanch · 2022-10-03T18:14:54Z

src/OpenTelemetry/OpenTelemetry.csproj

    <PackageReference Include="Microsoft.Extensions.Logging" Version="$(MicrosoftExtensionsLoggingPkgVer)" />
    <PackageReference Include="Microsoft.Extensions.Logging.Configuration" Version="$(MicrosoftExtensionsLoggingConfigurationPkgVer)" />
+    <PackageReference Include="Microsoft.Extensions.Options" Version="$(MicrosoftExtensionsOptionsPkgVer)" />


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.

codecov · 2022-10-03T18:20:56Z

Codecov Report

Merging #3720 (cff5bc7) into main (a6c3cac) will increase coverage by 0.07%.
The diff coverage is 98.55%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3720      +/-   ##
==========================================
+ Coverage   87.08%   87.15%   +0.07%     
==========================================
  Files         275      277       +2     
  Lines       10012    10060      +48     
==========================================
+ Hits         8719     8768      +49     
+ Misses       1293     1292       -1

Impacted Files	Coverage Δ
.../OpenTelemetry/Internal/ConfigurationExtensions.cs	`97.14% <97.14%> (ø)`
....Exporter.Jaeger/JaegerExporterHelperExtensions.cs	`98.11% <100.00%> (+0.28%)`	⬆️
...Telemetry.Exporter.Jaeger/JaegerExporterOptions.cs	`100.00% <100.00%> (ø)`
...ry.Exporter.Jaeger/JaegerExporterProtocolParser.cs	`100.00% <100.00%> (ø)`
...lder/ProviderBuilderServiceCollectionExtensions.cs	`100.00% <100.00%> (ø)`
...penTelemetry/Internal/EnvironmentVariableHelper.cs	`85.18% <100.00%> (+0.56%)`	⬆️
...emetry/Metrics/Builder/MeterProviderBuilderBase.cs	`96.03% <100.00%> (ø)`
...der/MeterProviderBuilderServiceCollectionHelper.cs	`100.00% <100.00%> (ø)`
...lemetry/Trace/Builder/TracerProviderBuilderBase.cs	`91.50% <100.00%> (ø)`
...er/TracerProviderBuilderServiceCollectionHelper.cs	`100.00% <100.00%> (ø)`
... and 4 more

github-actions · 2022-10-11T03:59:58Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs

alanwest · 2022-10-11T16:54:08Z

src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs

+    {
+        Debug.Assert(services != null, "services was null");
+
+        services.AddOptions();


No warning from the missing null forgiving operator?

Suggested change

services.AddOptions();

services!.AddOptions();

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.

alanwest · 2022-10-11T17:06:33Z

build/Common.props

    <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>


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.

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?

pellared · 2022-10-12T07:32:32Z

src/OpenTelemetry/Internal/ConfigurationExtensions.cs

+
+        if (!Uri.TryCreate(stringValue, UriKind.Absolute, out value))
+        {
+            throw new FormatException($"{key} environment variable has an invalid value: '{stringValue}'");


Just a bug reminder #3690 😉

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

* [Logs] Fix buffered log scopes being reused (#3731) * Fix buffered log scopes being reused. * CHANGELOG update. * Test fixes. Co-authored-by: Cijo Thomas <cithomas@microsoft.com> * clarify that Prometheus HttpListner is not for production at this moment (#3737) * [HttpClient] Export spans corresponding to retries (#3732) * Minor update to OTLP readme (#3739) * Nit fixes to prometheus readme * Add minor clarification about OTLP logs * Update workflow (#3741) * Minor improvement to log message (#3742) * [SDK + Jaeger] Support loading environment variables from IConfiguration in Traces & Metrics (#3720) * Support retrieval of environment variables through IConfiguration in SDK. * Update Jaeger to load environment variables through IConfiguration. * Warning fix. * CHANGELOG patch. * Bug fixes. * Warning cleanup. * Code review. * [Zipkin] Support loading envvars from IConfiguration (#3759) * Support loading envvars from IConfiguration in Zipkin exporter. * CHANGELOG patch. * SqlClient Instrumentation to leverage native Activity Status. (#3751) * [Metrics] Update default buckets for Explicit Bucket Histogram from spec (#3722) * [Logs] Fix: Respect AttributeValueLengthLimit when building otlp LogRecord data (#3684) * Respect SdkConfiguration.AttributeValueLengthLimit so otlp data points are not rejected by monitoring tools * Respect maxAttributeCount and use OtlpKeyValueTransformer in AddStringAttribute and in AddIntAttribute * Extend CHANGELOG.md Co-authored-by: Mikel Blanchard <mblanchard@macrosssoftware.com> * Bump actions/setup-dotnet from 3.0.1 to 3.0.2 (#3764) Bumps [actions/setup-dotnet](https://github.com/actions/setup-dotnet) from 3.0.1 to 3.0.2. - [Release notes](https://github.com/actions/setup-dotnet/releases) - [Commits](actions/setup-dotnet@v3.0.1...v3.0.2) --- updated-dependencies: - dependency-name: actions/setup-dotnet dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * [Repo] Attempting to stabilize the API Compatibility CI job (#3766) * Attempting to stablize the API Compatibility CI. * Tweak. * More logging. * Increase build logging level for api compat ci job. * Tweak. * Tweak. * Revert extra logging in ci job. * Switched some logging back to debug. * Adding MinMax to Histograms (#2735) * Logging state during building of TracerProvider (#3746) * [HttpClient] Add unit tests for `RecordException` case (#3761) * Add a separate example project for Logs redaction (#3744) * Update CHANGELOG for 1.4.0-beta.2 release (#3772) * [SDK + Otlp] Support loading envvars from IConfiguration (#3760) * Updated Otlp Trace & Metrics exporters to load envvars from IConfiguration. * Patch CHANGELOGs. * Fix up otlp log exporter for SdkOptions changes. * Revert SdkOptions public api. * Restore tests. * Fix benchmarks. * SdkLimitOptions IConfiguration test. * OtlpExporterOptions IConfiguration test. * MetricReaderOptions IConfiguration test. * Bug fix. * Nit. * [SqlClient] Add support for Filter expression (#3743) * [SDK, Jaeger, Zipkin, & Otlp] Support loading envvars for BatchExportActivityProcessorOptions from IConfiguration (#3776) * Support loading envvars for ExportActivityProcessorOptions & BatchExportActivityProcessorOptions from IConfiguration. * Update src/OpenTelemetry/CHANGELOG.md Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com> * Patch CHANGELOG. * Unit test. Co-authored-by: Cijo Thomas <cithomas@microsoft.com> Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com> * Remove Env from CI `DOTNET_MULTILEVEL_LOOKUP = 1` (#3779) * Link to .NET docs about different ports (#3704) * Update DS to rc2 (#3781) * ConsoleLogExporter to output full exception (#3784) * [ASP.NET Core] Add back netstandard2.0 and 2.1 targets (#3755) * [HttpClient] Add back netstandard2.0 target (#3787) * Add back netstandard2.0 target * changelog * public api files * Bump System.Text.Json version due to CVE-2021-26701 (#3789) * Auto-generated Semantic Conventions (#2069) * [SDK] Support dependency injection in ResourceBuilder and load envvars from IConfiguration (#3782) * Add Vishwesh as an Approver (#3783) Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> * Move more SDK docs to docs folder (#3794) * [Prometheus AspNetCore] Support named options in pipeline extensions (#3780) * Support named options in Prometheus AspNetCore pipeline extensions. * Patch CHANGELOG. Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> * [Logs] UnitTest: LogRecord attribute limits (#3758) * Unittest for LogRecord attribute limits * Remove maxValueLength from LogRecordExtensions.AddIntAttribute. Change requested from #3684 (comment) * Pr commits addressed Co-authored-by: Mikel Blanchard <mblanchard@macrosssoftware.com> * Add MinMax to console and doc additions (#3795) * Mark private and internal classes as sealed (#3799) Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> * Move more docs to docs folder (#3801) * [ASP.NET Core] Update enrich callbacks to use specific type. (#3749) * [Http] Update enrich callbacks for http (#3792) * [SDK] Support dependency injection in the GetDefaultResource API (#3798) * Support dependency injection in the GetDefaultResource API. * CHANGELOG patch. * Test tweaks. Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> * Fix circular reference issue building up tracer provider. (#3803) * [SDK] Add some missing nullable annotations (#3796) * Added some missing nullable annotations in SDK. * Handle null span names in SamplingParameters. * Warning cleanup. * Warning cleanup. * Added link to issue in comment. Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com> * Guard.Range now uses invariant culture for error message (#3778) Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com> * Fix circular reference issue building up meter provider. (#3806) Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com> * Add missing end of code block backticks (#3807) * More doc tweaks (#3805) * More doc tweaks * remove draft staatus * Update grpc client enrich callbacks (#3804) * Port refactor from main-logs branch. (#3808) Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> * Merge fixes. * Merge fixes. * Merge fix. Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Cijo Thomas <cithomas@microsoft.com> Co-authored-by: Reiley Yang <reyang@microsoft.com> Co-authored-by: Vishwesh Bankwar <vishweshbankwar@users.noreply.github.com> Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> Co-authored-by: Yun-Ting Lin <yunl@microsoft.com> Co-authored-by: Sebastian Schoder Moreno <35150382+schoder-moreno@users.noreply.github.com> Co-authored-by: Jonathan Wilhelm <Jonathan.wilhelm@sage.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Michael Maxwell <micmax@microsoft.com> Co-authored-by: ggoel <gaurav.goel111@gmail.com> Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com> Co-authored-by: Pavel Steinl <pavel.steinl@gmail.com> Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com> Co-authored-by: aristotelos <arisvd@gmail.com> Co-authored-by: Joao Grassi <joao@joaograssi.com> Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com> Co-authored-by: benhall_io <3179852+benbhall@users.noreply.github.com>

CodeBlanch added 2 commits October 3, 2022 10:13

Support retrieval of environment variables through IConfiguration in …

81cc629

…SDK.

Update Jaeger to load environment variables through IConfiguration.

d4ccabf

CodeBlanch requested a review from a team October 3, 2022 17:23

CodeBlanch added 4 commits October 3, 2022 10:26

Warning fix.

1405478

CHANGELOG patch.

b20705e

Bug fixes.

df5b8d3

Warning cleanup.

142dbd2

CodeBlanch commented Oct 3, 2022

View reviewed changes

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 11, 2022

pellared reviewed Oct 11, 2022

View reviewed changes

src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs Outdated Show resolved Hide resolved

pellared reviewed Oct 11, 2022

View reviewed changes

src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs Outdated Show resolved Hide resolved

alanwest reviewed Oct 11, 2022

View reviewed changes

alanwest approved these changes Oct 11, 2022

View reviewed changes

alanwest removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 11, 2022

CodeBlanch added 2 commits October 11, 2022 18:10

Code review.

305cfe5

Merge from main.

cff5bc7

pellared reviewed Oct 12, 2022

View reviewed changes

CodeBlanch merged commit f191e84 into open-telemetry:main Oct 12, 2022

CodeBlanch deleted the sdk-envvars-iconfiguration-support branch October 12, 2022 16:56

moonbox3 mentioned this pull request Nov 14, 2022

FormatException is thrown when an invalid env var value is set #3690

Closed

cijothomas mentioned this pull request Jan 18, 2023

Consider decoupling EnvariableVariable implementation from the SDK #4084

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

CodeBlanch commented Oct 3, 2022 •

edited

Loading

CodeBlanch Oct 3, 2022 •

edited

Loading

alanwest Oct 11, 2022

CodeBlanch Oct 11, 2022

pellared Oct 12, 2022

CodeBlanch Oct 12, 2022

CodeBlanch Oct 3, 2022

codecov bot commented Oct 3, 2022 •

edited

Loading

github-actions bot commented Oct 11, 2022

alanwest Oct 11, 2022

CodeBlanch Oct 11, 2022

alanwest Oct 11, 2022

CodeBlanch Oct 11, 2022

pellared Oct 12, 2022

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

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

Conversation

CodeBlanch commented Oct 3, 2022 • edited Loading

Changes

Details

TODOs

CodeBlanch Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 3, 2022 • edited Loading

Codecov Report

github-actions bot commented Oct 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented Oct 3, 2022 •

edited

Loading

CodeBlanch Oct 3, 2022 •

edited

Loading

codecov bot commented Oct 3, 2022 •

edited

Loading