Make JAEGER_ENDPOINT more priority over JAEGER_AGENT_XXX #342
Conversation
Signed-off-by: Eundoo Song <[email protected]>
config/config_env.go
Outdated
| if e := os.Getenv(envAgentPort); e != "" { | ||
| if ep != "" { | ||
| return nil, errors.Errorf("cannot set env vars %s and %s together", envAgentPort, envEndpoint) | ||
| rc.CollectorEndpoint = fmt.Sprintf("%s", u) |
There was a problem hiding this comment.
should use String() instead of fmt.Sprintf
Codecov Report
@@ Coverage Diff @@
## master #342 +/- ##
==========================================
- Coverage 87.21% 87.19% -0.02%
==========================================
Files 54 54
Lines 3003 2999 -4
==========================================
- Hits 2619 2615 -4
Misses 272 272
Partials 112 112
Continue to review full report at Codecov.
|
Signed-off-by: Eundoo Song <[email protected]>
eundoosong
force-pushed
the
fix_inconsistent_jaeger_endpoint
branch
from
d358f81 to
e4b1e58
Compare
config/config_env.go
Outdated
| if user != "" && pswd == "" || user == "" && pswd != "" { | ||
| return nil, errors.Errorf("you must set %s and %s env vars together", envUser, envPassword) | ||
| // the side effect of this is that we are building the default value, even if none of the env vars | ||
| // were not explicitly passed |
There was a problem hiding this comment.
nit: remove "not" (double negative)
Signed-off-by: Eundoo Song <[email protected]>
| { | ||
| envVar: envAgentHost, | ||
| value: "user", | ||
| err: fmt.Sprintf("cannot set env vars %s and %s together", envAgentHost, envEndpoint), |
There was a problem hiding this comment.
this error is no longer tested?
There was a problem hiding this comment.
Yes, we don't need that error any more.
With this PR, if both envEndpoint and envAgentHost are set, envEndpoint will be only used.
config/config_env.go
Outdated
| if user != "" && pswd == "" || user == "" && pswd != "" { | ||
| return nil, errors.Errorf("you must set %s and %s env vars together", envUser, envPassword) | ||
| // the side effect of this is that we are building the default value, even if none of the env vars | ||
| // were explicitly passed |
There was a problem hiding this comment.
let's remove this comment, II don't know what it is trying to say, only confuses people
There was a problem hiding this comment.
Ok let me remove.
It is there by original author.
Signed-off-by: Eundoo Song <[email protected]>
|
Thanks! |
Signed-off-by: Eundoo Song [email protected]
Which problem is this PR solving?
If the two config options(JAEGER_ENDPOINT, JAEGER_AGENT_XXX) are set, the traces are sent to the endpoint using JAEGER_ENDPOINT, making the JAEGER_AGENT_XXX vars ineffective.
This is already implemented and documented in java/node client.
Also, go client doc also explains the same
But the code just returns an error, which is different.
IMO, there is no reason just for go-client to be different. should be same.
Short description of the changes