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

Consolidate environment variable parsing #2500

Merged
merged 26 commits into from
Oct 26, 2021

Conversation

pellared
Copy link
Member

@pellared pellared commented Oct 20, 2021

Why

Address: #2219 (comment) proposed by @reyang

Partially addresses #1453.

What

Introduce EnvironmentVariableHelper and reuse it when reading the environmental variables.

Remarks

I also created open-telemetry/opentelemetry-specification#2045 when working on this issue. Thumbs up, approvals, comments, any feedback put there is welcome 😉

@codecov
Copy link

codecov bot commented Oct 20, 2021

Codecov Report

Merging #2500 (6cc2ce9) into main (105c068) will increase coverage by 0.11%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2500      +/-   ##
==========================================
+ Coverage   80.47%   80.59%   +0.11%     
==========================================
  Files         253      254       +1     
  Lines        8549     8507      -42     
==========================================
- Hits         6880     6856      -24     
+ Misses       1669     1651      -18     
Impacted Files Coverage Δ
...Jaeger/Implementation/JaegerExporterEventSource.cs 50.00% <ø> (-3.85%) ⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs 68.42% <ø> (+7.13%) ⬆️
...Zipkin/Implementation/ZipkinExporterEventSource.cs 50.00% <ø> (-22.73%) ⬇️
...penTelemetry/Internal/EnvironmentVariableHelper.cs 80.00% <80.00%> (ø)
...Telemetry.Exporter.Jaeger/JaegerExporterOptions.cs 100.00% <100.00%> (+15.78%) ⬆️
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 100.00% <100.00%> (+9.09%) ⬆️
...Telemetry.Exporter.Zipkin/ZipkinExporterOptions.cs 100.00% <100.00%> (ø)
...OpenTelemetry/Resources/OtelEnvResourceDetector.cs 81.25% <100.00%> (-1.11%) ⬇️
...lemetry/Resources/OtelServiceNameEnvVarDetector.cs 72.72% <100.00%> (-2.28%) ⬇️
...metry/Trace/BatchExportActivityProcessorOptions.cs 100.00% <100.00%> (+18.18%) ⬆️
... and 6 more

@pellared pellared changed the title [WIP] Introduce EnvironmentVariableHelper [WIP] Consolidate environmental variable parsing Oct 20, 2021
@pellared pellared changed the title [WIP] Consolidate environmental variable parsing Consolidate environment variable parsing Oct 20, 2021
@pellared pellared marked this pull request as ready for review October 20, 2021 18:54
@pellared pellared requested review from a team, RassK and dszmigielski October 20, 2021 18:54
@pellared pellared requested a review from mic-max October 21, 2021 18:34
Copy link
Contributor

@mic-max mic-max left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for making this change :)

src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs Outdated Show resolved Hide resolved
src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs Outdated Show resolved Hide resolved
src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs Outdated Show resolved Hide resolved
src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs Outdated Show resolved Hide resolved
src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs Outdated Show resolved Hide resolved
@cijothomas cijothomas merged commit 3aab124 into open-telemetry:main Oct 26, 2021
@pellared pellared deleted the refine-env-vars-parsing branch October 27, 2021 05:16
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.

8 participants