Fix OTEL_EXPORTER_OTLP_*_PROTOCOL handling#4601
Conversation
RassK
changed the title
GetExporterOtlpProtocol not to ignore OTEL_EXPORTER_OTLP_*_PROTOCOL
| var endpoint = "http://example.org/traces/v1"; | ||
| var endpointValue = new Uri(endpoint); | ||
| var protocol = "http/protobuf"; | ||
| var protocolValue = OtlpExportProtocol.HttpProtobuf; |
There was a problem hiding this comment.
What about other values like "grpc", "invalidProtocol" or "" (empty string)?
There was a problem hiding this comment.
*Applies to the line above.
| { | ||
| switch (exporterOtlpProtocol) | ||
| { | ||
| case "grpc": |
There was a problem hiding this comment.
Do we support mixed case like "gRPC"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Makes sense. Thanks for the reference!
| var exporterOtlpProtocol = configuration.GetString(priorityVar); | ||
|
|
||
| // SDK handles only general environment variables. | ||
| // In case priority env is set, the value must be maually passed. |
There was a problem hiding this comment.
| // In case priority env is set, the value must be maually passed. | |
| // In case priority env is set, the value must be manually passed. |
| } | ||
| } | ||
|
|
||
| exporterOtlpProtocol = configuration.GetString(OtlpSpecConfigDefinitions.DefaultProtocolEnvVarName); |
There was a problem hiding this comment.
The logic above should also be applied to this? (DefaultProtocolEnvVarName is OTEL_EXPORTER_OTLP_PROTOCOL.)
| { | ||
| // null value here means that it will be handled by OTEL .NET SDK | ||
| return null; | ||
| } |
There was a problem hiding this comment.
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.
|
Changes included in #4627 |
4 participants
Why
Fixes #4593
Follow up #3527
What
GetExporterOtlpProtocolignores the more specific (priority) environment variable and does not pass the value to SDK.Tests
Checklist
CHANGELOG.mdis updated.