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

Replace JAEGER_SAMPLER_MANAGER_HOST_PORT with JAEGER_SAMPLING_ENDPOINT #165

Merged
merged 3 commits into from Mar 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions README.md
Expand Up @@ -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

Expand Down
72 changes: 51 additions & 21 deletions src/Jaeger/Configuration.cs
Expand Up @@ -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";
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// The service name.
/// </summary>
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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;
Expand All @@ -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>
Expand All @@ -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)
{
Expand All @@ -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();
Expand All @@ -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>
Expand All @@ -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(','))
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
Expand All @@ -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))
Expand Down Expand Up @@ -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))
Expand All @@ -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(',');
Expand All @@ -835,7 +865,7 @@ private static string GetProperty(string name, IConfiguration configuration)
{
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
{
Expand All @@ -846,14 +876,14 @@ private static string GetProperty(string name, IConfiguration configuration)
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();
Expand Down
26 changes: 17 additions & 9 deletions src/Jaeger/Samplers/HttpSamplingManager.cs
Expand Up @@ -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)
Expand All @@ -31,15 +39,15 @@ internal SamplingStrategyResponse ParseJson(string json)

public async Task<SamplingStrategyResponse> GetSamplingStrategyAsync(string serviceName)
{
string url = "http://" + _hostPort + "/?service=" + Uri.EscapeDataString(serviceName);
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, this wasn't using the /sampling endpoint previously? The results from / endpoint are using legacy Thrift-to-JSON conversion that used numbers for enums, instead of strings. Have you tested this change with an agent? I don't think crossdock tests cover the sampling endpoint.

Copy link
Collaborator Author

@Falco20019 Falco20019 Mar 2, 2020

Choose a reason for hiding this comment

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

I checked the code again. The enum is not checked at all, therefore it's not making problems. C# is deserializing the following data structure as property. So it's looking for the field names instead of the enum. But I will adjust the test cases to make sure this reflects the real situation.

If there are no more open points, please approve :)

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})";
}
}
}
20 changes: 20 additions & 0 deletions test/Jaeger.Tests/ConfigurationTests.cs
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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>();
Expand Down
21 changes: 16 additions & 5 deletions test/Jaeger.Tests/Samplers/HttpSamplingManagerTests.cs
Expand Up @@ -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);
Expand All @@ -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(""));
Expand Down Expand Up @@ -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);
Expand Down
Expand Up @@ -21,5 +21,5 @@
"samplingRate": 0.001
},
"rateLimitingSampling": null,
"strategyType": 0
"strategyType": "PROBABILISTIC"
}
@@ -1,7 +1,7 @@
{
"strategyType": 0,
"probabilisticSampling": {
"samplingRate": 0.01
},
"rateLimitingSampling": null
"strategyType": "PROBABILISTIC",
"probabilisticSampling": {
"samplingRate": 0.01
},
"rateLimitingSampling": null
}
@@ -1,7 +1,7 @@
{
"strategyType": 1,
"probabilisticSampling": null,
"rateLimitingSampling": {
"maxTracesPerSecond": 2.1
}
"strategyType": "RATE_LIMITING",
"probabilisticSampling": null,
"rateLimitingSampling": {
"maxTracesPerSecond": 2.1
}
}