Replace JAEGER_SAMPLER_MANAGER_HOST_PORT with JAEGER_SAMPLING_ENDPOINT (

#165)

* Replace JAEGER_SAMPLER_MANAGER_HOST_PORT with JAEGER_SAMPLING_ENDPOINT
* Readded support for deprecated JAEGER_SAMPLER_MANAGER_HOST_PORT
* Adjusted tests for real-world sampler results

README.md

@@ -79,8 +79,9 @@ JAEGER_REPORTER_LOG_SPANS | no | Whether the reporter should also log the spans
 JAEGER_REPORTER_MAX_QUEUE_SIZE | no | The reporter's maximum queue size
 JAEGER_REPORTER_FLUSH_INTERVAL | no | The reporter's flush interval (ms)
 JAEGER_SAMPLER_TYPE | no | The sampler type
-JAEGER_SAMPLER_PARAM | no | The sampler parameter (number)
-JAEGER_SAMPLER_MANAGER_HOST_PORT | no | The host name and port when using the remote controlled sampler
+JAEGER_SAMPLER_PARAM | no | The sampler parameter (double)
+JAEGER_SAMPLER_MANAGER_HOST_PORT | no | (DEPRECATED) The host name and port when using the remote controlled sampler
+JAEGER_SAMPLING_ENDPOINT | no | The url for the remote sampling conf when using sampler type remote. Default is http://127.0.0.1:5778/sampling
 JAEGER_TAGS | no | 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
 JAEGER_TRACEID_128BIT | no | Whether to use 128bit TraceID instead of 64bit
 

src/Jaeger/Configuration.cs

@@ -87,6 +87,11 @@ public class Configuration
         /// </summary>
         public const string JaegerSamplerManagerHostPort = JaegerPrefix + "SAMPLER_MANAGER_HOST_PORT";
 
+        /// <summary>
+        /// The url for the remote sampling conf when using sampler type remote.
+        /// </summary>
+        public const string JaegerSamplingEndpoint = JaegerPrefix + "SAMPLING_ENDPOINT";
+
         /// <summary>
         /// The service name.
         /// </summary>
@@ -161,7 +166,7 @@ public static Configuration FromIConfiguration(ILoggerFactory loggerFactory, ICo
         {
             ILogger logger = loggerFactory.CreateLogger<Configuration>();
 
-            return new Configuration(GetProperty(JaegerServiceName, configuration), loggerFactory)
+            return new Configuration(GetProperty(JaegerServiceName, logger, configuration), loggerFactory)
                 .WithTracerTags(TracerTagsFromIConfiguration(logger, configuration))
                 .WithTraceId128Bit(GetPropertyAsBool(JaegerTraceId128Bit, logger, configuration).GetValueOrDefault(false))
                 .WithReporter(ReporterConfiguration.FromIConfiguration(loggerFactory, configuration))
@@ -312,10 +317,16 @@ public class SamplerConfiguration
 
             /// <summary>
             /// HTTP host:port of the sampling manager that can provide sampling strategy to this service.
-            /// Optional.
             /// </summary>
+            [Obsolete("Please use SamplingEndpoint instead!")]
             public string ManagerHostPort { get; private set; }
 
+            /// <summary>
+            /// The URL of the sampling manager that can provide sampling strategy to this service.
+            /// Optional.
+            /// </summary>
+            public string SamplingEndpoint { get; private set; }
+
             public SamplerConfiguration(ILoggerFactory loggerFactory)
             {
                 _loggerFactory = loggerFactory;
@@ -327,11 +338,14 @@ public SamplerConfiguration(ILoggerFactory loggerFactory)
             public static SamplerConfiguration FromIConfiguration(ILoggerFactory loggerFactory, IConfiguration configuration)
             {
                 ILogger logger = loggerFactory.CreateLogger<Configuration>();
-
+
+#pragma warning disable CS0618 // Supress warning on obsolete method: WithManagerHostPort
                 return new SamplerConfiguration(loggerFactory)
-                    .WithType(GetProperty(JaegerSamplerType, configuration))
+                    .WithType(GetProperty(JaegerSamplerType, logger, configuration))
                     .WithParam(GetPropertyAsDouble(JaegerSamplerParam, logger, configuration))
-                    .WithManagerHostPort(GetProperty(JaegerSamplerManagerHostPort, configuration));
+                    .WithManagerHostPort(GetProperty(JaegerSamplerManagerHostPort, logger, configuration, JaegerSamplingEndpoint))
+                    .WithSamplingEndpoint(GetProperty(JaegerSamplingEndpoint, logger, configuration));
+#pragma warning restore CS0618 // Supress warning on obsolete method: WithManagerHostPort
             }
 
             /// <summary>
@@ -347,9 +361,12 @@ public static SamplerConfiguration FromEnv(ILoggerFactory loggerFactory)
 
             public virtual ISampler GetSampler(string serviceName, IMetrics metrics)
             {
+#pragma warning disable CS0618 // Supress warning on obsolete property: ManagerHostPort
                 string samplerType = StringOrDefault(Type, RemoteControlledSampler.Type);
                 double samplerParam = Param.GetValueOrDefault(ProbabilisticSampler.DefaultSamplingProbability);
                 string hostPort = StringOrDefault(ManagerHostPort, HttpSamplingManager.DefaultHostPort);
+                string samplingEndpoint = StringOrDefault(SamplingEndpoint, "http://" + hostPort);
+#pragma warning disable CS0618 // Supress warning on obsolete property: ManagerHostPort
 
                 switch (samplerType)
                 {
@@ -362,7 +379,7 @@ public virtual ISampler GetSampler(string serviceName, IMetrics metrics)
                     case RemoteControlledSampler.Type:
                         return new RemoteControlledSampler.Builder(serviceName)
                             .WithLoggerFactory(_loggerFactory)
-                            .WithSamplingManager(new HttpSamplingManager(hostPort))
+                            .WithSamplingManager(new HttpSamplingManager(samplingEndpoint))
                             .WithInitialSampler(new ProbabilisticSampler(samplerParam))
                             .WithMetrics(metrics)
                             .Build();
@@ -383,11 +400,18 @@ public SamplerConfiguration WithParam(double? param)
                 return this;
             }
 
+            [Obsolete("Use WithSamplingEndpoint instead!")]
             public SamplerConfiguration WithManagerHostPort(string managerHostPort)
             {
                 ManagerHostPort = managerHostPort;
                 return this;
             }
+
+            public SamplerConfiguration WithSamplingEndpoint(string samplingEndpoint)
+            {
+                SamplingEndpoint = samplingEndpoint;
+                return this;
+            }
         }
 
         /// <summary>
@@ -412,7 +436,7 @@ public static CodecConfiguration FromIConfiguration(ILoggerFactory loggerFactory
                 ILogger logger = loggerFactory.CreateLogger<Configuration>();
 
                 CodecConfiguration codecConfiguration = new CodecConfiguration(loggerFactory);
-                string propagation = GetProperty(JaegerPropagation, configuration);
+                string propagation = GetProperty(JaegerPropagation, logger, configuration);
                 if (propagation != null)
                 {
                     foreach (string format in propagation.Split(','))
@@ -706,13 +730,13 @@ public static SenderConfiguration FromIConfiguration(ILoggerFactory loggerFactor
             {
                 ILogger logger = loggerFactory.CreateLogger<Configuration>();
 
-                string agentHost = GetProperty(JaegerAgentHost, configuration);
+                string agentHost = GetProperty(JaegerAgentHost, logger, configuration);
                 int? agentPort = GetPropertyAsInt(JaegerAgentPort, logger, configuration);
 
-                string collectorEndpoint = GetProperty(JaegerEndpoint, configuration);
-                string authToken = GetProperty(JaegerAuthToken, configuration);
-                string authUsername = GetProperty(JaegerUser, configuration);
-                string authPassword = GetProperty(JaegerPassword, configuration);
+                string collectorEndpoint = GetProperty(JaegerEndpoint, logger, configuration);
+                string authToken = GetProperty(JaegerAuthToken, logger, configuration);
+                string authUsername = GetProperty(JaegerUser, logger, configuration);
+                string authPassword = GetProperty(JaegerPassword, logger, configuration);
 
                 return new SenderConfiguration(loggerFactory)
                     .WithAgentHost(agentHost)
@@ -740,14 +764,20 @@ private static string StringOrDefault(string value, string defaultValue)
             return value != null && value.Length > 0 ? value : defaultValue;
         }
 
-        private static string GetProperty(string name, IConfiguration configuration)
+        private static string GetProperty(string name, ILogger logger, IConfiguration configuration, string replacedBy = null)
         {
-            return configuration[name];
+            var value = configuration[name];
+            if (replacedBy != null && value != null)
+            {
+                logger.LogWarning($"The entry {name} is obsolete. Use {replacedBy} instead!");
+            }
+
+            return value;
         }
 
         private static int? GetPropertyAsInt(string name, ILogger logger, IConfiguration configuration)
         {
-            string value = GetProperty(name, configuration);
+            string value = GetProperty(name, logger, configuration);
             if (!string.IsNullOrEmpty(value))
             {
                 if (int.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture, out int intValue))
@@ -764,7 +794,7 @@ private static string GetProperty(string name, IConfiguration configuration)
 
         private static double? GetPropertyAsDouble(string name, ILogger logger, IConfiguration configuration)
         {
-            string value = GetProperty(name, configuration);
+            string value = GetProperty(name, logger, configuration);
             if (!string.IsNullOrEmpty(value))
             {
                 if (double.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture, out double doubleValue))
@@ -795,7 +825,7 @@ private static string GetProperty(string name, IConfiguration configuration)
         /// </summary>
         private static bool? GetPropertyAsBool(string name, ILogger logger, IConfiguration configuration)
         {
-            string value = GetProperty(name, configuration);
+            string value = GetProperty(name, logger, configuration);
             if (!string.IsNullOrEmpty(value))
             {
                 if (string.Equals(value, "1", StringComparison.Ordinal))
@@ -822,7 +852,7 @@ private static string GetProperty(string name, IConfiguration configuration)
         private static Dictionary<string, string> TracerTagsFromIConfiguration(ILogger logger, IConfiguration configuration)
         {
             Dictionary<string, string> tracerTagMaps = null;
-            string tracerTags = GetProperty(JaegerTags, configuration);
+            string tracerTags = GetProperty(JaegerTags, logger, configuration);
             if (!string.IsNullOrEmpty(tracerTags))
             {
                 string[] tags = tracerTags.Split(',');
@@ -835,7 +865,7 @@ private static Dictionary<string, string> TracerTagsFromIConfiguration(ILogger l
                         {
                             tracerTagMaps = new Dictionary<string, string>();
                         }
-                        tracerTagMaps[tagValue[0].Trim()] = ResolveValue(tagValue[1].Trim(), configuration);
+                        tracerTagMaps[tagValue[0].Trim()] = ResolveValue(tagValue[1].Trim(), logger, configuration);
                     }
                     else
                     {
@@ -846,14 +876,14 @@ private static Dictionary<string, string> TracerTagsFromIConfiguration(ILogger l
             return tracerTagMaps;
         }
 
-        private static string ResolveValue(string value, IConfiguration configuration)
+        private static string ResolveValue(string value, ILogger logger, IConfiguration configuration)
         {
             if (value.StartsWith("${") && value.EndsWith("}"))
             {
                 string[] kvp = value.Substring(2, value.Length - 3).Split(':');
                 if (kvp.Length > 0)
                 {
-                    string propertyValue = GetProperty(kvp[0].Trim(), configuration);
+                    string propertyValue = GetProperty(kvp[0].Trim(), logger, configuration);
                     if (propertyValue == null && kvp.Length > 1)
                     {
                         propertyValue = kvp[1].Trim();

src/Jaeger/Samplers/HttpSamplingManager.cs

@@ -8,20 +8,28 @@ namespace Jaeger.Samplers
 {
     public class HttpSamplingManager : ISamplingManager
     {
-        public const string DefaultHostPort = "localhost:5778";
+        public const string DefaultHostPort = "127.0.0.1:5778";
+        public const string DefaultEndpoint = "http://" + DefaultHostPort + "/sampling";
 
         private readonly IHttpClient _httpClient;
-        private readonly string _hostPort;
+        private readonly string _endpoint;
 
-        public HttpSamplingManager(string hostPort = DefaultHostPort)
-            : this(new DefaultHttpClient(), hostPort)
+        public HttpSamplingManager(string endpoint = DefaultEndpoint)
+            : this(new DefaultHttpClient(), endpoint)
         {
         }
 
-        public HttpSamplingManager(IHttpClient httpClient, string hostPort = DefaultHostPort)
+        public HttpSamplingManager(IHttpClient httpClient, string endpoint = DefaultEndpoint)
         {
             _httpClient = httpClient ?? throw new ArgumentNullException(nameof(httpClient));
-            _hostPort = hostPort ?? DefaultHostPort;
+            _endpoint = endpoint ?? DefaultEndpoint;
+
+            // Workaround for obsolete HostPort notation.
+            // UriBuilder needs the schema if host is not an IP and port is given:
+            if (!_endpoint.StartsWith("http://") && !_endpoint.StartsWith("https://"))
+            {
+                _endpoint = "http://" + _endpoint;
+            }
         }
 
         internal SamplingStrategyResponse ParseJson(string json)
@@ -31,15 +39,15 @@ internal SamplingStrategyResponse ParseJson(string json)
 
         public async Task<SamplingStrategyResponse> GetSamplingStrategyAsync(string serviceName)
         {
-            string url = "http://" + _hostPort + "/?service=" + Uri.EscapeDataString(serviceName);
-            string jsonString = await _httpClient.MakeGetRequestAsync(url).ConfigureAwait(false);
+            Uri uri = new UriBuilder(_endpoint) {Query = "service=" + Uri.EscapeDataString(serviceName)}.Uri;
+            string jsonString = await _httpClient.MakeGetRequestAsync(uri.AbsoluteUri).ConfigureAwait(false);
 
             return ParseJson(jsonString);
         }
 
         public override string ToString()
         {
-            return $"{nameof(HttpSamplingManager)}(HostPort={_hostPort})";
+            return $"{nameof(HttpSamplingManager)}(Endpoint={_endpoint})";
         }
     }
 }

test/Jaeger.Tests/ConfigurationTests.cs

@@ -8,9 +8,12 @@
 using Jaeger.Metrics;
 using Jaeger.Samplers;
 using Jaeger.Senders;
+using Jaeger.Util;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Configuration;
+using NSubstitute;
+using NSubstitute.ReceivedExtensions;
 using OpenTracing;
 using OpenTracing.Noop;
 using OpenTracing.Propagation;
@@ -49,6 +52,7 @@ private void ClearProperties()
             ClearProperty(Configuration.JaegerSamplerType);
             ClearProperty(Configuration.JaegerSamplerParam);
             ClearProperty(Configuration.JaegerSamplerManagerHostPort);
+            ClearProperty(Configuration.JaegerSamplingEndpoint);
             ClearProperty(Configuration.JaegerServiceName);
             ClearProperty(Configuration.JaegerTags);
             ClearProperty(Configuration.JaegerTraceId128Bit);
@@ -527,6 +531,22 @@ public void TestRateLimitingSampler()
             Assert.True(sampler is RateLimitingSampler);
         }
 
+        [Fact]
+        public void TestDeprecatedSamplerManagerHostPort()
+        {
+            ILoggerFactory loggerFactory = Substitute.For<ILoggerFactory>();
+            ILogger logger = Substitute.For<ILogger>();
+            loggerFactory.CreateLogger<Configuration>().Returns(logger);
+
+            SetProperty(Configuration.JaegerSamplerManagerHostPort, HttpSamplingManager.DefaultHostPort);
+            SamplerConfiguration samplerConfiguration = SamplerConfiguration.FromEnv(loggerFactory);
+            ISampler sampler = samplerConfiguration.GetSampler("name",
+                new MetricsImpl(NoopMetricsFactory.Instance));
+            Assert.True(sampler is RemoteControlledSampler);
+            loggerFactory.Received(1).CreateLogger<Configuration>();
+            logger.Received(1).Log(LogLevel.Warning, Arg.Any<EventId>(), Arg.Any<object>(), null, Arg.Any<Func<object, Exception, string>>());
+        }
+
         internal class TestTextMap : ITextMap
         {
             private Dictionary<string, string> _values = new Dictionary<string, string>();

test/Jaeger.Tests/Samplers/HttpSamplingManagerTests.cs

@@ -22,14 +22,25 @@ public class HttpSamplingManagerTests
         public HttpSamplingManagerTests()
         {
             _httpClient = Substitute.For<IHttpClient>();
-            _undertest = new HttpSamplingManager(_httpClient, "www.example.com");
+            _undertest = new HttpSamplingManager(_httpClient, "http://www.example.com/sampling");
+        }
+
+        [Fact]
+        public async Task TestAllowHostPortSyntax()
+        {
+            var instance = new HttpSamplingManager(_httpClient, "example.com:80");
+            _httpClient.MakeGetRequestAsync("http://example.com/?service=clairvoyant")
+                .Returns("{\"strategyType\":\"PROBABILISTIC\",\"probabilisticSampling\":{\"samplingRate\":0.001},\"rateLimitingSampling\":null}");
+
+            SamplingStrategyResponse response = await instance.GetSamplingStrategyAsync("clairvoyant");
+            Assert.NotNull(response.ProbabilisticSampling);
         }
 
         [Fact]
         public async Task TestGetSamplingStrategy()
         {
-            _httpClient.MakeGetRequestAsync("http://www.example.com/?service=clairvoyant")
-               .Returns("{\"strategyType\":0,\"probabilisticSampling\":{\"samplingRate\":0.001},\"rateLimitingSampling\":null}");
+            _httpClient.MakeGetRequestAsync("http://www.example.com/sampling?service=clairvoyant")
+               .Returns("{\"strategyType\":\"PROBABILISTIC\",\"probabilisticSampling\":{\"samplingRate\":0.001},\"rateLimitingSampling\":null}");
 
             SamplingStrategyResponse response = await _undertest.GetSamplingStrategyAsync("clairvoyant");
             Assert.NotNull(response.ProbabilisticSampling);
@@ -38,7 +49,7 @@ public async Task TestGetSamplingStrategy()
         [Fact]
         public async Task TestGetSamplingStrategyError()
         {
-            _httpClient.MakeGetRequestAsync("http://www.example.com/?service=")
+            _httpClient.MakeGetRequestAsync("http://www.example.com/sampling?service=")
                 .Returns(new Func<CallInfo, string>(_ => { throw new InvalidOperationException(); }));
 
             await Assert.ThrowsAsync<InvalidOperationException>(() => _undertest.GetSamplingStrategyAsync(""));
@@ -87,7 +98,7 @@ public void TestParsePerOperationSampling()
         [Fact]
         public async Task TestDefaultConstructor()
         {
-            _httpClient.MakeGetRequestAsync("http://localhost:5778/?service=name")
+            _httpClient.MakeGetRequestAsync("http://127.0.0.1:5778/sampling?service=name")
                 .Returns(new Func<CallInfo, string>(_ => { throw new InvalidOperationException(); }));
 
             HttpSamplingManager httpSamplingManager = new HttpSamplingManager(_httpClient);

test/Jaeger.Tests/Samplers/Resources/per_operation_sampling.json

@@ -21,5 +21,5 @@
     "samplingRate": 0.001
   },
   "rateLimitingSampling": null,
-  "strategyType": 0
+  "strategyType": "PROBABILISTIC"
 }

test/Jaeger.Tests/Samplers/Resources/probabilistic_sampling.json

@@ -1,7 +1,7 @@
 {
-  "strategyType": 0,
-  "probabilisticSampling": {
-    "samplingRate": 0.01
-  },
-  "rateLimitingSampling": null
+    "strategyType": "PROBABILISTIC",
+    "probabilisticSampling": {
+        "samplingRate": 0.01
+    },
+    "rateLimitingSampling": null
 }

test/Jaeger.Tests/Samplers/Resources/ratelimiting_sampling.json

@@ -1,7 +1,7 @@
 {
-  "strategyType": 1,
-  "probabilisticSampling": null,
-  "rateLimitingSampling": {
-    "maxTracesPerSecond": 2.1
-  }
+    "strategyType": "RATE_LIMITING",
+    "probabilisticSampling": null,
+    "rateLimitingSampling": {
+        "maxTracesPerSecond": 2.1
+    }
 }