From 4b3ee96ffc39bc24c3b8377455b2c099bd9da6b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kie=C5=82kowicz?= Date: Tue, 22 Feb 2022 18:50:46 +0100 Subject: [PATCH] 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 Co-authored-by: Mikel Blanchard --- .../CHANGELOG.md | 5 +++ .../JaegerExporterOptions.cs | 16 ++++++--- .../JaegerExporterOptionsExtensions.cs | 28 ++++++++++++++++ src/OpenTelemetry.Exporter.Jaeger/README.md | 27 ++++++++------- .../OtlpExporterOptions.cs | 2 +- .../README.md | 12 +++---- .../Internal/EnvironmentVariableHelper.cs | 2 +- .../JaegerExporterOptionsExtensionsTests.cs | 33 +++++++++++++++++++ .../JaegerExporterOptionsTests.cs | 24 ++++++++++---- 9 files changed, 119 insertions(+), 30 deletions(-) create mode 100644 src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptionsExtensions.cs create mode 100644 test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsExtensionsTests.cs diff --git a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md index bdda877d331..2ad0e3555a9 100644 --- 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 index 3f2397bb498..eb78d6896f9 100644 --- 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 new file mode 100644 index 00000000000..12f225e421d --- /dev/null +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptionsExtensions.cs @@ -0,0 +1,28 @@ +// +// 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. +// + +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 index 57c9ba4f476..b73455af405 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/README.md +++ b/src/OpenTelemetry.Exporter.Jaeger/README.md @@ -64,10 +64,10 @@ 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. @@ -75,14 +75,17 @@ 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)). + + +| 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`) | + `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 index 0a624c68957..47435178ce5 100644 --- 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 index af7a3d694b4..abc7add6d94 100644 --- 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 index 0061000c3f0..df3f89df0aa 100644 --- 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 new file mode 100644 index 00000000000..0dfca9cc3cd --- /dev/null +++ b/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsExtensionsTests.cs @@ -0,0 +1,33 @@ +// +// 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 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 index 2aad340124e..983bd1ca210 100644 --- 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,6 +40,8 @@ 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] @@ -47,17 +49,23 @@ 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(() => 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); } } }