Skip to content
This repository was archived by the owner on Jul 1, 2022. It is now read-only.

Introduce JAEGER_CONFIG_MANAGER_ENDPOINT property #554

Closed
wants to merge 1 commit into from
Closed

Introduce JAEGER_CONFIG_MANAGER_ENDPOINT property #554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 32 additions & 8 deletions jaeger-core/src/main/java/io/jaegertracing/Configuration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,11 @@ public class Configuration {
*/
public static final String JAEGER_SAMPLER_MANAGER_HOST_PORT = JAEGER_PREFIX + "SAMPLER_MANAGER_HOST_PORT";

/**
* The config manager endpoint URL.
*/
public static final String JAEGER_CONFIG_MANAGER_ENDPOINT = JAEGER_PREFIX + "CONFIG_MANAGER_ENDPOINT";
Copy link
Member

Choose a reason for hiding this comment

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

We should use CONFIG_MANAGER_HOST_PORT as it is also used by baggage restriction. host port is also easier to configure.


/**
* The service name.
*/
Expand Down Expand Up @@ -332,26 +337,36 @@ public static class SamplerConfiguration {
private Number param;

/**
* HTTP host:port of the sampling manager that can provide sampling strategy to this service.
* HTTP URL of the sampling manager that can provide sampling strategy to this service.
* Optional.
*/
private String managerHostPort;
private String serverUrl;

public SamplerConfiguration() {
}

public static SamplerConfiguration fromEnv() {
String configManagerEndpoint = getProperty(JAEGER_CONFIG_MANAGER_ENDPOINT);
if (configManagerEndpoint == null) {
if (getProperty(JAEGER_SAMPLER_MANAGER_HOST_PORT) != null) {
configManagerEndpoint = "http://" + getProperty(JAEGER_SAMPLER_MANAGER_HOST_PORT) + "/sampling";
Copy link
Member

Choose a reason for hiding this comment

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

nit: String.format() might be cleaner here.

Copy link
Author

Choose a reason for hiding this comment

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

If we go with HOST_PORT, shouldn't need to use any sort of string manipulation; pending confirmation that we're happy to diverge from the Go lib.

} else if (getProperty(JAEGER_AGENT_HOST) != null) {
configManagerEndpoint = "http://" + getProperty(JAEGER_AGENT_HOST) + ":"
Copy link
Member

Choose a reason for hiding this comment

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

nit: String.format() might be cleaner here.

+ HttpSamplingManager.DEFAULT_SERVER_PORT + "/sampling";
}
}

return new SamplerConfiguration()
.withType(getProperty(JAEGER_SAMPLER_TYPE))
.withParam(getPropertyAsNum(JAEGER_SAMPLER_PARAM))
.withManagerHostPort(getProperty(JAEGER_SAMPLER_MANAGER_HOST_PORT));
.withServerUrl(configManagerEndpoint);
}

// for tests
Sampler createSampler(String serviceName, Metrics metrics) {
String samplerType = stringOrDefault(this.getType(), RemoteControlledSampler.TYPE);
Number samplerParam = numberOrDefault(this.getParam(), ProbabilisticSampler.DEFAULT_SAMPLING_PROBABILITY);
String hostPort = stringOrDefault(this.getManagerHostPort(), HttpSamplingManager.DEFAULT_HOST_PORT);
String serverUrl = stringOrDefault(this.getServerUrl(), HttpSamplingManager.DEFAULT_SERVER_URL);

if (samplerType.equals(ConstSampler.TYPE)) {
return new ConstSampler(samplerParam.intValue() != 0);
Expand All @@ -367,7 +382,7 @@ Sampler createSampler(String serviceName, Metrics metrics) {

if (samplerType.equals(RemoteControlledSampler.TYPE)) {
return new RemoteControlledSampler.Builder(serviceName)
.withSamplingManager(new HttpSamplingManager(hostPort))
.withSamplingManager(new HttpSamplingManager(serverUrl))
.withInitialSampler(new ProbabilisticSampler(samplerParam.doubleValue()))
.withMetrics(metrics)
.build();
Expand All @@ -384,8 +399,8 @@ public Number getParam() {
return param;
}

public String getManagerHostPort() {
return managerHostPort;
public String getServerUrl() {
return serverUrl;
}

public SamplerConfiguration withType(String type) {
Expand All @@ -398,8 +413,17 @@ public SamplerConfiguration withParam(Number param) {
return this;
}

/**
* @deprecated Use {@link #withServerUrl(String)} instead
*/
@Deprecated
public SamplerConfiguration withManagerHostPort(String managerHostPort) {
this.managerHostPort = managerHostPort;
this.serverUrl = "http://" + managerHostPort + "/sampling";
Copy link
Member

Choose a reason for hiding this comment

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

nit: String.format() might be cleaner here.

return this;
}

public SamplerConfiguration withServerUrl(String serverUrl) {
this.serverUrl = serverUrl;
return this;
}
}
Expand Down
17 changes: 10 additions & 7 deletions jaeger-core/src/main/java/io/jaegertracing/internal/samplers/HttpSamplingManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,23 @@

@ToString
public class HttpSamplingManager implements SamplingManager {
public static final String DEFAULT_HOST_PORT = "localhost:5778";
private final String hostPort;
public static final String DEFAULT_SERVER_HOST = "localhost";
Copy link
Member

Choose a reason for hiding this comment

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

It does not seem to be used. Only port is used in configuration

public static final int DEFAULT_SERVER_PORT = 5778;
public static final String DEFAULT_SERVER_URL =
"http://" + DEFAULT_SERVER_HOST + ":" + DEFAULT_SERVER_PORT + "/sampling";
private final String serverUrl;

@ToString.Exclude private final Gson gson = new Gson();

/**
* This constructor expects running sampling manager on {@link #DEFAULT_HOST_PORT}.
* This constructor expects running sampling manager on {@link #DEFAULT_SERVER_URL}.
*/
public HttpSamplingManager() {
this(DEFAULT_HOST_PORT);
this(DEFAULT_SERVER_URL);
}

public HttpSamplingManager(String hostPort) {
this.hostPort = hostPort != null ? hostPort : DEFAULT_HOST_PORT;
public HttpSamplingManager(String serverUrl) {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like a breaking change.

We can keep hostport. HttpBaggageRestrictionManagerProxy also accepts hostport. Only HTTP sender accepts full URL e.g. http://jaeger-collector:111/api/traces.

Host port is easier to configure. Not sure if there are any advantages of using full URL.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed - the primary driver of using ENDPOINT was to keep this in-line with the Go library. If that's not a concern, then this is something I can change.

this.serverUrl = serverUrl != null ? serverUrl : DEFAULT_SERVER_URL;
}

SamplingStrategyResponse parseJson(String json) {
Expand All @@ -59,7 +62,7 @@ public SamplingStrategyResponse getSamplingStrategy(String serviceName)
try {
jsonString =
makeGetRequest(
"http://" + hostPort + "/?service=" + URLEncoder.encode(serviceName, "UTF-8"));
serverUrl + "?service=" + URLEncoder.encode(serviceName, "UTF-8"));
} catch (IOException e) {
throw new SamplingStrategyErrorException(
"http call to get sampling strategy from local agent failed.", e);
Expand Down
37 changes: 37 additions & 0 deletions jaeger-core/src/test/java/io/jaegertracing/ConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public void clearProperties() throws NoSuchFieldException, IllegalAccessExceptio
System.clearProperty(Configuration.JAEGER_SAMPLER_TYPE);
System.clearProperty(Configuration.JAEGER_SAMPLER_PARAM);
System.clearProperty(Configuration.JAEGER_SAMPLER_MANAGER_HOST_PORT);
System.clearProperty(Configuration.JAEGER_CONFIG_MANAGER_ENDPOINT);
System.clearProperty(Configuration.JAEGER_SERVICE_NAME);
System.clearProperty(Configuration.JAEGER_TAGS);
System.clearProperty(Configuration.JAEGER_ENDPOINT);
Expand Down Expand Up @@ -110,6 +111,42 @@ public void testSamplerConstInvalidParam() {
assertNull(samplerConfig.getParam());
}

@Test
public void testConfigManagerUrl() {
System.setProperty(Configuration.JAEGER_CONFIG_MANAGER_ENDPOINT, "http://example.com/sampling");
SamplerConfiguration samplerConfig = SamplerConfiguration.fromEnv();
assertEquals("http://example.com/sampling", samplerConfig.getServerUrl());
}

@Test
public void testConfigManagerUrlOverridesSamplerManagerHostPort() {
System.setProperty(Configuration.JAEGER_CONFIG_MANAGER_ENDPOINT, "http://example.com/sampling");
System.setProperty(Configuration.JAEGER_SAMPLER_MANAGER_HOST_PORT, "example.org:80");
SamplerConfiguration samplerConfig = SamplerConfiguration.fromEnv();
assertEquals("http://example.com/sampling", samplerConfig.getServerUrl());
}

@Test
public void testSamplerManagerHostPortOverridesAgentHost() {
System.setProperty(Configuration.JAEGER_SAMPLER_MANAGER_HOST_PORT, "example.org:80");
System.setProperty(Configuration.JAEGER_AGENT_HOST, "example.com");
SamplerConfiguration samplerConfig = SamplerConfiguration.fromEnv();
assertEquals("http://example.org:80/sampling", samplerConfig.getServerUrl());
}

@Test
public void testAgentHostOverridesDefault() {
System.setProperty(Configuration.JAEGER_AGENT_HOST, "example.com");
SamplerConfiguration samplerConfig = SamplerConfiguration.fromEnv();
assertEquals("http://example.com:5778/sampling", samplerConfig.getServerUrl());
}

@Test
public void testSettingManagerHostPortProducesUrl() {
SamplerConfiguration samplerConfig = SamplerConfiguration.fromEnv().withManagerHostPort("example.com:80");
assertEquals("http://example.com:80/sampling", samplerConfig.getServerUrl());
}

@Test
public void testReporterConfiguration() {
System.setProperty(Configuration.JAEGER_REPORTER_LOG_SPANS, "true");
Expand Down
2 changes: 1 addition & 1 deletion jaeger-core/src/test/java/io/jaegertracing/internal/samplers/HttpSamplingManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ protected Application configure() {
@Test
public void testGetSamplingStrategy() throws Exception {
URI uri = target().getUri();
undertest = new HttpSamplingManager(uri.getHost() + ":" + uri.getPort());
undertest = new HttpSamplingManager("http://" + uri.getHost() + ":" + uri.getPort());
SamplingStrategyResponse response = undertest.getSamplingStrategy("clairvoyant");
assertNotNull(response.getProbabilisticSampling());
}
Expand Down