Update OTEL_EXPORTER_JAEGER_PROTOCOL parsing (#2914)

* typo fixes for messages while parsing ev vars * extend env variable names tests * auto cleanup test * typo fix for internal variable * verify default value for jeager exporter protocol * align env vars for jeager exporter protocol with specification * code review fix - better documentation * disable MD013 for table * extend test for OTEL_EXPORTER_JAEGER_ENDPOINT * code review - nit fix Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Mikel Blanchard <[email protected]>
open-telemetry · Feb 22, 2022 · 4b3ee96 · 4b3ee96
1 parent f234829
commit 4b3ee96
Show file tree

Hide file tree

Showing 9 changed files with 119 additions and 30 deletions.
diff --git a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md
@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+* Change supported values for `OTEL_EXPORTER_JAEGER_PROTOCOL`
+  Supported values: `udp/thrift.compact` and `http/thrift.binary` defined
+  in the [specification](https://github.com/open-telemetry/opentelemetry-specification/blob/9a0a3300c6269c2837a1d7c9c5232ec816f63222/specification/sdk-environment-variables.md?plain=1#L129).
+  ([#2914](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2914))
+
 ## 1.2.0-rc2
 
 Released 2022-Feb-02

diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs
@@ -35,7 +35,7 @@ public class JaegerExporterOptions
     {
         internal const int DefaultMaxPayloadSizeInBytes = 4096;
 
-        internal const string OtelProtocolEnvVarKey = "OTEL_EXPORTER_JAEGER_PROTOCOL";
+        internal const string OTelProtocolEnvVarKey = "OTEL_EXPORTER_JAEGER_PROTOCOL";
         internal const string OTelAgentHostEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_HOST";
         internal const string OTelAgentPortEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_PORT";
         internal const string OTelEndpointEnvVarKey = "OTEL_EXPORTER_JAEGER_ENDPOINT";
@@ -44,10 +44,18 @@ public class JaegerExporterOptions
 
         public JaegerExporterOptions()
         {
-            if (EnvironmentVariableHelper.LoadString(OtelProtocolEnvVarKey, out string protocolEnvVar)
-                && Enum.TryParse(protocolEnvVar, ignoreCase: true, out JaegerExportProtocol protocol))
+            if (EnvironmentVariableHelper.LoadString(OTelProtocolEnvVarKey, out string protocolEnvVar))
             {
-                this.Protocol = protocol;
+                var protocol = protocolEnvVar.ToJaegerExportProtocol();
+
+                if (protocol.HasValue)
+                {
+                    this.Protocol = protocol.Value;
+                }
+                else
+                {
+                    throw new FormatException($"{OTelProtocolEnvVarKey} environment variable has an invalid value: '{protocolEnvVar}'");
+                }
             }
 
             if (EnvironmentVariableHelper.LoadString(OTelAgentHostEnvVarKey, out string agentHostEnvVar))

diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptionsExtensions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptionsExtensions.cs
@@ -0,0 +1,28 @@
+// <copyright file="JaegerExporterOptionsExtensions.cs" company="OpenTelemetry Authors">
+// 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.
+// </copyright>
+
+namespace OpenTelemetry.Exporter;
+
+internal static class JaegerExporterOptionsExtensions
+{
+    public static JaegerExportProtocol? ToJaegerExportProtocol(this string protocol) =>
+        protocol?.Trim() switch
+        {
+            "udp/thrift.compact" => JaegerExportProtocol.UdpCompactThrift,
+            "http/thrift.binary" => JaegerExportProtocol.HttpBinaryThrift,
+            _ => null,
+        };
+}
diff --git a/src/OpenTelemetry.Exporter.Jaeger/README.md b/src/OpenTelemetry.Exporter.Jaeger/README.md
@@ -64,25 +64,28 @@ properties:
 
 * `Protocol`: The protocol to use. The default value is `UdpCompactThrift`.
 
-  | Protocol       | Description                                           |
-  |----------------|-------------------------------------------------------|
-  |UdpCompactThrift| Apache Thrift compact over UDP to a Jaeger Agent.     |
-  |HttpBinaryThrift| Apache Thrift binary over HTTP to a Jaeger Collector. |
+  | Protocol         | Description                                           |
+  |------------------|-------------------------------------------------------|
+  |`UdpCompactThrift`| Apache Thrift compact over UDP to a Jaeger Agent.     |
+  |`HttpBinaryThrift`| Apache Thrift binary over HTTP to a Jaeger Collector. |
 
 See the [`TestJaegerExporter.cs`](../../examples/Console/TestJaegerExporter.cs)
 for an example of how to use the exporter.
 
 ## Environment Variables
 
 The following environment variables can be used to override the default
-values of the `JaegerExporterOptions`.
-
-| Environment variable               | `JaegerExporterOptions` property |
-| ---------------------------------- | -------------------------------- |
-| `OTEL_EXPORTER_JAEGER_AGENT_HOST`  | `AgentHost`                      |
-| `OTEL_EXPORTER_JAEGER_AGENT_PORT`  | `AgentPort`                      |
-| `OTEL_EXPORTER_JAEGER_ENDPOINT`    | `Endpoint`                       |
-| `OTEL_EXPORTER_JAEGER_PROTOCOL`    | `Protocol`                       |
+values of the `JaegerExporterOptions`
+(following the [OpenTelemetry specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#jaeger-exporter)).
+
+  <!-- markdownlint-disable MD013 -->
+| Environment variable              | `JaegerExporterOptions` property                          |
+|-----------------------------------|-----------------------------------------------------------|
+| `OTEL_EXPORTER_JAEGER_AGENT_HOST` | `AgentHost`                                               |
+| `OTEL_EXPORTER_JAEGER_AGENT_PORT` | `AgentPort`                                               |
+| `OTEL_EXPORTER_JAEGER_ENDPOINT`   | `Endpoint`                                                |
+| `OTEL_EXPORTER_JAEGER_PROTOCOL`   | `Protocol` (`udp/thrift.compact` or `http/thrift.binary`) |
+  <!-- markdownlint-enable MD013 -->
 
 `FormatException` is thrown in case of an invalid value for any of the
 supported environment variables.

diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs
@@ -79,7 +79,7 @@ public OtlpExporterOptions()
                 }
                 else
                 {
-                    throw new FormatException($"{ProtocolEnvVarName} environment variable has an invalid value: '${protocolEnvVar}'");
+                    throw new FormatException($"{ProtocolEnvVarName} environment variable has an invalid value: '{protocolEnvVar}'");
                 }
             }
 

diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md
@@ -56,12 +56,12 @@ The following environment variables can be used to override the default
 values of the `OtlpExporterOptions`
 (following the [OpenTelemetry specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md)).
 
-| Environment variable          | `OtlpExporterOptions` property    |
-| ------------------------------| ----------------------------------|
-| `OTEL_EXPORTER_OTLP_ENDPOINT` | `Endpoint`                        |
-| `OTEL_EXPORTER_OTLP_HEADERS`  | `Headers`                         |
-| `OTEL_EXPORTER_OTLP_TIMEOUT`  | `TimeoutMilliseconds`             |
-| `OTEL_EXPORTER_OTLP_PROTOCOL` | `Protocol` (grpc or http/protobuf)|
+| Environment variable          | `OtlpExporterOptions` property        |
+| ------------------------------| --------------------------------------|
+| `OTEL_EXPORTER_OTLP_ENDPOINT` | `Endpoint`                            |
+| `OTEL_EXPORTER_OTLP_HEADERS`  | `Headers`                             |
+| `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.

diff --git a/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs b/src/OpenTelemetry/Internal/EnvironmentVariableHelper.cs
@@ -78,7 +78,7 @@ public static bool LoadNumeric(string envVarKey, out int result)
 
             if (!int.TryParse(value, NumberStyles.None, CultureInfo.InvariantCulture, out result))
             {
-                throw new FormatException($"{envVarKey} environment variable has an invalid value: '${value}'");
+                throw new FormatException($"{envVarKey} environment variable has an invalid value: '{value}'");
             }
 
             return true;

diff --git a/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsExtensionsTests.cs b/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsExtensionsTests.cs
@@ -0,0 +1,33 @@
+// <copyright file="JaegerExporterOptionsExtensionsTests.cs" company="OpenTelemetry Authors">
+// 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.
+// </copyright>
+
+using Xunit;
+
+namespace OpenTelemetry.Exporter.Jaeger.Tests;
+
+public class JaegerExporterOptionsExtensionsTests
+{
+    [Theory]
+    [InlineData("udp/thrift.compact", JaegerExportProtocol.UdpCompactThrift)]
+    [InlineData("http/thrift.binary", JaegerExportProtocol.HttpBinaryThrift)]
+    [InlineData("unsupported", null)]
+    public void ToJaegerExportProtocol_Protocol_MapsToCorrectValue(string protocol, JaegerExportProtocol? expectedExportProtocol)
+    {
+        var exportProtocol = protocol.ToJaegerExportProtocol();
+
+        Assert.Equal(expectedExportProtocol, exportProtocol);
+    }
+}
diff --git a/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs b/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs
@@ -23,12 +23,12 @@ public class JaegerExporterOptionsTests : IDisposable
     {
         public JaegerExporterOptionsTests()
         {
-            this.ClearEnvVars();
+            ClearEnvVars();
         }
 
         public void Dispose()
         {
-            this.ClearEnvVars();
+            ClearEnvVars();
         }
 
         [Fact]
@@ -40,24 +40,32 @@ public void JaegerExporterOptions_Defaults()
             Assert.Equal(6831, options.AgentPort);
             Assert.Equal(4096, options.MaxPayloadSizeInBytes);
             Assert.Equal(ExportProcessorType.Batch, options.ExportProcessorType);
+            Assert.Equal(JaegerExportProtocol.UdpCompactThrift, options.Protocol);
+            Assert.Equal(new Uri("http://localhost:14268"), options.Endpoint);
         }
 
         [Fact]
         public void JaegerExporterOptions_EnvironmentVariableOverride()
         {
             Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelAgentHostEnvVarKey, "jeager-host");
             Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelAgentPortEnvVarKey, "123");
+            Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelProtocolEnvVarKey, "http/thrift.binary");
+            Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelEndpointEnvVarKey, "http://custom-endpoint:12345");
 
             var options = new JaegerExporterOptions();
 
             Assert.Equal("jeager-host", options.AgentHost);
             Assert.Equal(123, options.AgentPort);
+            Assert.Equal(JaegerExportProtocol.HttpBinaryThrift, options.Protocol);
+            Assert.Equal(new Uri("http://custom-endpoint:12345"), options.Endpoint);
         }
 
-        [Fact]
-        public void JaegerExporterOptions_InvalidPortEnvironmentVariableOverride()
+        [Theory]
+        [InlineData(JaegerExporterOptions.OTelAgentPortEnvVarKey)]
+        [InlineData(JaegerExporterOptions.OTelProtocolEnvVarKey)]
+        public void JaegerExporterOptions_InvalidEnvironmentVariableOverride(string envVar)
         {
-            Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelAgentPortEnvVarKey, "invalid");
+            Environment.SetEnvironmentVariable(envVar, "invalid");
 
             Assert.Throws<FormatException>(() => new JaegerExporterOptions());
         }
@@ -78,14 +86,18 @@ public void JaegerExporterOptions_SetterOverridesEnvironmentVariable()
         [Fact]
         public void JaegerExporterOptions_EnvironmentVariableNames()
         {
+            Assert.Equal("OTEL_EXPORTER_JAEGER_PROTOCOL", JaegerExporterOptions.OTelProtocolEnvVarKey);
             Assert.Equal("OTEL_EXPORTER_JAEGER_AGENT_HOST", JaegerExporterOptions.OTelAgentHostEnvVarKey);
             Assert.Equal("OTEL_EXPORTER_JAEGER_AGENT_PORT", JaegerExporterOptions.OTelAgentPortEnvVarKey);
+            Assert.Equal("OTEL_EXPORTER_JAEGER_ENDPOINT", JaegerExporterOptions.OTelEndpointEnvVarKey);
         }
 
-        private void ClearEnvVars()
+        private static void ClearEnvVars()
         {
+            Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelProtocolEnvVarKey, null);
             Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelAgentHostEnvVarKey, null);
             Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelAgentPortEnvVarKey, null);
+            Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelEndpointEnvVarKey, null);
         }
     }
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -79,7 +79,7 @@ public OtlpExporterOptions() @@
                     }
                     else
                     {
-                        throw new FormatException($"{ProtocolEnvVarName} environment variable has an invalid value: '${protocolEnvVar}'");
+                        throw new FormatException($"{ProtocolEnvVarName} environment variable has an invalid value: '{protocolEnvVar}'");
                     }
                 }
@@ Expand Down @@