Skip to content

Conversation

@TimothyMothra
Copy link

Towards #3690

According the OTel spec:

For variables accepting an enum value, if the user provides a value the SDK does not recognize, the SDK MUST generate a warning and gracefully ignore the setting.

In this PR, I changed the TryParse to NOT throw FormatException and instead log to EventSource.
Unit tests and documentation will be fixed in a follow up PR to keep the PR a reasonable size.

Changes

  • ConfigurationExtensions
    remove throw FormatException and instead log a Warning in EventSource.
  • temporarily disable unit tests.

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@TimothyMothra TimothyMothra requested a review from a team January 21, 2023 00:53
{
throw new FormatException($"{key} environment variable has an invalid value: '{stringValue}'");
OpenTelemetrySdkEventSource.Log.InvalidEnvironmentVariable(key, stringValue);
value = default;
Copy link
Member

Choose a reason for hiding this comment

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

Should value be touched here? (I guess no)

Copy link
Author

Choose a reason for hiding this comment

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

good catch! it's an out param so it needs to be set, but it's already been set by the Try method in the if statement.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@codecov
Copy link

codecov bot commented Jan 21, 2023

Codecov Report

Merging #4095 (a399c88) into main (10389e3) will decrease coverage by 0.11%.
The diff coverage is 25.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4095      +/-   ##
==========================================
- Coverage   85.67%   85.57%   -0.11%     
==========================================
  Files         289      289              
  Lines       11256    11261       +5     
==========================================
- Hits         9644     9637       -7     
- Misses       1612     1624      +12     
Impacted Files Coverage Δ
.../OpenTelemetry/Internal/ConfigurationExtensions.cs 87.50% <0.00%> (-12.50%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 80.46% <100.00%> (+0.31%) ⬆️
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs 72.72% <0.00%> (-13.64%) ⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs 85.00% <0.00%> (-10.00%) ⬇️
...tpListener/Internal/PrometheusCollectionManager.cs 73.62% <0.00%> (-2.20%) ⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️

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.

3 participants