From 210c11025e680666bbbeb9d041bc175d021caf77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 12:14:42 +0200 Subject: [PATCH 01/23] Introduce EnvironmentVariableHelper --- .../Internal/EnvironmentVariableHelper.cs | 62 +++++++++++++++ .../BatchExportActivityProcessorOptions.cs | 39 +--------- .../EnvironmentVariableHelperTests.cs | 78 +++++++++++++++++++ 3 files changed, 144 insertions(+), 35 deletions(-) create mode 100644 src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs create mode 100644 test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs new file mode 100644 index 00000000000..7859900ff07 --- /dev/null +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -0,0 +1,62 @@ +// +// 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. +// + +using System; +using System.Globalization; +using System.Security; + +namespace OpenTelemetry.Internal +{ + internal static class EnvironmentVariableHelper + { + public static bool LoadString(string envVarKey, out string result) + { + result = null; + + try + { + result = Environment.GetEnvironmentVariable(envVarKey); + } + catch (SecurityException ex) + { + // The caller does not have the required permission to + // retrieve the value of an environment variable from the current process. + OpenTelemetrySdkEventSource.Log.MissingPermissionsToReadEnvironmentVariable(ex); + return false; + } + + return result != null; + } + + public static bool LoadNonNegativeInt32(string envVarKey, out int result) + { + result = 0; + + LoadString(envVarKey, out string value); + if (string.IsNullOrEmpty(value)) + { + return false; + } + + if (!int.TryParse(value, NumberStyles.None, CultureInfo.InvariantCulture, out result)) + { + throw new ArgumentException($"{envVarKey} environment variable has an invalid value: '${value}'"); + } + + return true; + } + } +} \ No newline at end of file diff --git a/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs b/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs index 3e4d1ac91ae..50b113d19e2 100644 --- a/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs +++ b/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs @@ -16,7 +16,6 @@ using System; using System.Diagnostics; -using System.Security; using OpenTelemetry.Internal; namespace OpenTelemetry.Trace @@ -44,55 +43,25 @@ public BatchExportActivityProcessorOptions() { int value; - if (LoadEnvVarInt(ExporterTimeoutEnvVarKey, out value)) + if (EnvironmentVariableHelper.LoadNonNegativeInt32(ExporterTimeoutEnvVarKey, out value)) { this.ExporterTimeoutMilliseconds = value; } - if (LoadEnvVarInt(MaxExportBatchSizeEnvVarKey, out value)) + if (EnvironmentVariableHelper.LoadNonNegativeInt32(MaxExportBatchSizeEnvVarKey, out value)) { this.MaxExportBatchSize = value; } - if (LoadEnvVarInt(MaxQueueSizeEnvVarKey, out value)) + if (EnvironmentVariableHelper.LoadNonNegativeInt32(MaxQueueSizeEnvVarKey, out value)) { this.MaxQueueSize = value; } - if (LoadEnvVarInt(ScheduledDelayEnvVarKey, out value)) + if (EnvironmentVariableHelper.LoadNonNegativeInt32(ScheduledDelayEnvVarKey, out value)) { this.ScheduledDelayMilliseconds = value; } } - - private static bool LoadEnvVarInt(string envVarKey, out int result) - { - result = 0; - - string value; - try - { - value = Environment.GetEnvironmentVariable(envVarKey); - } - catch (SecurityException ex) - { - // The caller does not have the required permission to - // retrieve the value of an environment variable from the current process. - OpenTelemetrySdkEventSource.Log.MissingPermissionsToReadEnvironmentVariable(ex); - return false; - } - - if (string.IsNullOrEmpty(value)) - { - return false; - } - - if (!int.TryParse(value, out result)) - { - throw new ArgumentException($"{envVarKey} environment variable has an invalid value: '${value}'"); - } - - return true; - } } } diff --git a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs new file mode 100644 index 00000000000..e6d1e9043b6 --- /dev/null +++ b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs @@ -0,0 +1,78 @@ +// +// 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. +// + +using System; +using Xunit; + +namespace OpenTelemetry.Internal.Tests +{ + public class EnvironmentVariableHelperTests + { + private const string EnvVar = "OTEL_EXAMPLE_VARIABLE"; + + public EnvironmentVariableHelperTests() + { + Environment.SetEnvironmentVariable(EnvVar, null); + } + + public void Dispose() + { + Environment.SetEnvironmentVariable(EnvVar, null); + } + + [Theory] + [InlineData(null, false, null)] + [InlineData("", false, null)] // Environment.SetEnvironmentVariable(EnvVar, ""); clears the environemtal variable as well + [InlineData("something", true, "something")] + public void LoadString(string value, bool expectedBool, string expectedValue) + { + Environment.SetEnvironmentVariable(EnvVar, value); + + bool actualBool = EnvironmentVariableHelper.LoadString(EnvVar, out string actualValue); + + Assert.Equal(expectedBool, actualBool); + Assert.Equal(expectedValue, actualValue); + } + + [Theory] + [InlineData(null, false, 0)] + [InlineData("", false, 0)] + [InlineData("123", true, 123)] + [InlineData("0", true, 0)] + public void LoadNonNegativeInt32(string value, bool expectedBool, int expectedValue) + { + Environment.SetEnvironmentVariable(EnvVar, value); + + bool actualBool = EnvironmentVariableHelper.LoadNonNegativeInt32(EnvVar, out int actualValue); + + Assert.Equal(expectedBool, actualBool); + Assert.Equal(expectedValue, actualValue); + } + + [Theory] + [InlineData("something")] // NaN + [InlineData("-12")] // negative number not allowed + [InlineData("-0")] // sign not allowed + [InlineData(" 123 ")] // whitespaces not allowed + [InlineData("0xFF")] // only decimal number allowed + public void LoadNonNegativeInt32_Invalid(string value) + { + Environment.SetEnvironmentVariable(EnvVar, value); + + Assert.Throws(() => EnvironmentVariableHelper.LoadNonNegativeInt32(EnvVar, out int _)); + } + } +} From 0dbcd713f3206dc784eec1a58164e3580f960c31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 12:22:39 +0200 Subject: [PATCH 02/23] EnvironmentVariableHelperTests implements IDisposable --- .../Internal/EnvironmentVariableHelperTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs index e6d1e9043b6..3081c68ed1d 100644 --- a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs +++ b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs @@ -19,7 +19,7 @@ namespace OpenTelemetry.Internal.Tests { - public class EnvironmentVariableHelperTests + public class EnvironmentVariableHelperTests : IDisposable { private const string EnvVar = "OTEL_EXAMPLE_VARIABLE"; From 6afa561edffc4ba3fc947fa8e776c8f39093fded Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 12:26:26 +0200 Subject: [PATCH 03/23] Reuse EnvironmentVariableHelper in resource detectors --- src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs | 3 +-- src/OpenTelemetry/Resources/OtelServiceNameEnvVarDetector.cs | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs b/src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs index cd424e2d034..c59c443ae96 100644 --- a/src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs +++ b/src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs @@ -14,7 +14,6 @@ // limitations under the License. // -using System; using System.Collections.Generic; using System.Security; using OpenTelemetry.Internal; @@ -33,7 +32,7 @@ public Resource Detect() try { - string envResourceAttributeValue = Environment.GetEnvironmentVariable(EnvVarKey); + EnvironmentVariableHelper.LoadString(EnvVarKey, out string envResourceAttributeValue); if (!string.IsNullOrEmpty(envResourceAttributeValue)) { var attributes = ParseResourceAttributes(envResourceAttributeValue); diff --git a/src/OpenTelemetry/Resources/OtelServiceNameEnvVarDetector.cs b/src/OpenTelemetry/Resources/OtelServiceNameEnvVarDetector.cs index 806ced7c793..5a208a5b9a7 100644 --- a/src/OpenTelemetry/Resources/OtelServiceNameEnvVarDetector.cs +++ b/src/OpenTelemetry/Resources/OtelServiceNameEnvVarDetector.cs @@ -14,7 +14,6 @@ // limitations under the License. // -using System; using System.Collections.Generic; using System.Security; using OpenTelemetry.Internal; @@ -31,7 +30,7 @@ public Resource Detect() try { - string envResourceAttributeValue = Environment.GetEnvironmentVariable(EnvVarKey); + EnvironmentVariableHelper.LoadString(EnvVarKey, out string envResourceAttributeValue); if (!string.IsNullOrEmpty(envResourceAttributeValue)) { resource = new Resource(new Dictionary From edbdf8edc7c4ffeb006dc175299ccad77ce98e69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 12:29:03 +0200 Subject: [PATCH 04/23] Fix final newline --- src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index 7859900ff07..d56f6bceeb3 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -59,4 +59,4 @@ public static bool LoadNonNegativeInt32(string envVarKey, out int result) return true; } } -} \ No newline at end of file +} From 728d26f3c9758fcebe6f90a726f7586e57c43422 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 17:36:32 +0200 Subject: [PATCH 05/23] Interpret empty env var value as it was unset --- src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs | 5 ++--- src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs | 3 +-- src/OpenTelemetry/Resources/OtelServiceNameEnvVarDetector.cs | 3 +-- .../Internal/EnvironmentVariableHelperTests.cs | 2 +- 4 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index d56f6bceeb3..1b1355d874f 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -38,15 +38,14 @@ public static bool LoadString(string envVarKey, out string result) return false; } - return result != null; + return !string.IsNullOrEmpty(result); } public static bool LoadNonNegativeInt32(string envVarKey, out int result) { result = 0; - LoadString(envVarKey, out string value); - if (string.IsNullOrEmpty(value)) + if (!LoadString(envVarKey, out string value)) { return false; } diff --git a/src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs b/src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs index c59c443ae96..a84e23cf721 100644 --- a/src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs +++ b/src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs @@ -32,8 +32,7 @@ public Resource Detect() try { - EnvironmentVariableHelper.LoadString(EnvVarKey, out string envResourceAttributeValue); - if (!string.IsNullOrEmpty(envResourceAttributeValue)) + if (EnvironmentVariableHelper.LoadString(EnvVarKey, out string envResourceAttributeValue)) { var attributes = ParseResourceAttributes(envResourceAttributeValue); resource = new Resource(attributes); diff --git a/src/OpenTelemetry/Resources/OtelServiceNameEnvVarDetector.cs b/src/OpenTelemetry/Resources/OtelServiceNameEnvVarDetector.cs index 5a208a5b9a7..ff05610697a 100644 --- a/src/OpenTelemetry/Resources/OtelServiceNameEnvVarDetector.cs +++ b/src/OpenTelemetry/Resources/OtelServiceNameEnvVarDetector.cs @@ -30,8 +30,7 @@ public Resource Detect() try { - EnvironmentVariableHelper.LoadString(EnvVarKey, out string envResourceAttributeValue); - if (!string.IsNullOrEmpty(envResourceAttributeValue)) + if (EnvironmentVariableHelper.LoadString(EnvVarKey, out string envResourceAttributeValue)) { resource = new Resource(new Dictionary { diff --git a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs index 3081c68ed1d..e088fd2cca8 100644 --- a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs +++ b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs @@ -49,7 +49,7 @@ public void LoadString(string value, bool expectedBool, string expectedValue) [Theory] [InlineData(null, false, 0)] - [InlineData("", false, 0)] + [InlineData("", false, 0)] // Environment.SetEnvironmentVariable(EnvVar, ""); clears the environemtal variable as well [InlineData("123", true, 123)] [InlineData("0", true, 0)] public void LoadNonNegativeInt32(string value, bool expectedBool, int expectedValue) From 53f3c31bd7db0a1e9d4a4519ef17d60151e9c3b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 17:43:05 +0200 Subject: [PATCH 06/23] Throw FormatException instead of AgumentException --- src/OpenTelemetry/CHANGELOG.md | 2 +- src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs | 2 +- src/OpenTelemetry/README.md | 2 +- src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs | 2 +- .../Internal/EnvironmentVariableHelperTests.cs | 2 +- .../Trace/BatchExportActivityProcessorOptionsTest.cs | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 75b3c1c4d6d..6d7c47f2c14 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -* Changed `BatchExportActivityProcessorOptions` to throw `ArgumentException` +* Changed `BatchExportActivityProcessorOptions` to throw `FormatException` if it fails to parse any of the supported environment variables. ## 1.2.0-beta1 diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index 1b1355d874f..4f8e322a0dd 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -52,7 +52,7 @@ public static bool LoadNonNegativeInt32(string envVarKey, out int result) if (!int.TryParse(value, NumberStyles.None, CultureInfo.InvariantCulture, out result)) { - throw new ArgumentException($"{envVarKey} environment variable has an invalid value: '${value}'"); + throw new FormatException($"{envVarKey} environment variable has an invalid value: '${value}'"); } return true; diff --git a/src/OpenTelemetry/README.md b/src/OpenTelemetry/README.md index 4c8f087a0a3..03d8b75da02 100644 --- a/src/OpenTelemetry/README.md +++ b/src/OpenTelemetry/README.md @@ -215,7 +215,7 @@ purposes, the SDK provides the following built-in processors: | `OTEL_BSP_MAX_EXPORT_BATCH_SIZE` | `MaxExportBatchSizeEnvVarKey` | - `ArgumentException` is thrown in case of an invalid value for any of the + `FormatException` is thrown in case of an invalid value for any of the supported environment variables. * [CompositeProcessor<T>](../../src/OpenTelemetry/CompositeProcessor.cs) diff --git a/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs b/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs index 50b113d19e2..bcbf7f6a7cd 100644 --- a/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs +++ b/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs @@ -26,7 +26,7 @@ namespace OpenTelemetry.Trace /// environment variables are parsed during object construction. /// /// - /// The constructor throws if it fails to parse + /// The constructor throws if it fails to parse /// any of the supported environment variables. /// public class BatchExportActivityProcessorOptions : BatchExportProcessorOptions diff --git a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs index e088fd2cca8..ed73aad606b 100644 --- a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs +++ b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs @@ -72,7 +72,7 @@ public void LoadNonNegativeInt32_Invalid(string value) { Environment.SetEnvironmentVariable(EnvVar, value); - Assert.Throws(() => EnvironmentVariableHelper.LoadNonNegativeInt32(EnvVar, out int _)); + Assert.Throws(() => EnvironmentVariableHelper.LoadNonNegativeInt32(EnvVar, out int _)); } } } diff --git a/test/OpenTelemetry.Tests/Trace/BatchExportActivityProcessorOptionsTest.cs b/test/OpenTelemetry.Tests/Trace/BatchExportActivityProcessorOptionsTest.cs index cd62ae57c7e..dc834b51d2d 100644 --- a/test/OpenTelemetry.Tests/Trace/BatchExportActivityProcessorOptionsTest.cs +++ b/test/OpenTelemetry.Tests/Trace/BatchExportActivityProcessorOptionsTest.cs @@ -63,7 +63,7 @@ public void BatchExportProcessorOptions_InvalidPortEnvironmentVariableOverride() { Environment.SetEnvironmentVariable(BatchExportActivityProcessorOptions.ExporterTimeoutEnvVarKey, "invalid"); - Assert.Throws(() => new BatchExportActivityProcessorOptions()); + Assert.Throws(() => new BatchExportActivityProcessorOptions()); } [Fact] From d219e1308590423d65cd5e2c9fdadca21888833a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 18:09:05 +0200 Subject: [PATCH 07/23] Reuse EnvironmentVariableHelper in Jaeger exporter --- .../JaegerExporterEventSource.cs | 22 ------------- .../JaegerExporterOptions.cs | 32 ++++--------------- .../OpenTelemetry.Exporter.Jaeger.csproj | 2 ++ .../JaegerExporterOptionsTests.cs | 4 +-- 4 files changed, 9 insertions(+), 51 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerExporterEventSource.cs b/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerExporterEventSource.cs index 45ec68f326a..747982f555b 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerExporterEventSource.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerExporterEventSource.cs @@ -16,7 +16,6 @@ using System; using System.Diagnostics.Tracing; -using System.Security; using OpenTelemetry.Internal; namespace OpenTelemetry.Exporter.Jaeger.Implementation @@ -38,31 +37,10 @@ public void FailedExport(Exception ex) } } - [NonEvent] - public void MissingPermissionsToReadEnvironmentVariable(SecurityException ex) - { - if (this.IsEnabled(EventLevel.Warning, EventKeywords.All)) - { - this.MissingPermissionsToReadEnvironmentVariable(ex.ToInvariantString()); - } - } - [Event(1, Message = "Failed to send spans: '{0}'", Level = EventLevel.Error)] public void FailedExport(string exception) { this.WriteEvent(1, exception); } - - [Event(2, Message = "Failed to parse environment variable: '{0}', value: '{1}'.", Level = EventLevel.Warning)] - public void FailedToParseEnvironmentVariable(string name, string value) - { - this.WriteEvent(2, name, value); - } - - [Event(3, Message = "Missing permissions to read environment variable: '{0}'", Level = EventLevel.Warning)] - public void MissingPermissionsToReadEnvironmentVariable(string exception) - { - this.WriteEvent(3, exception); - } } } diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs index b578beeb41c..b77dc2b9f3f 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs @@ -14,10 +14,8 @@ // limitations under the License. // -using System; using System.Diagnostics; -using System.Security; -using OpenTelemetry.Exporter.Jaeger.Implementation; +using OpenTelemetry.Internal; using OpenTelemetry.Trace; namespace OpenTelemetry.Exporter @@ -31,32 +29,14 @@ public class JaegerExporterOptions public JaegerExporterOptions() { - try + if (EnvironmentVariableHelper.LoadString(OTelAgentHostEnvVarKey, out string agentHostEnvVar)) { - string agentHostEnvVar = Environment.GetEnvironmentVariable(OTelAgentHostEnvVarKey); - if (!string.IsNullOrEmpty(agentHostEnvVar)) - { - this.AgentHost = agentHostEnvVar; - } - - string agentPortEnvVar = Environment.GetEnvironmentVariable(OTelAgentPortEnvVarKey); - if (!string.IsNullOrEmpty(agentPortEnvVar)) - { - if (int.TryParse(agentPortEnvVar, out var agentPortValue)) - { - this.AgentPort = agentPortValue; - } - else - { - JaegerExporterEventSource.Log.FailedToParseEnvironmentVariable(OTelAgentPortEnvVarKey, agentPortEnvVar); - } - } + this.AgentHost = agentHostEnvVar; } - catch (SecurityException ex) + + if (EnvironmentVariableHelper.LoadNonNegativeInt32(OTelAgentPortEnvVarKey, out int agentPortEnvVar)) { - // The caller does not have the required permission to - // retrieve the value of an environment variable from the current process. - JaegerExporterEventSource.Log.MissingPermissionsToReadEnvironmentVariable(ex); + this.AgentPort = agentPortEnvVar; } } diff --git a/src/OpenTelemetry.Exporter.Jaeger/OpenTelemetry.Exporter.Jaeger.csproj b/src/OpenTelemetry.Exporter.Jaeger/OpenTelemetry.Exporter.Jaeger.csproj index 1b264bc21fd..072a7c98259 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/OpenTelemetry.Exporter.Jaeger.csproj +++ b/src/OpenTelemetry.Exporter.Jaeger/OpenTelemetry.Exporter.Jaeger.csproj @@ -25,6 +25,8 @@ + + diff --git a/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs b/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs index 4716f975a2f..2aad340124e 100644 --- a/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs +++ b/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs @@ -59,9 +59,7 @@ public void JaegerExporterOptions_InvalidPortEnvironmentVariableOverride() { Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelAgentPortEnvVarKey, "invalid"); - var options = new JaegerExporterOptions(); - - Assert.Equal(6831, options.AgentPort); // use default + Assert.Throws(() => new JaegerExporterOptions()); } [Fact] From c9a93ec5af2fccb142caaa9bdd29299b7bc27188 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 18:22:01 +0200 Subject: [PATCH 08/23] Fix Benchmarks build --- test/Benchmarks/Benchmarks.csproj | 2 +- test/Benchmarks/Exporter/JaegerExporterBenchmarks.cs | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/test/Benchmarks/Benchmarks.csproj b/test/Benchmarks/Benchmarks.csproj index 3dc85369b1b..3b4997d56a5 100644 --- a/test/Benchmarks/Benchmarks.csproj +++ b/test/Benchmarks/Benchmarks.csproj @@ -17,7 +17,7 @@ - + diff --git a/test/Benchmarks/Exporter/JaegerExporterBenchmarks.cs b/test/Benchmarks/Exporter/JaegerExporterBenchmarks.cs index cf452c2cb74..d8312ede3c5 100644 --- a/test/Benchmarks/Exporter/JaegerExporterBenchmarks.cs +++ b/test/Benchmarks/Exporter/JaegerExporterBenchmarks.cs @@ -14,13 +14,15 @@ // limitations under the License. // +extern alias Jaeger; + using System.Diagnostics; using BenchmarkDotNet.Attributes; using Benchmarks.Helper; +using Jaeger::OpenTelemetry.Exporter; +using Jaeger::Thrift.Transport; using OpenTelemetry; -using OpenTelemetry.Exporter; using OpenTelemetry.Internal; -using Thrift.Transport; namespace Benchmarks.Exporter { @@ -50,7 +52,7 @@ public void JaegerExporter_Batching() new JaegerExporterOptions(), new BlackHoleTransport()) { - Process = new OpenTelemetry.Exporter.Jaeger.Implementation.Process("TestService"), + Process = new Jaeger::OpenTelemetry.Exporter.Jaeger.Implementation.Process("TestService"), }; for (int i = 0; i < this.NumberOfBatches; i++) From c78bb2ad943e73f45e1b523c5d46cc88a138974e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 18:44:50 +0200 Subject: [PATCH 09/23] Reuse EnvironmentVariableHelper in OTLP exporter --- .../JaegerExporterOptions.cs | 2 +- ...penTelemetryProtocolExporterEventSource.cs | 37 ---------- ...etry.Exporter.OpenTelemetryProtocol.csproj | 2 + .../OtlpExporterOptions.cs | 69 +++++++------------ .../Internal/EnvironmentVariableHelper.cs | 2 +- .../BatchExportActivityProcessorOptions.cs | 8 +-- test/Benchmarks/Benchmarks.csproj | 2 +- .../Exporter/OtlpGrpcExporterBenchmarks.cs | 8 ++- .../Exporter/OtlpHttpExporterBenchmarks.cs | 6 +- .../OtlpExporterOptionsTests.cs | 12 +--- .../EnvironmentVariableHelperTests.cs | 8 +-- 11 files changed, 49 insertions(+), 107 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs index b77dc2b9f3f..54f166d09be 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs @@ -34,7 +34,7 @@ public JaegerExporterOptions() this.AgentHost = agentHostEnvVar; } - if (EnvironmentVariableHelper.LoadNonNegativeInt32(OTelAgentPortEnvVarKey, out int agentPortEnvVar)) + if (EnvironmentVariableHelper.LoadNumeric(OTelAgentPortEnvVarKey, out int agentPortEnvVar)) { this.AgentPort = agentPortEnvVar; } diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OpenTelemetryProtocolExporterEventSource.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OpenTelemetryProtocolExporterEventSource.cs index 8aecdf51f07..d42e9937832 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OpenTelemetryProtocolExporterEventSource.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OpenTelemetryProtocolExporterEventSource.cs @@ -16,7 +16,6 @@ using System; using System.Diagnostics.Tracing; -using System.Security; using OpenTelemetry.Internal; namespace OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation @@ -26,24 +25,6 @@ internal class OpenTelemetryProtocolExporterEventSource : EventSource { public static readonly OpenTelemetryProtocolExporterEventSource Log = new OpenTelemetryProtocolExporterEventSource(); - [NonEvent] - public void MissingPermissionsToReadEnvironmentVariable(SecurityException ex) - { - if (this.IsEnabled(EventLevel.Warning, EventKeywords.All)) - { - this.MissingPermissionsToReadEnvironmentVariable(ex.ToInvariantString()); - } - } - - [NonEvent] - public void FailedToConvertToProtoDefinitionError(Exception ex) - { - if (Log.IsEnabled(EventLevel.Error, EventKeywords.All)) - { - this.FailedToConvertToProtoDefinitionError(ex.ToInvariantString()); - } - } - [NonEvent] public void FailedToReachCollector(Exception ex) { @@ -62,12 +43,6 @@ public void ExportMethodException(Exception ex) } } - [Event(1, Message = "Exporter failed to convert SpanData content into gRPC proto definition. Data will not be sent. Exception: {0}", Level = EventLevel.Error)] - public void FailedToConvertToProtoDefinitionError(string ex) - { - this.WriteEvent(1, ex); - } - [Event(2, Message = "Exporter failed send data to collector. Data will not be sent. Exception: {0}", Level = EventLevel.Error)] public void FailedToReachCollector(string ex) { @@ -92,18 +67,6 @@ public void CouldNotTranslateMetric(string className, string methodName) this.WriteEvent(5, className, methodName); } - [Event(6, Message = "Failed to parse environment variable: '{0}', value: '{1}'.", Level = EventLevel.Warning)] - public void FailedToParseEnvironmentVariable(string name, string value) - { - this.WriteEvent(6, name, value); - } - - [Event(7, Message = "Missing permissions to read environment variable: '{0}'", Level = EventLevel.Warning)] - public void MissingPermissionsToReadEnvironmentVariable(string exception) - { - this.WriteEvent(7, exception); - } - [Event(8, Message = "Unsupported value for protocol '{0}' is configured, default protocol 'grpc' will be used.", Level = EventLevel.Warning)] public void UnsupportedProtocol(string protocol) { diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj index fa9ece21ad7..c6c6c3812ee 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj @@ -42,6 +42,8 @@ + + diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs index 1c1265bbd35..5853d6299f4 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs @@ -16,8 +16,7 @@ using System; using System.Diagnostics; -using System.Security; -using OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation; +using OpenTelemetry.Internal; using OpenTelemetry.Metrics; using OpenTelemetry.Trace; @@ -41,60 +40,40 @@ public class OtlpExporterOptions /// public OtlpExporterOptions() { - try + if (EnvironmentVariableHelper.LoadString(EndpointEnvVarName, out string endpointEnvVar)) { - string endpointEnvVar = Environment.GetEnvironmentVariable(EndpointEnvVarName); - if (!string.IsNullOrEmpty(endpointEnvVar)) + if (Uri.TryCreate(endpointEnvVar, UriKind.Absolute, out var endpoint)) { - if (Uri.TryCreate(endpointEnvVar, UriKind.Absolute, out var endpoint)) - { - this.Endpoint = endpoint; - } - else - { - OpenTelemetryProtocolExporterEventSource.Log.FailedToParseEnvironmentVariable(EndpointEnvVarName, endpointEnvVar); - } + this.Endpoint = endpoint; } - - string headersEnvVar = Environment.GetEnvironmentVariable(HeadersEnvVarName); - if (!string.IsNullOrEmpty(headersEnvVar)) + else { - this.Headers = headersEnvVar; + throw new FormatException($"{EndpointEnvVarName} environment variable has an invalid value: '${endpointEnvVar}'"); } + } + + if (EnvironmentVariableHelper.LoadString(HeadersEnvVarName, out string headersEnvVar)) + { + this.Headers = headersEnvVar; + } + + if (EnvironmentVariableHelper.LoadNumeric(TimeoutEnvVarName, out int timeout)) + { + this.TimeoutMilliseconds = timeout; + } - string timeoutEnvVar = Environment.GetEnvironmentVariable(TimeoutEnvVarName); - if (!string.IsNullOrEmpty(timeoutEnvVar)) + if (EnvironmentVariableHelper.LoadString(ProtocolEnvVarName, out string protocolEnvVar)) + { + var protocol = protocolEnvVar.ToOtlpExportProtocol(); + if (protocol.HasValue) { - if (int.TryParse(timeoutEnvVar, out var timeout)) - { - this.TimeoutMilliseconds = timeout; - } - else - { - OpenTelemetryProtocolExporterEventSource.Log.FailedToParseEnvironmentVariable(TimeoutEnvVarName, timeoutEnvVar); - } + this.Protocol = protocol.Value; } - - string protocolEnvVar = Environment.GetEnvironmentVariable(ProtocolEnvVarName); - if (!string.IsNullOrEmpty(protocolEnvVar)) + else { - var protocol = protocolEnvVar.ToOtlpExportProtocol(); - if (protocol.HasValue) - { - this.Protocol = protocol.Value; - } - else - { - OpenTelemetryProtocolExporterEventSource.Log.UnsupportedProtocol(protocolEnvVar); - } + throw new FormatException($"{ProtocolEnvVarName} environment variable has an invalid value: '${protocolEnvVar}'"); } } - catch (SecurityException ex) - { - // The caller does not have the required permission to - // retrieve the value of an environment variable from the current process. - OpenTelemetryProtocolExporterEventSource.Log.MissingPermissionsToReadEnvironmentVariable(ex); - } } /// diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index 4f8e322a0dd..511fbc0b484 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -41,7 +41,7 @@ public static bool LoadString(string envVarKey, out string result) return !string.IsNullOrEmpty(result); } - public static bool LoadNonNegativeInt32(string envVarKey, out int result) + public static bool LoadNumeric(string envVarKey, out int result) { result = 0; diff --git a/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs b/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs index bcbf7f6a7cd..5b78df251cb 100644 --- a/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs +++ b/src/OpenTelemetry/Trace/BatchExportActivityProcessorOptions.cs @@ -43,22 +43,22 @@ public BatchExportActivityProcessorOptions() { int value; - if (EnvironmentVariableHelper.LoadNonNegativeInt32(ExporterTimeoutEnvVarKey, out value)) + if (EnvironmentVariableHelper.LoadNumeric(ExporterTimeoutEnvVarKey, out value)) { this.ExporterTimeoutMilliseconds = value; } - if (EnvironmentVariableHelper.LoadNonNegativeInt32(MaxExportBatchSizeEnvVarKey, out value)) + if (EnvironmentVariableHelper.LoadNumeric(MaxExportBatchSizeEnvVarKey, out value)) { this.MaxExportBatchSize = value; } - if (EnvironmentVariableHelper.LoadNonNegativeInt32(MaxQueueSizeEnvVarKey, out value)) + if (EnvironmentVariableHelper.LoadNumeric(MaxQueueSizeEnvVarKey, out value)) { this.MaxQueueSize = value; } - if (EnvironmentVariableHelper.LoadNonNegativeInt32(ScheduledDelayEnvVarKey, out value)) + if (EnvironmentVariableHelper.LoadNumeric(ScheduledDelayEnvVarKey, out value)) { this.ScheduledDelayMilliseconds = value; } diff --git a/test/Benchmarks/Benchmarks.csproj b/test/Benchmarks/Benchmarks.csproj index 3b4997d56a5..92417929cae 100644 --- a/test/Benchmarks/Benchmarks.csproj +++ b/test/Benchmarks/Benchmarks.csproj @@ -18,7 +18,7 @@ - + diff --git a/test/Benchmarks/Exporter/OtlpGrpcExporterBenchmarks.cs b/test/Benchmarks/Exporter/OtlpGrpcExporterBenchmarks.cs index 2fb7678432f..f30469e31e7 100644 --- a/test/Benchmarks/Exporter/OtlpGrpcExporterBenchmarks.cs +++ b/test/Benchmarks/Exporter/OtlpGrpcExporterBenchmarks.cs @@ -14,6 +14,8 @@ // limitations under the License. // +extern alias OpenTelemetryProtocol; + using System; using System.Diagnostics; using System.Threading; @@ -21,10 +23,10 @@ using Benchmarks.Helper; using Grpc.Core; using OpenTelemetry; -using OpenTelemetry.Exporter; -using OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient; using OpenTelemetry.Internal; -using OtlpCollector = Opentelemetry.Proto.Collector.Trace.V1; +using OpenTelemetryProtocol::OpenTelemetry.Exporter; +using OpenTelemetryProtocol::OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient; +using OtlpCollector = OpenTelemetryProtocol::Opentelemetry.Proto.Collector.Trace.V1; namespace Benchmarks.Exporter { diff --git a/test/Benchmarks/Exporter/OtlpHttpExporterBenchmarks.cs b/test/Benchmarks/Exporter/OtlpHttpExporterBenchmarks.cs index de4477f5b52..5b24ce1974c 100644 --- a/test/Benchmarks/Exporter/OtlpHttpExporterBenchmarks.cs +++ b/test/Benchmarks/Exporter/OtlpHttpExporterBenchmarks.cs @@ -14,16 +14,18 @@ // limitations under the License. // +extern alias OpenTelemetryProtocol; + using System; using System.Diagnostics; using System.IO; using BenchmarkDotNet.Attributes; using Benchmarks.Helper; using OpenTelemetry; -using OpenTelemetry.Exporter; -using OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient; using OpenTelemetry.Internal; using OpenTelemetry.Tests; +using OpenTelemetryProtocol::OpenTelemetry.Exporter; +using OpenTelemetryProtocol::OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient; namespace Benchmarks.Exporter { diff --git a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterOptionsTests.cs b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterOptionsTests.cs index 4a1c1186110..2b7a612fa06 100644 --- a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterOptionsTests.cs +++ b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterOptionsTests.cs @@ -63,9 +63,7 @@ public void OtlpExporterOptions_InvalidEndpointVariableOverride() { Environment.SetEnvironmentVariable(OtlpExporterOptions.EndpointEnvVarName, "invalid"); - var options = new OtlpExporterOptions(); - - Assert.Equal(new Uri("http://localhost:4317"), options.Endpoint); // use default + Assert.Throws(() => new OtlpExporterOptions()); } [Fact] @@ -73,9 +71,7 @@ public void OtlpExporterOptions_InvalidTimeoutVariableOverride() { Environment.SetEnvironmentVariable(OtlpExporterOptions.TimeoutEnvVarName, "invalid"); - var options = new OtlpExporterOptions(); - - Assert.Equal(10000, options.TimeoutMilliseconds); // use default + Assert.Throws(() => new OtlpExporterOptions()); } [Fact] @@ -83,9 +79,7 @@ public void OtlpExporterOptions_InvalidProtocolVariableOverride() { Environment.SetEnvironmentVariable(OtlpExporterOptions.ProtocolEnvVarName, "invalid"); - var options = new OtlpExporterOptions(); - - Assert.Equal(OtlpExportProtocol.Grpc, options.Protocol); // use default + Assert.Throws(() => new OtlpExporterOptions()); } [Fact] diff --git a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs index ed73aad606b..7cfce48465a 100644 --- a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs +++ b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs @@ -52,11 +52,11 @@ public void LoadString(string value, bool expectedBool, string expectedValue) [InlineData("", false, 0)] // Environment.SetEnvironmentVariable(EnvVar, ""); clears the environemtal variable as well [InlineData("123", true, 123)] [InlineData("0", true, 0)] - public void LoadNonNegativeInt32(string value, bool expectedBool, int expectedValue) + public void LoadNumeric(string value, bool expectedBool, int expectedValue) { Environment.SetEnvironmentVariable(EnvVar, value); - bool actualBool = EnvironmentVariableHelper.LoadNonNegativeInt32(EnvVar, out int actualValue); + bool actualBool = EnvironmentVariableHelper.LoadNumeric(EnvVar, out int actualValue); Assert.Equal(expectedBool, actualBool); Assert.Equal(expectedValue, actualValue); @@ -68,11 +68,11 @@ public void LoadNonNegativeInt32(string value, bool expectedBool, int expectedVa [InlineData("-0")] // sign not allowed [InlineData(" 123 ")] // whitespaces not allowed [InlineData("0xFF")] // only decimal number allowed - public void LoadNonNegativeInt32_Invalid(string value) + public void LoadNumeric_Invalid(string value) { Environment.SetEnvironmentVariable(EnvVar, value); - Assert.Throws(() => EnvironmentVariableHelper.LoadNonNegativeInt32(EnvVar, out int _)); + Assert.Throws(() => EnvironmentVariableHelper.LoadNumeric(EnvVar, out int _)); } } } From d3761628379a2f51bb3a77b0b1e3d715c39330fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 19:18:31 +0200 Subject: [PATCH 10/23] Add EnvironmentVariableHelper.LoadUri --- .../OtlpExporterOptions.cs | 11 +-- .../Internal/EnvironmentVariableHelper.cs | 17 +++++ .../EnvironmentVariableHelperTests.cs | 73 +++++++++++++++---- 3 files changed, 79 insertions(+), 22 deletions(-) diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs index 5853d6299f4..4903180c487 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs @@ -40,16 +40,9 @@ public class OtlpExporterOptions /// public OtlpExporterOptions() { - if (EnvironmentVariableHelper.LoadString(EndpointEnvVarName, out string endpointEnvVar)) + if (EnvironmentVariableHelper.LoadUri(EndpointEnvVarName, out Uri endpoint)) { - if (Uri.TryCreate(endpointEnvVar, UriKind.Absolute, out var endpoint)) - { - this.Endpoint = endpoint; - } - else - { - throw new FormatException($"{EndpointEnvVarName} environment variable has an invalid value: '${endpointEnvVar}'"); - } + this.Endpoint = endpoint; } if (EnvironmentVariableHelper.LoadString(HeadersEnvVarName, out string headersEnvVar)) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index 511fbc0b484..66579ad9b21 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -57,5 +57,22 @@ public static bool LoadNumeric(string envVarKey, out int result) return true; } + + public static bool LoadUri(string envVarKey, out Uri result) + { + result = null; + + if (!LoadString(envVarKey, out string value)) + { + return false; + } + + if (!Uri.TryCreate(value, UriKind.Absolute, out result)) + { + throw new FormatException($"{envVarKey} environment variable has an invalid value: '${value}'"); + } + + return true; + } } } diff --git a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs index 7cfce48465a..9cc42956108 100644 --- a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs +++ b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs @@ -33,35 +33,49 @@ public void Dispose() Environment.SetEnvironmentVariable(EnvVar, null); } - [Theory] - [InlineData(null, false, null)] - [InlineData("", false, null)] // Environment.SetEnvironmentVariable(EnvVar, ""); clears the environemtal variable as well - [InlineData("something", true, "something")] - public void LoadString(string value, bool expectedBool, string expectedValue) + [Fact] + public void LoadString() { + const string value = "something"; Environment.SetEnvironmentVariable(EnvVar, value); bool actualBool = EnvironmentVariableHelper.LoadString(EnvVar, out string actualValue); - Assert.Equal(expectedBool, actualBool); - Assert.Equal(expectedValue, actualValue); + Assert.True(actualBool); + Assert.Equal(value, actualValue); + } + + [Fact] + public void LoadString_NoValue() + { + bool actualBool = EnvironmentVariableHelper.LoadString(EnvVar, out string actualValue); + + Assert.False(actualBool); + Assert.Null(actualValue); } [Theory] - [InlineData(null, false, 0)] - [InlineData("", false, 0)] // Environment.SetEnvironmentVariable(EnvVar, ""); clears the environemtal variable as well - [InlineData("123", true, 123)] - [InlineData("0", true, 0)] - public void LoadNumeric(string value, bool expectedBool, int expectedValue) + [InlineData("123", 123)] + [InlineData("0", 0)] + public void LoadNumeric(string value, int expectedValue) { Environment.SetEnvironmentVariable(EnvVar, value); bool actualBool = EnvironmentVariableHelper.LoadNumeric(EnvVar, out int actualValue); - Assert.Equal(expectedBool, actualBool); + Assert.True(actualBool); Assert.Equal(expectedValue, actualValue); } + [Fact] + public void LoadNumeric_NoValue() + { + bool actualBool = EnvironmentVariableHelper.LoadNumeric(EnvVar, out int actualValue); + + Assert.False(actualBool); + Assert.Equal(0, actualValue); + } + [Theory] [InlineData("something")] // NaN [InlineData("-12")] // negative number not allowed @@ -74,5 +88,38 @@ public void LoadNumeric_Invalid(string value) Assert.Throws(() => EnvironmentVariableHelper.LoadNumeric(EnvVar, out int _)); } + + [Theory] + [InlineData("http://www.example.com", "http://www.example.com/")] + [InlineData("http://www.example.com/space%20here.html", "http://www.example.com/space here.html")] // characters are converted + [InlineData("http://www.example.com/space here.html", "http://www.example.com/space here.html")] // characters are escaped + public void LoadUri(string value, string expectedValue) + { + Environment.SetEnvironmentVariable(EnvVar, value); + + bool actualBool = EnvironmentVariableHelper.LoadUri(EnvVar, out Uri actualValue); + + Assert.True(actualBool); + Assert.Equal(expectedValue, actualValue.ToString()); + } + + [Fact] + public void LoadUri_NoValue() + { + bool actualBool = EnvironmentVariableHelper.LoadUri(EnvVar, out Uri actualValue); + + Assert.False(actualBool); + Assert.Null(actualValue); + } + + [Theory] + [InlineData("invalid")] // invalid format + [InlineData(" ")] // whitespace + public void LoadUri_Invalid(string value) + { + Environment.SetEnvironmentVariable(EnvVar, value); + + Assert.Throws(() => EnvironmentVariableHelper.LoadUri(EnvVar, out Uri _)); + } } } From afb1e7b6573a9abb1c84c3375acbc11a8ddf31ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 19:50:24 +0200 Subject: [PATCH 11/23] Reuse EnvironmentVariableHelper in Zipkin exporter --- .../Implementation/ZipkinExporterEventSource.cs | 15 --------------- .../OpenTelemetry.Exporter.Zipkin.csproj | 2 ++ .../ZipkinExporterOptions.cs | 13 ++++--------- test/Benchmarks/Benchmarks.csproj | 2 +- .../Exporter/ZipkinExporterBenchmarks.cs | 4 +++- .../ZipkinExporterTests.cs | 4 +--- 6 files changed, 11 insertions(+), 29 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinExporterEventSource.cs b/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinExporterEventSource.cs index de9d6856fe1..f2d1bd94f40 100644 --- a/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinExporterEventSource.cs +++ b/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinExporterEventSource.cs @@ -37,25 +37,10 @@ public void FailedExport(Exception ex) } } - [NonEvent] - public void FailedEndpointInitialization(Exception ex) - { - if (this.IsEnabled(EventLevel.Error, EventKeywords.All)) - { - this.FailedEndpointInitialization(ex.ToInvariantString()); - } - } - [Event(1, Message = "Failed to export activities: '{0}'", Level = EventLevel.Error)] public void FailedExport(string exception) { this.WriteEvent(1, exception); } - - [Event(2, Message = "Error initializing Zipkin endpoint, falling back to default value: '{0}'", Level = EventLevel.Error)] - public void FailedEndpointInitialization(string exception) - { - this.WriteEvent(2, exception); - } } } diff --git a/src/OpenTelemetry.Exporter.Zipkin/OpenTelemetry.Exporter.Zipkin.csproj b/src/OpenTelemetry.Exporter.Zipkin/OpenTelemetry.Exporter.Zipkin.csproj index 3555bd8722f..d37cc1a357e 100644 --- a/src/OpenTelemetry.Exporter.Zipkin/OpenTelemetry.Exporter.Zipkin.csproj +++ b/src/OpenTelemetry.Exporter.Zipkin/OpenTelemetry.Exporter.Zipkin.csproj @@ -16,6 +16,8 @@ + + diff --git a/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterOptions.cs b/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterOptions.cs index a2af6f2a776..35631e0287d 100644 --- a/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterOptions.cs +++ b/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterOptions.cs @@ -16,7 +16,7 @@ using System; using System.Diagnostics; -using OpenTelemetry.Exporter.Zipkin.Implementation; +using OpenTelemetry.Internal; using OpenTelemetry.Trace; namespace OpenTelemetry.Exporter @@ -36,14 +36,9 @@ public sealed class ZipkinExporterOptions /// public ZipkinExporterOptions() { - try + if (EnvironmentVariableHelper.LoadUri(ZipkinEndpointEnvVar, out Uri endpoint)) { - this.Endpoint = new Uri(Environment.GetEnvironmentVariable(ZipkinEndpointEnvVar) ?? DefaultZipkinEndpoint); - } - catch (Exception ex) - { - this.Endpoint = new Uri(DefaultZipkinEndpoint); - ZipkinExporterEventSource.Log.FailedEndpointInitialization(ex); + this.Endpoint = endpoint; } } @@ -51,7 +46,7 @@ public ZipkinExporterOptions() /// Gets or sets Zipkin endpoint address. See https://zipkin.io/zipkin-api/#/default/post_spans. /// Typically https://zipkin-server-name:9411/api/v2/spans. /// - public Uri Endpoint { get; set; } + public Uri Endpoint { get; set; } = new Uri(DefaultZipkinEndpoint); /// /// Gets or sets a value indicating whether short trace id should be used. diff --git a/test/Benchmarks/Benchmarks.csproj b/test/Benchmarks/Benchmarks.csproj index 92417929cae..d9302f52f9b 100644 --- a/test/Benchmarks/Benchmarks.csproj +++ b/test/Benchmarks/Benchmarks.csproj @@ -20,7 +20,7 @@ - + diff --git a/test/Benchmarks/Exporter/ZipkinExporterBenchmarks.cs b/test/Benchmarks/Exporter/ZipkinExporterBenchmarks.cs index 9d86b1522d2..22612497395 100644 --- a/test/Benchmarks/Exporter/ZipkinExporterBenchmarks.cs +++ b/test/Benchmarks/Exporter/ZipkinExporterBenchmarks.cs @@ -14,15 +14,17 @@ // limitations under the License. // +extern alias Zipkin; + using System; using System.Diagnostics; using System.IO; using BenchmarkDotNet.Attributes; using Benchmarks.Helper; using OpenTelemetry; -using OpenTelemetry.Exporter; using OpenTelemetry.Internal; using OpenTelemetry.Tests; +using Zipkin::OpenTelemetry.Exporter; namespace Benchmarks.Exporter { diff --git a/test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs b/test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs index 8b4a6482b65..f3d766ad04b 100644 --- a/test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs +++ b/test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs @@ -175,9 +175,7 @@ public void ErrorGettingUriFromEnvVarSetsDefaultEndpointValue() { Environment.SetEnvironmentVariable(ZipkinExporterOptions.ZipkinEndpointEnvVar, "InvalidUri"); - var exporterOptions = new ZipkinExporterOptions(); - - Assert.Equal(new Uri(ZipkinExporterOptions.DefaultZipkinEndpoint).AbsoluteUri, exporterOptions.Endpoint.AbsoluteUri); + Assert.Throws(() => new ZipkinExporterOptions()); } finally { From a60a5ac9fe2fc66046709ec8c50e0840910319df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 20:07:49 +0200 Subject: [PATCH 12/23] Add doc comments to ExporterOptions --- .../JaegerExporterOptions.cs | 10 ++++++++++ .../OtlpExporterOptions.cs | 8 +++++++- .../ZipkinExporterOptions.cs | 8 +++++++- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs index 54f166d09be..bbe8f457d53 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs @@ -14,12 +14,22 @@ // limitations under the License. // +using System; using System.Diagnostics; using OpenTelemetry.Internal; using OpenTelemetry.Trace; namespace OpenTelemetry.Exporter { + /// + /// Jaeger exporter options. + /// OTEL_EXPORTER_JAEGER_AGENT_HOST, OTEL_EXPORTER_JAEGER_AGENT_PORT + /// environment variables are parsed during object construction. + /// + /// + /// The constructor throws if it fails to parse + /// any of the supported environment variables. + /// public class JaegerExporterOptions { internal const int DefaultMaxPayloadSizeInBytes = 4096; diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs index 4903180c487..5a313952473 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs @@ -23,8 +23,14 @@ namespace OpenTelemetry.Exporter { /// - /// Configuration options for the OpenTelemetry Protocol (OTLP) exporter. + /// OpenTelemetry Protocol (OTLP) exporter options. + /// OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS, OTEL_EXPORTER_OTLP_TIMEOUT, OTEL_EXPORTER_OTLP_PROTOCOL + /// environment variables are parsed during object construction. /// + /// + /// The constructor throws if it fails to parse + /// any of the supported environment variables. + /// public class OtlpExporterOptions { internal const string EndpointEnvVarName = "OTEL_EXPORTER_OTLP_ENDPOINT"; diff --git a/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterOptions.cs b/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterOptions.cs index 35631e0287d..a9065f0a024 100644 --- a/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterOptions.cs +++ b/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterOptions.cs @@ -22,8 +22,14 @@ namespace OpenTelemetry.Exporter { /// - /// Zipkin trace exporter options. + /// Zipkin span exporter options. + /// OTEL_EXPORTER_ZIPKIN_ENDPOINT + /// environment variables are parsed during object construction. /// + /// + /// The constructor throws if it fails to parse + /// any of the supported environment variables. + /// public sealed class ZipkinExporterOptions { internal const int DefaultMaxPayloadSizeInBytes = 4096; From 2523936ce7083feaecaa9c11bd5e95798e4a5efe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 20:17:31 +0200 Subject: [PATCH 13/23] Update exporter readmes --- src/OpenTelemetry.Exporter.Jaeger/README.md | 3 +++ src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md | 3 +++ src/OpenTelemetry.Exporter.Zipkin/README.md | 3 +++ 3 files changed, 9 insertions(+) diff --git a/src/OpenTelemetry.Exporter.Jaeger/README.md b/src/OpenTelemetry.Exporter.Jaeger/README.md index e93dbab241d..b2dd7ee355c 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/README.md +++ b/src/OpenTelemetry.Exporter.Jaeger/README.md @@ -62,6 +62,9 @@ values of the `JaegerExporterOptions`. | `OTEL_EXPORTER_JAEGER_AGENT_HOST` | `AgentHost` | | `OTEL_EXPORTER_JAEGER_AGENT_PORT` | `AgentPort` | +`FormatException` is thrown in case of an invalid value for any of the +supported environment variables. + ## References * [Jaeger](https://www.jaegertracing.io) diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md index 172ed9895da..3dc7a10a9dc 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md @@ -54,6 +54,9 @@ values of the `OtlpExporterOptions` | `OTEL_EXPORTER_OTLP_TIMEOUT` | `TimeoutMilliseconds` | | `OTEL_EXPORTER_OTLP_PROTOCOL` | `Protocol` (grpc or http/protobuf)| +`FormatException` is thrown in case of an invalid value for any of the +supported environment variables. + ## OTLP Logs This package currently only supports exporting traces and metrics. Once the diff --git a/src/OpenTelemetry.Exporter.Zipkin/README.md b/src/OpenTelemetry.Exporter.Zipkin/README.md index 779d464be55..967caa4bb1e 100644 --- a/src/OpenTelemetry.Exporter.Zipkin/README.md +++ b/src/OpenTelemetry.Exporter.Zipkin/README.md @@ -62,6 +62,9 @@ values of the `ZipkinExporterOptions`. | --------------------------------| -------------------------------- | | `OTEL_EXPORTER_ZIPKIN_ENDPOINT` | `Endpoint` | +`FormatException` is thrown in case of an invalid value for any of the +supported environment variables. + ## References * [OpenTelemetry Project](https://opentelemetry.io/) From 81949eeb7f18962b38d5a4879a1817ff5b6ea080 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 20:24:05 +0200 Subject: [PATCH 14/23] Update changelogs --- src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md | 4 ++++ .../CHANGELOG.md | 4 ++++ src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md | 4 ++++ src/OpenTelemetry/CHANGELOG.md | 5 +++-- 4 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md index 54846046013..53d2a0434cf 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md +++ b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +* Changed `JaegerExporterOptions` constructor to throw + `FormatException` if it fails to parse any of the supported environment + variables. + ## 1.2.0-beta1 Released 2021-Oct-08 diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md index 3a6d5d4d230..3c730e087e7 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +* Changed `OtlpExporterOptions` constructor to throw + `FormatException` if it fails to parse any of the supported environment + variables. + ## 1.2.0-beta1 Released 2021-Oct-08 diff --git a/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md b/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md index fa628efb7e5..3b8b054e0b6 100644 --- a/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md +++ b/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +* Changed `ZipkinExporterOptions` constructor to throw + `FormatException` if it fails to parse any of the supported environment + variables. + ## 1.2.0-beta1 Released 2021-Oct-08 diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 6d7c47f2c14..c3d325864ff 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,8 +2,9 @@ ## Unreleased -* Changed `BatchExportActivityProcessorOptions` to throw `FormatException` - if it fails to parse any of the supported environment variables. +* Changed `BatchExportActivityProcessorOptions` constructor to throw + `FormatException` if it fails to parse any of the supported environment + variables. ## 1.2.0-beta1 From 921a40578556c915ad9577aa14db85dcd1c8f571 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 20:41:55 +0200 Subject: [PATCH 15/23] Add doc comments to EnvironmentVariableHelper --- .../Internal/EnvironmentVariableHelper.cs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index 66579ad9b21..1449cba79dc 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -20,8 +20,19 @@ namespace OpenTelemetry.Internal { + /// + /// EnvironmentVariableHelper facilitates parsing environmetal variable values defined by + /// + /// the specification + /// internal static class EnvironmentVariableHelper { + /// + /// Reads an environmetal variable without any parsing. + /// + /// + /// Returns true when if a non-empty value was read; otherwise, false. + /// public static bool LoadString(string envVarKey, out string result) { result = null; @@ -41,6 +52,15 @@ public static bool LoadString(string envVarKey, out string result) return !string.IsNullOrEmpty(result); } + /// + /// Reads an environmetal variable and parses is as a non-negative decimal number. + /// + /// + /// Returns true when if a non-empty value was read; otherwise, false. + /// + /// + /// Thrown when failed to parse the non-empty value. + /// public static bool LoadNumeric(string envVarKey, out int result) { result = 0; @@ -58,6 +78,15 @@ public static bool LoadNumeric(string envVarKey, out int result) return true; } + /// + /// Reads an environmetal variable and parses is as a . + /// + /// + /// Returns true when if a non-empty value was read; otherwise, false. + /// + /// + /// Thrown when failed to parse the non-empty value. + /// public static bool LoadUri(string envVarKey, out Uri result) { result = null; From 86a3878da089efa0d7c5c04d41e484575664857a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 20:42:44 +0200 Subject: [PATCH 16/23] Fix grammar error --- src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index 1449cba79dc..c82a8580bcb 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -21,7 +21,7 @@ namespace OpenTelemetry.Internal { /// - /// EnvironmentVariableHelper facilitates parsing environmetal variable values defined by + /// EnvironmentVariableHelper facilitates parsing environmetal variable values as defined by /// /// the specification /// From c917863b2e971284cc7f96b5f4b3d22a95fa283e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 20:49:45 +0200 Subject: [PATCH 17/23] Fix lint errors --- src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index c82a8580bcb..96b346dc322 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -23,13 +23,15 @@ namespace OpenTelemetry.Internal /// /// EnvironmentVariableHelper facilitates parsing environmetal variable values as defined by /// - /// the specification + /// the specification. /// internal static class EnvironmentVariableHelper { /// /// Reads an environmetal variable without any parsing. /// + /// The name of the environment variable. + /// The parsed value of the environment variable. /// /// Returns true when if a non-empty value was read; otherwise, false. /// @@ -55,6 +57,8 @@ public static bool LoadString(string envVarKey, out string result) /// /// Reads an environmetal variable and parses is as a non-negative decimal number. /// + /// The name of the environment variable. + /// The parsed value of the environment variable. /// /// Returns true when if a non-empty value was read; otherwise, false. /// @@ -81,6 +85,8 @@ public static bool LoadNumeric(string envVarKey, out int result) /// /// Reads an environmetal variable and parses is as a . /// + /// The name of the environment variable. + /// The parsed value of the environment variable. /// /// Returns true when if a non-empty value was read; otherwise, false. /// From b6d34fedd2f2a12756a4b9d29aa9ac4698b78bea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Oct 2021 20:51:50 +0200 Subject: [PATCH 18/23] Fix typos --- src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index 96b346dc322..d2aa2d36638 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -21,14 +21,14 @@ namespace OpenTelemetry.Internal { /// - /// EnvironmentVariableHelper facilitates parsing environmetal variable values as defined by + /// EnvironmentVariableHelper facilitates parsing environment variable values as defined by /// /// the specification. /// internal static class EnvironmentVariableHelper { /// - /// Reads an environmetal variable without any parsing. + /// Reads an environment variable without any parsing. /// /// The name of the environment variable. /// The parsed value of the environment variable. @@ -55,7 +55,7 @@ public static bool LoadString(string envVarKey, out string result) } /// - /// Reads an environmetal variable and parses is as a non-negative decimal number. + /// Reads an environment variable and parses is as a non-negative decimal number. /// /// The name of the environment variable. /// The parsed value of the environment variable. @@ -83,7 +83,7 @@ public static bool LoadNumeric(string envVarKey, out int result) } /// - /// Reads an environmetal variable and parses is as a . + /// Reads an environment variable and parses is as a . /// /// The name of the environment variable. /// The parsed value of the environment variable. From 0fe88c95b16c6a1a58e06bd6893ccc47d752329d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 21 Oct 2021 10:58:05 +0200 Subject: [PATCH 19/23] LoadNumeric handles -1 --- src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs | 9 ++++++++- .../Internal/EnvironmentVariableHelperTests.cs | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index d2aa2d36638..a01784d18ab 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -55,7 +55,7 @@ public static bool LoadString(string envVarKey, out string result) } /// - /// Reads an environment variable and parses is as a non-negative decimal number. + /// Reads an environment variable and parses is as a non-negative decimal number or -1. /// /// The name of the environment variable. /// The parsed value of the environment variable. @@ -74,6 +74,13 @@ public static bool LoadNumeric(string envVarKey, out int result) return false; } + if (value == "-1") + { + // -1 is a special case according to the specifiaction + result = -1; + return true; + } + if (!int.TryParse(value, NumberStyles.None, CultureInfo.InvariantCulture, out result)) { throw new FormatException($"{envVarKey} environment variable has an invalid value: '${value}'"); diff --git a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs index 9cc42956108..380f05042f7 100644 --- a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs +++ b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs @@ -57,6 +57,7 @@ public void LoadString_NoValue() [Theory] [InlineData("123", 123)] [InlineData("0", 0)] + [InlineData("-1", -1)] public void LoadNumeric(string value, int expectedValue) { Environment.SetEnvironmentVariable(EnvVar, value); From da979a317257fd4f5c9b7bf9d56cefc0ec230099 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 21 Oct 2021 20:33:54 +0200 Subject: [PATCH 20/23] Update EnvironmentVariableHelper.cs --- src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index a01784d18ab..072a0f6d3d7 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -33,7 +33,7 @@ internal static class EnvironmentVariableHelper /// The name of the environment variable. /// The parsed value of the environment variable. /// - /// Returns true when if a non-empty value was read; otherwise, false. + /// Returns true when a non-empty value was read; otherwise, false. /// public static bool LoadString(string envVarKey, out string result) { @@ -60,7 +60,7 @@ public static bool LoadString(string envVarKey, out string result) /// The name of the environment variable. /// The parsed value of the environment variable. /// - /// Returns true when if a non-empty value was read; otherwise, false. + /// Returns true when a non-empty value was read; otherwise, false. /// /// /// Thrown when failed to parse the non-empty value. @@ -95,7 +95,7 @@ public static bool LoadNumeric(string envVarKey, out int result) /// The name of the environment variable. /// The parsed value of the environment variable. /// - /// Returns true when if a non-empty value was read; otherwise, false. + /// Returns true when a non-empty value was read; otherwise, false. /// /// /// Thrown when failed to parse the non-empty value. From 6a4f9af77697bf7f9e9a4f9a6694092bb7ca9ade Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 21 Oct 2021 21:01:47 +0200 Subject: [PATCH 21/23] Apply suggestions from code review Co-authored-by: Michael Maxwell --- src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index 072a0f6d3d7..4db8e2240d4 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -55,7 +55,7 @@ public static bool LoadString(string envVarKey, out string result) } /// - /// Reads an environment variable and parses is as a non-negative decimal number or -1. + /// Reads an environment variable and parses is as a non-negative decimal integer or -1. /// /// The name of the environment variable. /// The parsed value of the environment variable. @@ -76,7 +76,7 @@ public static bool LoadNumeric(string envVarKey, out int result) if (value == "-1") { - // -1 is a special case according to the specifiaction + // -1 is a special case according to the specification result = -1; return true; } @@ -90,7 +90,7 @@ public static bool LoadNumeric(string envVarKey, out int result) } /// - /// Reads an environment variable and parses is as a . + /// Reads an environment variable and parses it as a . /// /// The name of the environment variable. /// The parsed value of the environment variable. From 28e583ce1efa55662976edef09215bb23feae4ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 21 Oct 2021 21:08:34 +0200 Subject: [PATCH 22/23] Update EnvironmentVariableHelper.cs --- src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index 4db8e2240d4..7b3d03942db 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -55,7 +55,9 @@ public static bool LoadString(string envVarKey, out string result) } /// - /// Reads an environment variable and parses is as a non-negative decimal integer or -1. + /// Reads an environment variable and parses is as a + /// + /// numeric value - a non-negative decimal integer or -1. /// /// The name of the environment variable. /// The parsed value of the environment variable. From 22acae85136ea0e5dcbf25e5d8202f5f8e84e2eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 21 Oct 2021 22:30:35 +0200 Subject: [PATCH 23/23] I am unable to read with understanding --- src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs | 9 +-------- .../Internal/EnvironmentVariableHelperTests.cs | 2 +- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs index 7b3d03942db..0061000c3f0 100644 --- a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs +++ b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs @@ -57,7 +57,7 @@ public static bool LoadString(string envVarKey, out string result) /// /// Reads an environment variable and parses is as a /// - /// numeric value - a non-negative decimal integer or -1. + /// numeric value - a non-negative decimal integer. /// /// The name of the environment variable. /// The parsed value of the environment variable. @@ -76,13 +76,6 @@ public static bool LoadNumeric(string envVarKey, out int result) return false; } - if (value == "-1") - { - // -1 is a special case according to the specification - result = -1; - return true; - } - if (!int.TryParse(value, NumberStyles.None, CultureInfo.InvariantCulture, out result)) { throw new FormatException($"{envVarKey} environment variable has an invalid value: '${value}'"); diff --git a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs index 380f05042f7..f5a4618f601 100644 --- a/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs +++ b/test/OpenTelemetry.Tests/Internal/EnvironmentVariableHelperTests.cs @@ -57,7 +57,6 @@ public void LoadString_NoValue() [Theory] [InlineData("123", 123)] [InlineData("0", 0)] - [InlineData("-1", -1)] public void LoadNumeric(string value, int expectedValue) { Environment.SetEnvironmentVariable(EnvVar, value); @@ -81,6 +80,7 @@ public void LoadNumeric_NoValue() [InlineData("something")] // NaN [InlineData("-12")] // negative number not allowed [InlineData("-0")] // sign not allowed + [InlineData("-1")] // -1 is not allowed [InlineData(" 123 ")] // whitespaces not allowed [InlineData("0xFF")] // only decimal number allowed public void LoadNumeric_Invalid(string value)