-
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
Merged
codeboten
merged 18 commits into
open-telemetry:master
from
rydzykje:rydzykje-support_env_var_conf_jaeger
Oct 15, 2020
Merged
Changes from 6 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
d180fd6
Introduce OTEL Environment Variables support
rydzykje f7847dd
Update and add OTEL Envs test
rydzykje 0913ee0
Apply lint comments
rydzykje 79cee16
Reverse setup of username and password
rydzykje 2ff3c9f
Update CHANGELOG.md
rydzykje 3d8f333
Add configuration information
rydzykje a6bceb1
Merge branch 'master' into rydzykje-support_env_var_conf_jaeger
rydzykje f98c6db
Merge branch 'master' into rydzykje-support_env_var_conf_jaeger
rydzykje d466113
Introduce Configuration
rydzykje 50a9312
Update tests
rydzykje ff5ae8d
Merge branch 'master' into rydzykje-support_env_var_conf_jaeger
rydzykje 952549c
Swap username and password with env vars
rydzykje b3c0da6
Introduce parameter setter, constructor parameters over env vars
rydzykje 8cfc4ed
Merge branch 'master' into rydzykje-support_env_var_conf_jaeger
36e9819
Merge branch 'master' into rydzykje-support_env_var_conf_jaeger
d536a91
Merge branch 'master' into rydzykje-support_env_var_conf_jaeger
lzchen 406486e
Merge branch 'master' into rydzykje-support_env_var_conf_jaeger
47ab344
Merge branch 'master' into rydzykje-support_env_var_conf_jaeger
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.