From c40de22a779c4ab8b3d1a71dbe846c514c9819db Mon Sep 17 00:00:00 2001 From: CI-Bot for Emmanuel Courreges Date: Fri, 24 Jan 2020 16:26:47 +0100 Subject: [PATCH 1/3] fix configuration from environment variables 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 --- README.md | 4 ++-- src/jaegertracing/ConfigTest.cpp | 10 ++++++---- src/jaegertracing/samplers/Config.cpp | 6 +++++- src/jaegertracing/samplers/Config.h | 2 +- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 9347e054..944f6cf2 100644 --- a/README.md +++ b/README.md @@ -103,8 +103,8 @@ JAEGER_REPORTER_LOG_SPANS | Whether the reporter should also log the spans JAEGER_REPORTER_MAX_QUEUE_SIZE | The reporter's maximum queue size JAEGER_REPORTER_FLUSH_INTERVAL | The reporter's flush interval (ms) JAEGER_SAMPLER_TYPE | The [sampler type](https://www.jaegertracing.io/docs/latest/sampling/#client-sampling-configuration) -JAEGER_SAMPLER_PARAM | The sampler parameter (number) -JAEGER_SAMPLER_MANAGER_HOST_PORT | The host name and port when using the remote controlled sampler +JAEGER_SAMPLER_PARAM | The sampler parameter (double) +JAEGER_SAMPLER_SERVER_URL | The url for the remote conf when using sampler type remote. Default is http://127.0.0.1:5778/sampling JAEGER_TAGS | A comma separated list of `name = value` tracer level tags, which get added to all reported spans. The value can also refer to an environment variable using the format `${envVarName:default}`, where the `:default` is optional, and identifies a value to be used if the environment variable cannot be found ## License diff --git a/src/jaegertracing/ConfigTest.cpp b/src/jaegertracing/ConfigTest.cpp index 5392fe9c..f0054acc 100644 --- a/src/jaegertracing/ConfigTest.cpp +++ b/src/jaegertracing/ConfigTest.cpp @@ -147,8 +147,9 @@ TEST(Config, testFromEnv) setEnv("JAEGER_REPORTER_FLUSH_INTERVAL", "45"); setEnv("JAEGER_REPORTER_LOG_SPANS", "true"); - setEnv("JAEGER_SAMPLER_PARAM", "33"); - setEnv("JAEGER_SAMPLER_TYPE", "const"); + setEnv("JAEGER_SAMPLER_TYPE", "remote"); + setEnv("JAEGER_SAMPLER_PARAM", "0.33"); + setEnv("JAEGER_SAMPLER_SERVER_URL", "http://myagent:1234"); setEnv("JAEGER_SERVICE_NAME", "AService"); setEnv("JAEGER_TAGS", "hostname=foobar,my.app.version=4.5.6"); @@ -163,8 +164,9 @@ TEST(Config, testFromEnv) config.reporter().bufferFlushInterval()); ASSERT_EQ(true, config.reporter().logSpans()); - ASSERT_EQ(33., config.sampler().param()); - ASSERT_EQ(std::string("const"), config.sampler().type()); + ASSERT_EQ(std::string("remote"), config.sampler().type()); + ASSERT_EQ(0.33, config.sampler().param()); + ASSERT_EQ(std::string("http://myagent:1234"), config.sampler().samplingServerURL()); ASSERT_EQ(std::string("AService"), config.serviceName()); diff --git a/src/jaegertracing/samplers/Config.cpp b/src/jaegertracing/samplers/Config.cpp index 33ea092a..1aab8774 100644 --- a/src/jaegertracing/samplers/Config.cpp +++ b/src/jaegertracing/samplers/Config.cpp @@ -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; } } + const auto samplingServerURL = utils::EnvVariable::getStringVariable(kJAEGER_SAMPLER_SERVER_URL_ENV_PROP); + if (!samplingServerURL.empty()) { + _samplingServerURL = samplingServerURL; + } } } // namespace samplers diff --git a/src/jaegertracing/samplers/Config.h b/src/jaegertracing/samplers/Config.h index 58df7f99..7a0adab8 100644 --- a/src/jaegertracing/samplers/Config.h +++ b/src/jaegertracing/samplers/Config.h @@ -47,7 +47,7 @@ class Config { static constexpr auto kJAEGER_SAMPLER_TYPE_ENV_PROP = "JAEGER_SAMPLER_TYPE"; static constexpr auto kJAEGER_SAMPLER_PARAM_ENV_PROP = "JAEGER_SAMPLER_PARAM"; - static constexpr auto kJAEGER_SAMPLER_MANAGER_HOST_PORT_ENV_PROP = "JAEGER_SAMPLER_MANAGER_HOST_PORT"; + static constexpr auto kJAEGER_SAMPLER_SERVER_URL_ENV_PROP = "JAEGER_SAMPLER_SERVER_URL"; static Clock::duration defaultSamplingRefreshInterval() { From 647e9f60a360fc85f1d6e70d585b0201aed9dd51 Mon Sep 17 00:00:00 2001 From: CI-Bot for Emmanuel Courreges Date: Mon, 27 Jan 2020 14:51:54 +0100 Subject: [PATCH 2/3] proper unit testing of RemotelyControlledSampler Signed-off-by: CI-Bot for Emmanuel Courreges --- src/jaegertracing/samplers/SamplerTest.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/jaegertracing/samplers/SamplerTest.cpp b/src/jaegertracing/samplers/SamplerTest.cpp index 6fb40677..3c082d32 100644 --- a/src/jaegertracing/samplers/SamplerTest.cpp +++ b/src/jaegertracing/samplers/SamplerTest.cpp @@ -331,6 +331,17 @@ TEST(Sampler, testRemotelyControlledSampler) mockAgent->start(); const auto logger = logging::nullLogger(); const auto metrics = metrics::Metrics::makeNullMetrics(); + + // Make sure remote sampling probability is 1 + sampling_manager::thrift::SamplingStrategyResponse config; + config.__set_strategyType( + sampling_manager::thrift::SamplingStrategyType::PROBABILISTIC); + sampling_manager::thrift::ProbabilisticSamplingStrategy probaStrategy; + probaStrategy.__set_samplingRate(1.0); + config.__set_probabilisticSampling(probaStrategy); + mockAgent->addSamplingStrategy("test-service", config); + + // Default probability of 0.5, switches to 1 when downloaded RemotelyControlledSampler sampler( "test-service", "http://" + mockAgent->samplingServerAddress().authority(), @@ -339,6 +350,9 @@ TEST(Sampler, testRemotelyControlledSampler) std::chrono::milliseconds(100), *logger, *metrics); + + // Wait a bit for remote config download to be done + std::this_thread::sleep_for(std::chrono::milliseconds(100)); std::random_device device; std::mt19937_64 rng; rng.seed(device()); @@ -347,7 +361,8 @@ TEST(Sampler, testRemotelyControlledSampler) RemotelyControlledSampler::Clock::now() - startTime) .count() < 1;) { TraceID traceID(rng(), rng()); - sampler.isSampled(traceID, kTestOperationName); + // If probability was 0.5 we could reasonnably assume one of 50 samples fail + ASSERT_TRUE(sampler.isSampled(traceID, kTestOperationName).isSampled()); std::this_thread::sleep_for(std::chrono::milliseconds(20)); } sampler.close(); From cbfb00b288b4440c14ae4ab6ee35a89de6f3cdb9 Mon Sep 17 00:00:00 2001 From: CI-Bot for Emmanuel Courreges Date: Tue, 28 Jan 2020 11:43:59 +0100 Subject: [PATCH 3/3] include /sampling in all samplingServerURL examples to avoid confusion / or /whatever does not work for this driver: the agent answers slightly differently with a numerical strategyType --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 944f6cf2..1799f7d5 100644 --- a/README.md +++ b/README.md @@ -83,7 +83,7 @@ The default sampling collector URL is `http://127.0.0.1:5778/sampling`. Similar ```yml sampler: - samplingServerURL: http://jaeger-agent.local:5778 + samplingServerURL: http://jaeger-agent.local:5778/sampling ``` ### Configuration via Environment