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.

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 more environment variables in configuration #323

Merged
merged 3 commits into from Jul 13, 2018

Conversation

danehans
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Previously, the client could only send spans via UDP to the agent.
    This PR adds support for sending spans using HTTP to the collector
    by setting the JAEGER_ENDPOINT env var. Additionally, the HTTP
    transport can be secured through basic auth by setting the
    JAEGER_USER and JAEGER_PASSWORD env vars.

Signed-off-by: Daneyon Hansen danehans@cisco.com

config/config.go Outdated
@@ -369,5 +382,13 @@ func (rc *ReporterConfig) NewReporter(
}

func (rc *ReporterConfig) newTransport() (jaeger.Transport, error) {
var opts transport.HTTPOption
if rc.User != "" && rc.Password != "" {
opts = transport.HTTPBasicAuth(rc.User, rc.Password)

Choose a reason for hiding this comment

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

ineffectual assignment to opts

@codecov
Copy link

codecov bot commented Jul 12, 2018

Codecov Report

Merging #323 into master will decrease coverage by <.01%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
- Coverage   87.15%   87.14%   -0.01%     
==========================================
  Files          54       54              
  Lines        2965     2988      +23     
==========================================
+ Hits         2584     2604      +20     
- Misses        269      272       +3     
  Partials      112      112
Impacted Files Coverage Δ
config/config_env.go 100% <100%> (ø) ⬆️
config/config.go 93.19% <62.5%> (-1.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7e0d47...4024f94. Read the comment docs.

Previously, the client could only send spans via UDP to the agent.
This PR adds support for sending spans using HTTP to the collector
by setting the JAEGER_ENDPOINT env var. Additionally, the HTTP
transport can be secured through basic auth by setting the
JAEGER_USER and JAEGER_PASSWORD env vars.

Fixes: jaegertracing/jaeger#920

Signed-off-by: Daneyon Hansen <danehans@cisco.com>
@yurishkuro yurishkuro changed the title Adds HTTP Collector Support Support more environment variables in configuration Jul 13, 2018
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks! I'll let @black-adder to review as well.

Copy link
Contributor

@black-adder black-adder left a comment

Choose a reason for hiding this comment

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

thanks for the contribution! just a couple of nits

@@ -157,11 +161,17 @@ func reporterConfigFromEnv() (*ReporterConfig, error) {

host := jaeger.DefaultUDPSpanServerHost
if e := os.Getenv(envAgentHost); e != "" {
if ep := os.Getenv(envEndpoint); ep != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about we read the envEndpoint once and use ep in all the checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

rc.CollectorEndpoint = fmt.Sprintf("%s", u)
}

if e := os.Getenv(envUser); e != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use xor here instead and combine everything into 1 check? Albeit, it might not be a worthwhile optimization given this only happens during initialization but it's always fun to use xor every now and again.

Signed-off-by: Daneyon Hansen <danehans@cisco.com>
pswd := os.Getenv(envPassword)
if user != "" && pswd == "" || user == "" && pswd != "" {
return nil, errors.Errorf("you must set %s and %s env vars together", envUser, envPassword)
} else {

Choose a reason for hiding this comment

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

if block ends with a return statement, so drop this else and outdent its block


user := os.Getenv(envUser)
pswd := os.Getenv(envPassword)
if user != "" && pswd == "" || user == "" && pswd != "" {

Choose a reason for hiding this comment

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

File is not gofmt-ed with -s

Signed-off-by: Daneyon Hansen <danehans@cisco.com>
@black-adder black-adder merged commit 252d853 into jaegertracing:master Jul 13, 2018
@black-adder
Copy link
Contributor

@danehans thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants