exporter/zipkin: adding support for env var OTEL_EXPORTER_ZIPKIN_ENDPOINT#1064
exporter/zipkin: adding support for env var OTEL_EXPORTER_ZIPKIN_ENDPOINT#1064codeboten merged 8 commits intoopen-telemetry:masterfrom
Conversation
ffe4
left a comment
ffe4
left a comment
There was a problem hiding this comment.
LGTM but shouldn't we also mention the environment variable in the documentation?
| ) | ||
| self.url = os.environ.get("OTEL_EXPORTER_ZIPKIN_ENDPOINT", DEFAULT_URL) | ||
|
|
||
| url = "{}://{}:{}{}".format(protocol, host_name, port, endpoint) |
There was a problem hiding this comment.
I think default value for protocol should be https. We can have a special case to use http as a default when endpoint == DEFAULT_ENDPOINT.
There was a problem hiding this comment.
I'm kinda wondering if the configuration parameters should just be a URL instead of protocol/host/port/endpoint. Looking at the other implementations, it looks like that is how they've implemented it. This would also simplify supporting that env variable.
JS
Go
There was a problem hiding this comment.
+1
I think that makes sense. My only concern is how users can discover the path part of the URL. If a user knows their zipkin endpoints is available at https://zipkin.corp.com, would they know or remember to add :9411 and /api/v2/spans? Perhaps documenting the default value in zipkin exporter docs would be enough of an indication?
There was a problem hiding this comment.
Going with documenting the default value for now.
Good point, updated the docs |
5 participants
Description
As per spec, adding support for env var:
OTEL_EXPORTER_ZIPKIN_ENDPOINTFixes #1055
Type of change
Checklist: