-
Notifications
You must be signed in to change notification settings - Fork 135
Fix OTEL_EXPORTER_OTLP_*_PROTOCOL handling #4601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -158,8 +158,44 @@ private static Uri GetOtlpHttpEndpoint(string? endpoint, OtlpSignalType signalTy | |
| { | ||
| // the default in SDK is grpc. http/protobuf should be default for our purposes | ||
| var priorityVar = OtlpSpecConfigDefinitions.GetProtocolEnvVar(signalType); | ||
| var exporterOtlpProtocol = configuration.GetString(priorityVar) ?? | ||
| configuration.GetString(OtlpSpecConfigDefinitions.DefaultProtocolEnvVarName); | ||
| var exporterOtlpProtocol = configuration.GetString(priorityVar); | ||
|
|
||
| // SDK handles only general environment variables. | ||
| // In case priority env is set, the value must be maually passed. | ||
| if (!string.IsNullOrEmpty(exporterOtlpProtocol)) | ||
| { | ||
| switch (exporterOtlpProtocol) | ||
| { | ||
| case "grpc": | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we support mixed case like "gRPC"?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the moment, I think we should keep it as it is (case sensitive), since all our enum values are currently case sensitive. However, in the latest release of the specification, this behavior was changed, and we should start making all enum values as case insensitive. It should be a separate issue.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Thanks for the reference! |
||
| #if NETFRAMEWORK | ||
| if (configuration.FailFast) | ||
| { | ||
| throw new ArgumentException( | ||
| $"OTLP protocol 'grpc' is not supported on .NET Framework in environment variable '{priorityVar}'. Use 'http/protobuf' instead."); | ||
| } | ||
| else | ||
| { | ||
| // null value here means that it will be handled by OTEL .NET SDK | ||
| return null; | ||
| } | ||
|
Comment on lines
+177
to
+180
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can stop relying on the Otel .NET SDK to handle this and manage the protocol ourselves instead. That way, there will be less confusion about when it’s handled by us and when it’s handled by the SDK. |
||
| #else | ||
| return OtlpExportProtocol.Grpc; | ||
| #endif | ||
| case "http/protobuf": | ||
| return OtlpExportProtocol.HttpProtobuf; | ||
| default: | ||
| if (configuration.FailFast) | ||
| { | ||
| throw new ArgumentException( | ||
| $"Invalid OTLP protocol value '{exporterOtlpProtocol}' in environment variable '{priorityVar}'. Supported values are 'grpc' and 'http/protobuf'."); | ||
| } | ||
|
|
||
| // null value here means that it will be handled by OTEL .NET SDK | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| exporterOtlpProtocol = configuration.GetString(OtlpSpecConfigDefinitions.DefaultProtocolEnvVarName); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic above should also be applied to this? ( |
||
|
|
||
| if (string.IsNullOrEmpty(exporterOtlpProtocol)) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| using System.Reflection; | ||
| using OpenTelemetry.AutoInstrumentation.Configurations; | ||
| using OpenTelemetry.AutoInstrumentation.Configurations.Otlp; | ||
| using OpenTelemetry.Exporter; | ||
| using Xunit; | ||
|
|
||
|
|
@@ -393,6 +395,48 @@ internal void OtlpExportProtocol_DependsOnCorrespondingEnvVariable(string? otlpP | |
| Assert.Equal(expectedOtlpExportProtocol, settings.OtlpSettings.Protocol); | ||
| } | ||
|
|
||
| [Fact] | ||
| internal void OtlpExportProtocol_CheckPriorityEnvIsSet_Traces() | ||
| { | ||
| VerifyOtlpPrioritySettings(((string Endpoint, string Protocol, string Timeout, string Headers) values) => | ||
| { | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.TracesEndpointEnvVarName, values.Endpoint); | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.TracesProtocolEnvVarName, values.Protocol); | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.TracesTimeoutEnvVarName, values.Timeout); | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.TracesHeadersEnvVarName, values.Headers); | ||
|
|
||
| return Settings.FromDefaultSources<TracerSettings>(false).OtlpSettings; | ||
| }); | ||
| } | ||
|
|
||
| [Fact] | ||
| internal void OtlpExportProtocol_CheckPriorityEnvIsSet_Metrics() | ||
| { | ||
| VerifyOtlpPrioritySettings(((string Endpoint, string Protocol, string Timeout, string Headers) values) => | ||
| { | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.MetricsEndpointEnvVarName, values.Endpoint); | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.MetricsProtocolEnvVarName, values.Protocol); | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.MetricsTimeoutEnvVarName, values.Timeout); | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.MetricsHeadersEnvVarName, values.Headers); | ||
|
|
||
| return Settings.FromDefaultSources<MetricSettings>(false).OtlpSettings; | ||
| }); | ||
| } | ||
|
|
||
| [Fact] | ||
| internal void OtlpExportProtocol_CheckPriorityEnvIsSet_Logs() | ||
| { | ||
| VerifyOtlpPrioritySettings(((string Endpoint, string Protocol, string Timeout, string Headers) values) => | ||
| { | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.LogsEndpointEnvVarName, values.Endpoint); | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.LogsProtocolEnvVarName, values.Protocol); | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.LogsTimeoutEnvVarName, values.Timeout); | ||
| Environment.SetEnvironmentVariable(AutoOtlpDefinitions.LogsHeadersEnvVarName, values.Headers); | ||
|
|
||
| return Settings.FromDefaultSources<LogSettings>(false).OtlpSettings; | ||
| }); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("true", true)] | ||
| [InlineData("false", false)] | ||
|
|
@@ -477,5 +521,39 @@ private static void ClearEnvVars() | |
| Environment.SetEnvironmentVariable(ConfigurationKeys.Traces.InstrumentationOptions.HttpInstrumentationCaptureRequestHeaders, null); | ||
| Environment.SetEnvironmentVariable(ConfigurationKeys.Traces.InstrumentationOptions.HttpInstrumentationCaptureResponseHeaders, null); | ||
| Environment.SetEnvironmentVariable(ConfigurationKeys.Traces.InstrumentationOptions.OracleMdaSetDbStatementForText, null); | ||
|
|
||
| // Cleanup OTLP env vars | ||
| foreach (var envVar in GetAllOtlpEnvVarNames()) | ||
| { | ||
| Environment.SetEnvironmentVariable(envVar, null); | ||
| } | ||
| } | ||
|
|
||
| private static IEnumerable<string> GetAllOtlpEnvVarNames() | ||
| { | ||
| return typeof(AutoOtlpDefinitions) | ||
| .GetFields(BindingFlags.Public | BindingFlags.Static) | ||
| .Where(f => f.FieldType == typeof(string)) | ||
| .Select(f => (string)f.GetValue(null)!) | ||
| .ToList() ?? Enumerable.Empty<string>(); | ||
| } | ||
|
|
||
| private static void VerifyOtlpPrioritySettings(Func<(string Endpoint, string Protocol, string Timeout, string Headers), OtlpSettings?> setup) | ||
| { | ||
| var endpoint = "http://example.org/traces/v1"; | ||
| var endpointValue = new Uri(endpoint); | ||
| var protocol = "http/protobuf"; | ||
| var protocolValue = OtlpExportProtocol.HttpProtobuf; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about other values like "grpc", "invalidProtocol" or "" (empty string)?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. *Applies to the line above. |
||
| var timeout = "1"; | ||
| var timeoutValue = 1; | ||
| var headers = "key1=value1,key2=value2"; | ||
|
|
||
| var settings = setup((endpoint, protocol, timeout, headers)); | ||
|
|
||
| Assert.NotNull(settings); | ||
| Assert.Equal(endpointValue, settings.Endpoint); | ||
| Assert.Equal(protocolValue, settings.Protocol); | ||
| Assert.Equal(timeoutValue, settings.TimeoutMilliseconds); | ||
| Assert.Equal(headers, settings.Headers); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.