Skip to content
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.

Sign up for GitHub

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

exporter/zipkin: adding support for env var OTEL_EXPORTER_ZIPKIN_ENDPOINT #1064

Merged
merged 8 commits into from
Sep 9, 2020
Merged

exporter/zipkin: adding support for env var OTEL_EXPORTER_ZIPKIN_ENDPOINT #1064

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6f03bdb
add support for env var: OTEL_EXPORTER_ZIPKIN_ENDPOINT
Sep 2, 2020
320f84c
update changelog
Sep 2, 2020
309d5b8
add docs note
Sep 2, 2020
27b7856
Merge branch 'master' into zipkin-env-var
Sep 2, 2020
0beb750
update method signature to use url
Sep 3, 2020
ae8e5d7
Merge branch 'master' into zipkin-env-var
Sep 3, 2020
57d84b1
Merge branch 'master' into zipkin-env-var
Sep 8, 2020
d7e4f8c
Merge branch 'master' into zipkin-env-var
lzchen Sep 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions exporter/opentelemetry-exporter-zipkin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

- Add support for OTEL_EXPORTER_ZIPKIN_ENDPOINT env var
([#1064](https://github.com/open-telemetry/opentelemetry-python/pull/1064))

## Version 0.12b0

Released 2020-08-14
Expand Down
20 changes: 13 additions & 7 deletions exporter/opentelemetry-exporter-zipkin/src/opentelemetry/exporter/zipkin/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@

import json
import logging
import os
from typing import Optional, Sequence
from urllib.parse import urlparse

import requests

Expand All @@ -75,6 +77,9 @@
DEFAULT_PORT = 9411
DEFAULT_PROTOCOL = "http"
DEFAULT_RETRY = False
DEFAULT_URL = "{}://{}:{}{}".format(
DEFAULT_PROTOCOL, DEFAULT_HOST_NAME, DEFAULT_PORT, DEFAULT_ENDPOINT
)
ZIPKIN_HEADERS = {"Content-Type": "application/json"}

SPAN_KIND_MAP = {
Expand Down Expand Up @@ -117,13 +122,14 @@ def __init__(
retry: Optional[str] = DEFAULT_RETRY,
):
self.service_name = service_name
self.host_name = host_name
self.port = port
self.endpoint = endpoint
self.protocol = protocol
self.url = "{}://{}:{}{}".format(
self.protocol, self.host_name, self.port, self.endpoint
)
self.url = os.environ.get("OTEL_EXPORTER_ZIPKIN_ENDPOINT", DEFAULT_URL)

url = "{}://{}:{}{}".format(protocol, host_name, port, endpoint)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-exporter-zipkin/src/zipkin.ts#L44

Go

https://github.com/open-telemetry/opentelemetry-go/blob/master/exporters/trace/zipkin/zipkin.go#L37

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going with documenting the default value for now.

if url != DEFAULT_URL:
self.url = url

self.port = urlparse(self.url).port

self.ipv4 = ipv4
self.ipv6 = ipv6
self.retry = retry
Expand Down
30 changes: 21 additions & 9 deletions exporter/opentelemetry-exporter-zipkin/tests/test_zipkin_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.

import json
import os
import unittest
from unittest.mock import MagicMock, patch

Expand Down Expand Up @@ -43,25 +44,39 @@ def setUp(self):
self._test_span.start()
self._test_span.end()

def tearDown(self):
if "OTEL_EXPORTER_ZIPKIN_ENDPOINT" in os.environ:
del os.environ["OTEL_EXPORTER_ZIPKIN_ENDPOINT"]

def test_constructor_env_var(self):
"""Test the default values assigned by constructor."""
os.environ["OTEL_EXPORTER_ZIPKIN_ENDPOINT"] = "https://foo:9911/path"
service_name = "my-service-name"
port = 9911
exporter = ZipkinSpanExporter(service_name)
ipv4 = None
ipv6 = None
url = "https://foo:9911/path"

self.assertEqual(exporter.service_name, service_name)
self.assertEqual(exporter.port, port)
self.assertEqual(exporter.ipv4, ipv4)
self.assertEqual(exporter.ipv6, ipv6)
self.assertEqual(exporter.url, url)

def test_constructor_default(self):
"""Test the default values assigned by constructor."""
service_name = "my-service-name"
host_name = "localhost"
port = 9411
endpoint = "/api/v2/spans"
exporter = ZipkinSpanExporter(service_name)
ipv4 = None
ipv6 = None
protocol = "http"
url = "http://localhost:9411/api/v2/spans"

self.assertEqual(exporter.service_name, service_name)
self.assertEqual(exporter.host_name, host_name)
self.assertEqual(exporter.port, port)
self.assertEqual(exporter.endpoint, endpoint)
self.assertEqual(exporter.ipv4, ipv4)
self.assertEqual(exporter.ipv6, ipv6)
self.assertEqual(exporter.protocol, protocol)
self.assertEqual(exporter.url, url)

def test_constructor_explicit(self):
Expand All @@ -85,12 +100,9 @@ def test_constructor_explicit(self):
)

self.assertEqual(exporter.service_name, service_name)
self.assertEqual(exporter.host_name, host_name)
self.assertEqual(exporter.port, port)
self.assertEqual(exporter.endpoint, endpoint)
self.assertEqual(exporter.ipv4, ipv4)
self.assertEqual(exporter.ipv6, ipv6)
self.assertEqual(exporter.protocol, protocol)
self.assertEqual(exporter.url, url)

# pylint: disable=too-many-locals
Expand Down