Skip to content

Conversation

@CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Jan 20, 2023

Fixes #4084

Changes

Consumes the bits from Microsoft.Extensions.Configuration.EnvironmentVariables so the SDK doesn't need a dependency.

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes

@codecov
Copy link

codecov bot commented Jan 21, 2023

Codecov Report

Merging #4092 (ea968ca) into main (84b3778) will decrease coverage by 0.11%.
The diff coverage is 62.22%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4092      +/-   ##
==========================================
- Coverage   85.59%   85.49%   -0.11%     
==========================================
  Files         289      292       +3     
  Lines       11261    11306      +45     
==========================================
+ Hits         9639     9666      +27     
- Misses       1622     1640      +18     
Impacted Files Coverage Δ
...ronmentVariables/EnvironmentVariablesExtensions.cs 40.00% <40.00%> (ø)
...ables/EnvironmentVariablesConfigurationProvider.cs 63.15% <63.15%> (ø)
...riables/EnvironmentVariablesConfigurationSource.cs 100.00% <100.00%> (ø)
src/OpenTelemetry/BatchExportProcessor.cs 82.24% <0.00%> (-1.87%) ⬇️
...Listener/Internal/PrometheusExporterEventSource.cs 66.66% <0.00%> (+5.55%) ⬆️

Comment on lines +18 to +21
private const string MySqlServerPrefix = "MYSQLCONNSTR_";
private const string SqlAzureServerPrefix = "SQLAZURECONNSTR_";
private const string SqlServerPrefix = "SQLCONNSTR_";
private const string CustomConnectionStringPrefix = "CUSTOMCONNSTR_";
Copy link
Member Author

Choose a reason for hiding this comment

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

@alanwest @cijothomas I was just chatting with @reyang about this. So the new feature we are supporting in 1.4.0 is we will look for environment variables using IConfiguration. This code is only ever used for Sdk.Create* style to build IConfiguration from environment variables as backwards compatibility for users without hosting. So strictly speaking, we do not need this connection string logic. We don't support it in 1.3.0 and we don't need to in 1.4.0. Thinking we should remove this and slim it down as a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

@CodeBlanch CodeBlanch marked this pull request as ready for review January 24, 2023 20:12
@CodeBlanch CodeBlanch requested a review from a team January 24, 2023 20:12
@CodeBlanch CodeBlanch merged commit 03af717 into open-telemetry:main Jan 24, 2023
@CodeBlanch CodeBlanch deleted the envvar-dependency branch January 24, 2023 21:22
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM

xiang17 added a commit to xiang17/opentelemetry-dotnet-instrumentation that referenced this pull request Feb 2, 2023
aunikitin pushed a commit to aunikitin/opentelemetry-dotnet that referenced this pull request Feb 4, 2023
…etry#4092)

* Remove the dependency on Microsoft.Extensions.Configuration.EnvironmentVariables from SDK.

* Added notes.

* CHANGELOG patch.
@reyang reyang mentioned this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider decoupling EnvariableVariable implementation from the SDK

5 participants