-
Notifications
You must be signed in to change notification settings - Fork 657
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
Support Environment Variables for JaegerSpanExporter configuration #1114
Changes from 10 commits
d180fd6
f7847dd
0913ee0
79cee16
2ff3c9f
3d8f333
a6bceb1
f98c6db
d466113
50a9312
ff5ae8d
952549c
b3c0da6
8cfc4ed
36e9819
d536a91
406486e
47ab344
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 |
---|---|---|
|
@@ -39,10 +39,7 @@ | |
agent_host_name='localhost', | ||
agent_port=6831, | ||
# optional: configure also collector | ||
# collector_host_name='localhost', | ||
# collector_port=14268, | ||
# collector_endpoint='/api/traces?format=jaeger.thrift', | ||
# collector_protocol='http', | ||
# collector_endpoint='http://localhost:14268/api/traces?format=jaeger.thrift', | ||
# username=xxxx, # optional | ||
# password=xxxx, # optional | ||
) | ||
|
@@ -69,16 +66,14 @@ | |
from thrift.protocol import TBinaryProtocol, TCompactProtocol | ||
from thrift.transport import THttpClient, TTransport | ||
|
||
import opentelemetry.trace as trace_api | ||
from opentelemetry.configuration import Configuration | ||
from opentelemetry.exporter.jaeger.gen.agent import Agent as agent | ||
from opentelemetry.exporter.jaeger.gen.jaeger import Collector as jaeger | ||
from opentelemetry.sdk.trace.export import Span, SpanExporter, SpanExportResult | ||
from opentelemetry.trace.status import StatusCanonicalCode | ||
|
||
DEFAULT_AGENT_HOST_NAME = "localhost" | ||
DEFAULT_AGENT_PORT = 6831 | ||
DEFAULT_COLLECTOR_ENDPOINT = "/api/traces?format=jaeger.thrift" | ||
DEFAULT_COLLECTOR_PROTOCOL = "http" | ||
|
||
UDP_PACKET_MAX_LENGTH = 65000 | ||
|
||
|
@@ -93,11 +88,7 @@ class JaegerSpanExporter(SpanExporter): | |
when query for spans. | ||
agent_host_name: The host name of the Jaeger-Agent. | ||
agent_port: The port of the Jaeger-Agent. | ||
collector_host_name: The host name of the Jaeger-Collector HTTP/HTTPS | ||
Thrift. | ||
collector_port: The port of the Jaeger-Collector HTTP/HTTPS Thrift. | ||
collector_endpoint: The endpoint of the Jaeger-Collector HTTP/HTTPS Thrift. | ||
collector_protocol: The transfer protocol for the Jaeger-Collector(HTTP or HTTPS). | ||
username: The user name of the Basic Auth if authentication is | ||
required. | ||
password: The password of the Basic Auth if authentication is | ||
|
@@ -109,23 +100,23 @@ def __init__( | |
service_name, | ||
agent_host_name=DEFAULT_AGENT_HOST_NAME, | ||
agent_port=DEFAULT_AGENT_PORT, | ||
collector_host_name=None, | ||
collector_port=None, | ||
collector_endpoint=DEFAULT_COLLECTOR_ENDPOINT, | ||
collector_protocol=DEFAULT_COLLECTOR_PROTOCOL, | ||
collector_endpoint=None, | ||
username=None, | ||
password=None, | ||
): | ||
self.service_name = service_name | ||
self.agent_host_name = agent_host_name | ||
self.agent_port = agent_port | ||
self.agent_host_name = ( | ||
Configuration().EXPORTER_JAEGER_AGENT_HOST or agent_host_name | ||
) | ||
self.agent_port = ( | ||
Configuration().EXPORTER_JAEGER_AGENT_PORT or agent_port | ||
) | ||
self._agent_client = None | ||
self.collector_host_name = collector_host_name | ||
self.collector_port = collector_port | ||
self.collector_endpoint = collector_endpoint | ||
self.collector_protocol = collector_protocol | ||
self.username = username | ||
self.password = password | ||
self.collector_endpoint = ( | ||
Configuration().EXPORTER_JAEGER_ENDPOINT or collector_endpoint | ||
) | ||
self.username = username or Configuration().EXPORTER_JAEGER_USER | ||
self.password = password or Configuration().EXPORTER_JAEGER_PASSWORD | ||
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. It looks like for username/password, precedence is given to the configured parameter, whereas for agent host,port and endpoint, the precedence is given to the environment variable. this would be confusing to me as a user. A good way to test this would be to ensure the precedence is consistent when both environment variables and the constructor is called with all parameters. |
||
self._collector = None | ||
|
||
@property | ||
|
@@ -141,21 +132,16 @@ def collector(self): | |
if self._collector is not None: | ||
return self._collector | ||
|
||
if self.collector_host_name is None or self.collector_port is None: | ||
if self.collector_endpoint is None: | ||
return None | ||
|
||
thrift_url = "{}://{}:{}{}".format( | ||
self.collector_protocol, | ||
self.collector_host_name, | ||
self.collector_port, | ||
self.collector_endpoint, | ||
) | ||
|
||
auth = None | ||
if self.username is not None and self.password is not None: | ||
auth = (self.username, self.password) | ||
|
||
self._collector = Collector(thrift_url=thrift_url, auth=auth) | ||
self._collector = Collector( | ||
thrift_url=self.collector_endpoint, auth=auth | ||
) | ||
return self._collector | ||
|
||
def export(self, spans): | ||
|
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.
Isn't the default supposed to be
http://localhost:14250
?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.
I think in this case it shouldn't. Previously
collector_endpoint
was related to/api/traces?format=jaeger.thrift
and it was just a part of complete endpoint address. If we setcollector_endpoint
default then it will be used overagent
which was default implementation.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.
Any reason why the specs would have that is the default value then?
I'm also not sure what you mean by the above statement.
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.
I just want to say that in previous implementation
collector
by default was not used andcollector_endpoint
was just an end part of complete URL. In current implementation there's no need to specify host,port and endpoint separately because they will be provided by one environment variable calledOTEL_EXPORTER_JAEGER_ENDPOINT
coming from otel specification.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.
Correct, I am asking whether the
collector_endpoint
value should default tohttp://localhost:14250
as defined by the spec if the value is not explicitly passed into the exporter AND the environment variable is not configured.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.
According to our discussion on the Gitter. We've decided to do not set the
collector_endpoint
default because it is causing not entirely clear path of the used protocol to export spans. Separate issue and PR should be created for this purpose.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.
Hmm, looking at java's implementation, it looks like they only have the concept of the collector endpoint. This leads me to believe that we should still have the default value for
collector_endpoint
, and if there's anything that errors with the creation of theCollector
, then we would use the AgentClientUDP instead. So we should still set the default value tohttp://localhost:14250
.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.
@codeboten @rydzykje
Thoughts on this?
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.
Looking at the code, I don't know if there's any scenario where the
.constructor
property would returnNone
if the default ofcollector_endpoint
is always set. Based on the jaeger docs, it would appear that only the agent configuration is usually set by default, and a collector endpoint would have to be explicitly set:I wonder if the spec should default to not having a collector endpoint. You're right @lzchen that other implementations only have the concept of an endpoint, go does the same thing.
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.
I would be ok to go ahead with this implementation and raise an issue with the spec about this.