Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Fix loading of sampling endpoint address from JAEGER_SAMPLING_ENDPOINT environment variable #200

Merged
merged 4 commits into from
Jan 29, 2020

Conversation

ecourreges-orange
Copy link
Contributor

fix conversion to double of JAEGER_SAMPLER_PARAM
rename env var JAEGER_SAMPLER_MANAGER_HOST_PORT to JAEGER_SAMPLER_SERVER_URL
properly load JAEGER_SAMPLER_SERVER_URL

Signed-off-by: CI-Bot for Emmanuel Courreges [email protected]

fix conversion to double of JAEGER_SAMPLER_PARAM
rename env var JAEGER_SAMPLER_MANAGER_HOST_PORT to JAEGER_SAMPLER_SERVER_URL
properly load JAEGER_SAMPLER_SERVER_URL

Signed-off-by: CI-Bot for Emmanuel Courreges <[email protected]>
@yurishkuro
Copy link
Member

JAEGER_SAMPLER_MANAGER_HOST_PORT to JAEGER_SAMPLER_SERVER_URL

what is the motivation for this?

@AppVeyorBot
Copy link

Build jaeger-client-cpp 78 completed (commit 9dbc84b6a3 by @)

@ecourreges-orange
Copy link
Contributor Author

JAEGER_SAMPLER_MANAGER_HOST_PORT to JAEGER_SAMPLER_SERVER_URL

what is the motivation for this?

In the yaml configuration it is named samplingServerURL, and the env variables for samplers started with JAEGER_SAMPLER, so I decided to rename it like that for consistency (and not JAEGER_SAMPLER_SAMPLING_SERVER_URL which is also redundant). Also it is really a URL and not a host:port.
In any case that env var was not used by the code, only declared, so I don't think the renaming will break anyone.
However if you prefer the term "manager", then it's the yaml that would need to be changed instead, with more impact to users as yaml used to be the only way of loading conf.

@AppVeyorBot
Copy link

Build jaeger-client-cpp 79 completed (commit a14911fc41 by @)

@AppVeyorBot
Copy link

Build jaeger-client-cpp 80 completed (commit b2766e7eaa by @)

Signed-off-by: CI-Bot for Emmanuel Courreges <[email protected]>
/ or /whatever does not work for this driver: the agent answers slightly differently with a numerical strategyType

Signed-off-by: CI-Bot for Emmanuel Courreges <[email protected]>
@AppVeyorBot
Copy link

Build jaeger-client-cpp 81 completed (commit b2766e7eaa by @)

@AppVeyorBot
Copy link

Build jaeger-client-cpp 82 completed (commit e39fee5c02 by @)

@AppVeyorBot
Copy link

Build jaeger-client-cpp 83 completed (commit e39fee5c02 by @)

@yurishkuro
Copy link
Member

We had a discussion (jaegertracing/jaeger#1971 (comment)) about the naming, and the preferred name is JAEGER_SAMPLING_ENDPOINT.

@ecourreges-orange
Copy link
Contributor Author

Done renaming the env var JAEGER_SAMPLING_ENDPOINT as requested.

@AppVeyorBot
Copy link

Build jaeger-client-cpp 85 completed (commit ebbfd97987 by @)

@@ -36,11 +36,15 @@ void Config::fromEnv()
const auto param = utils::EnvVariable::getStringVariable(kJAEGER_SAMPLER_PARAM_ENV_PROP);
if (!param.empty()) {
std::istringstream iss(param);
int paramVal = 0;
double paramVal = 0;
if (iss >> paramVal) {
_param = paramVal;
}
Copy link
Member

Choose a reason for hiding this comment

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

Probably should log a warning if value cannot be parsed. But it's not in scope for this PR.

@yurishkuro yurishkuro changed the title fix configuration from environment variables Fix loading of sampling endpoint address from JAEGER_SAMPLING_ENDPOINT environment variable Jan 29, 2020
@yurishkuro yurishkuro merged commit 9e1b39c into jaegertracing:master Jan 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants