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

* 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 <[email protected]>

* proper unit testing of RemotelyControlledSampler

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

* 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

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

* rename JAEGER_SAMPLER_MANAGER_HOST_PORT into JAEGER_SAMPLING_ENDPOINT as per jaegertracing/jaeger#1971 (comment)

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

Author: Emmanuel Courreges

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_SAMPLING_ENDPOINT | The url for the remote sampling 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

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_SAMPLING_ENDPOINT", "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());
 

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_SAMPLING_ENDPOINT_ENV_PROP);
+    if (!samplingServerURL.empty()) {
+        _samplingServerURL = samplingServerURL;
+    }
 }
 
 }  // namespace samplers

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_SAMPLING_ENDPOINT_ENV_PROP = "JAEGER_SAMPLING_ENDPOINT";
 
     static Clock::duration defaultSamplingRefreshInterval()
     {

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();