Add additional env variables for Jaeger exporter #1453
Conversation
| Addtionally, the following environment variables are reserved for future | ||
| usage in Jaeger Exporter configuration: | ||
|
|
||
| - `OTEL_EXPORTER_JAEGER_CERTIFICATE` |
There was a problem hiding this comment.
how about OTEL_EXPORTER_JAEGER_CLIENT_CERT_FILE? It's almost self-explanatory.
But even that seems not sufficient. For example, in the Jaeger C# client there are 3 env vars for TLS: https://github.com/jaegertracing/jaeger-client-csharp#configuration-via-environment
There was a problem hiding this comment.
For reference I had tried a pr for the three variables but couldn't get it in for 1.0 so need to rework it in a backwards compatible way.
I would go with just CERTIFICATE here to match otlp.
There was a problem hiding this comment.
As I understand it the private key and certificate chain pair is optional and can be null even for the secure connection. What do you think about OTEL_EXPORTER_JAEGER_CERTIFICATE, OTEL_EXPORTER_JAEGER_CLIENT_KEY and OTEL_EXPORTER_JAEGER_CLIENT_CHAIN?
There was a problem hiding this comment.
Do we have any SDK that already implements this auth mode and supports various certs? I would model the names after the APIs used (although different languages may call them differently).
BTW, I would recommend OTEL_EXPORTER_JAEGER_TLS_ prefix for all three, it provides a clear scope.
There was a problem hiding this comment.
Looks like Java provides way to set the ChannelCreds but doesn't have the support for loading from env. Python implementation uses OTEL_EXPORTER_JAEGER_CERTIFICATE. I couldn't find any other language SDK supporting them yet.
There was a problem hiding this comment.
@yurishkuro I don't think we should deviate from what we have for otlp since the protocols are so similar. I tried to get the tls prefix in before 1.0 but it didn't happen - I would say at this point consistency would be simpler. All grpc transports follow the same pattern.
|
|
||
| - `OTEL_EXPORTER_JAEGER_PROTOCOL` | ||
|
|
||
| Jaeger exporter has more than one transport protocol it can support, e.g. grpc, http/thrift, udp/thrift. |
There was a problem hiding this comment.
Since the exporters are vastly different, would it make more sense to have different values for exportet, e.g. jaeger, jaeger_http, jaeger_udp?
There was a problem hiding this comment.
I tried to follow along the similar lines of OTLP(grpc, http/protobuf, http/json), Zipkin for consistency. And also jaeger for Jaeger gRPC is misleading/not aligning with others.
There was a problem hiding this comment.
I see - it's a bit difficult because we have udp here, unlike the others. So any variables that relate to http specifically just can't apply, for example http headers (we have for otlp and still need to add for the others) and even tls I think. Would leaving udp to be separate make sense?
There was a problem hiding this comment.
Not sure If I understood you correctly but if headers are not applicable then they will not be set and I don't think tls applies to udp. What is the issue with udp not being separate?
There was a problem hiding this comment.
Yeah ignoring unsupported values is another approach too, it's mostly just to compare them. I don't think we currently have many variables that have a dependency on other ones, e.g. var2 is only read if var1 has a certain value. The separation could be clearer or it could be less clear, not too sure.
Co-authored-by: Yuri Shkuro <[email protected]>
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Changes
This change adds two additional env variables for Jaeger exporter
OTEL_EXPORTER_JAEGER_CERTIFICATEfor gRPC andOTEL_EXPORTER_JAEGER_PROTOCOLfor specifying protocol to use.