Add support for DD_TRACE_AGENT_PORT setting

tylerbenson · tylerbenson · commit 93c046a664b6 · 2018-11-16T11:09:26.000-08:00
Makes this more consistent with other apm language clients.
diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java
@@ -31,7 +31,8 @@ public class Config {
   public static final String SERVICE_NAME = "service.name";
   public static final String WRITER_TYPE = "writer.type";
   public static final String AGENT_HOST = "agent.host";
-  public static final String AGENT_PORT = "agent.port";
+  public static final String TRACE_AGENT_PORT = "trace.agent.port";
+  public static final String AGENT_PORT_LEGACY = "agent.port";
   public static final String PRIORITY_SAMPLING = "priority.sampling";
   public static final String TRACE_RESOLVER_ENABLED = "trace.resolver.enabled";
   public static final String SERVICE_MAPPING = "service.mapping";
@@ -54,7 +55,7 @@ public class Config {
   public static final String DEFAULT_AGENT_WRITER_TYPE = DD_AGENT_WRITER_TYPE;
 
   public static final String DEFAULT_AGENT_HOST = "localhost";
-  public static final int DEFAULT_AGENT_PORT = 8126;
+  public static final int DEFAULT_TRACE_AGENT_PORT = 8126;
 
   private static final boolean DEFAULT_PRIORITY_SAMPLING_ENABLED = false;
   private static final boolean DEFAULT_TRACE_RESOLVER_ENABLED = true;
@@ -94,7 +95,10 @@ public class Config {
     serviceName = getSettingFromEnvironment(SERVICE_NAME, DEFAULT_SERVICE_NAME);
     writerType = getSettingFromEnvironment(WRITER_TYPE, DEFAULT_AGENT_WRITER_TYPE);
     agentHost = getSettingFromEnvironment(AGENT_HOST, DEFAULT_AGENT_HOST);
-    agentPort = getIntegerSettingFromEnvironment(AGENT_PORT, DEFAULT_AGENT_PORT);
+    agentPort =
+        getIntegerSettingFromEnvironment(
+            TRACE_AGENT_PORT,
+            getIntegerSettingFromEnvironment(AGENT_PORT_LEGACY, DEFAULT_TRACE_AGENT_PORT));
     prioritySamplingEnabled =
         getBooleanSettingFromEnvironment(PRIORITY_SAMPLING, DEFAULT_PRIORITY_SAMPLING_ENABLED);
     traceResolverEnabled =
@@ -124,7 +128,11 @@ private Config(final Properties properties, final Config parent) {
     serviceName = properties.getProperty(SERVICE_NAME, parent.serviceName);
     writerType = properties.getProperty(WRITER_TYPE, parent.writerType);
     agentHost = properties.getProperty(AGENT_HOST, parent.agentHost);
-    agentPort = getPropertyIntegerValue(properties, AGENT_PORT, parent.agentPort);
+    agentPort =
+        getPropertyIntegerValue(
+            properties,
+            TRACE_AGENT_PORT,
+            getPropertyIntegerValue(properties, AGENT_PORT_LEGACY, parent.agentPort));
     prioritySamplingEnabled =
         getPropertyBooleanValue(properties, PRIORITY_SAMPLING, parent.prioritySamplingEnabled);
     traceResolverEnabled =
diff --git a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy
@@ -19,6 +19,8 @@ class ConfigTest extends Specification {
   private static final DD_SPAN_TAGS_ENV = "DD_SPAN_TAGS"
   private static final DD_HEADER_TAGS_ENV = "DD_HEADER_TAGS"
   private static final DD_JMXFETCH_METRICS_CONFIGS_ENV = "DD_JMXFETCH_METRICS_CONFIGS"
+  private static final DD_TRACE_AGENT_PORT_ENV = "DD_TRACE_AGENT_PORT"
+  private static final DD_AGENT_PORT_LEGACY_ENV = "DD_AGENT_PORT"
 
   def "verify defaults"() {
     when:
@@ -49,7 +51,8 @@ class ConfigTest extends Specification {
     System.setProperty(PREFIX + SERVICE_NAME, "something else")
     System.setProperty(PREFIX + WRITER_TYPE, "LoggingWriter")
     System.setProperty(PREFIX + AGENT_HOST, "somehost")
-    System.setProperty(PREFIX + AGENT_PORT, "123")
+    System.setProperty(PREFIX + TRACE_AGENT_PORT, "123")
+    System.setProperty(PREFIX + AGENT_PORT_LEGACY, "456")
     System.setProperty(PREFIX + PRIORITY_SAMPLING, "true")
     System.setProperty(PREFIX + TRACE_RESOLVER_ENABLED, "false")
     System.setProperty(PREFIX + SERVICE_MAPPING, "a:1")
@@ -92,24 +95,47 @@ class ConfigTest extends Specification {
     environmentVariables.set(DD_WRITER_TYPE_ENV, "LoggingWriter")
     environmentVariables.set(DD_JMXFETCH_METRICS_CONFIGS_ENV, "some/file")
 
+    if (overridePort) {
+      environmentVariables.set(DD_TRACE_AGENT_PORT_ENV, "123")
+    }
+    if (overrideLegacyPort) {
+      environmentVariables.set(DD_AGENT_PORT_LEGACY_ENV, "456")
+    }
+
     when:
     def config = new Config()
 
     then:
     config.serviceName == "still something else"
     config.writerType == "LoggingWriter"
     config.jmxFetchMetricsConfigs == ["some/file"]
+    config.agentPort == expectedPort
+
+    where:
+    overridePort | overrideLegacyPort | expectedPort
+    true         | true               | 123
+    true         | false              | 123
+    false        | true               | 456
+    false        | false              | 8126
+
   }
 
   def "sys props override env vars"() {
     setup:
     environmentVariables.set(DD_SERVICE_NAME_ENV, "still something else")
     environmentVariables.set(DD_WRITER_TYPE_ENV, "LoggingWriter")
+    environmentVariables.set(DD_TRACE_AGENT_PORT_ENV, "777")
 
     System.setProperty(PREFIX + SERVICE_NAME, "what we actually want")
     System.setProperty(PREFIX + WRITER_TYPE, "DDAgentWriter")
     System.setProperty(PREFIX + AGENT_HOST, "somewhere")
-    System.setProperty(PREFIX + AGENT_PORT, "9999")
+
+    if (overridePort) {
+      System.setProperty(PREFIX + TRACE_AGENT_PORT, "123")
+    }
+    if (overrideLegacyPort) {
+      System.setProperty(PREFIX + AGENT_PORT_LEGACY, "456")
+    }
 
     when:
     def config = new Config()
@@ -118,7 +144,14 @@ class ConfigTest extends Specification {
     config.serviceName == "what we actually want"
     config.writerType == "DDAgentWriter"
     config.agentHost == "somewhere"
-    config.agentPort == 9999
+    config.agentPort == expectedPort
+
+    where:
+    overridePort | overrideLegacyPort | expectedPort
+    true         | true               | 123
+    true         | false              | 123
+    false        | true               | 777 // env var gets picked up instead.
+    false        | false              | 777 // env var gets picked up instead.
   }
 
   def "sys props override properties"() {
@@ -127,7 +160,7 @@ class ConfigTest extends Specification {
     properties.setProperty(SERVICE_NAME, "something else")
     properties.setProperty(WRITER_TYPE, "LoggingWriter")
     properties.setProperty(AGENT_HOST, "somehost")
-    properties.setProperty(AGENT_PORT, "123")
+    properties.setProperty(TRACE_AGENT_PORT, "123")
     properties.setProperty(PRIORITY_SAMPLING, "true")
     properties.setProperty(TRACE_RESOLVER_ENABLED, "false")
     properties.setProperty(SERVICE_MAPPING, "a:1")
diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java
@@ -1,7 +1,7 @@
 package datadog.trace.common.writer;
 
 import static datadog.trace.api.Config.DEFAULT_AGENT_HOST;
-import static datadog.trace.api.Config.DEFAULT_AGENT_PORT;
+import static datadog.trace.api.Config.DEFAULT_TRACE_AGENT_PORT;
 
 import datadog.opentracing.DDSpan;
 import java.util.List;
@@ -64,7 +64,7 @@ public Thread newThread(final Runnable r) {
   private boolean queueFullReported = false;
 
   public DDAgentWriter() {
-    this(new DDApi(DEFAULT_AGENT_HOST, DEFAULT_AGENT_PORT));
+    this(new DDApi(DEFAULT_AGENT_HOST, DEFAULT_TRACE_AGENT_PORT));
   }
 
   public DDAgentWriter(final DDApi api) {
diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/DDTracerTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/DDTracerTest.groovy
@@ -5,7 +5,6 @@ import datadog.trace.api.Config
 import datadog.trace.common.sampling.AllSampler
 import datadog.trace.common.sampling.RateByServiceSampler
 import datadog.trace.common.writer.DDAgentWriter
-import datadog.trace.common.writer.DDApi
 import datadog.trace.common.writer.ListWriter
 import datadog.trace.common.writer.LoggingWriter
 import org.junit.Rule
@@ -27,6 +26,16 @@ class DDTracerTest extends Specification {
   @Rule
   public final EnvironmentVariables environmentVariables = new EnvironmentVariables()
 
+  def setupSpec() {
+    // assert that a trace agent isn't running locally as that messes up the test.
+    try {
+      (new Socket("localhost", 8126)).close()
+      throw new IllegalStateException("Trace Agent unexpectedly running locally.")
+    } catch (final ConnectException ioe) {
+      // trace agent is not running locally.
+    }
+  }
+
   def "verify defaults on tracer"() {
     when:
     def tracer = new DDTracer()
@@ -93,11 +102,12 @@ class DDTracerTest extends Specification {
 
     where:
 
-    source   | key           | value           | expected
-    "writer" | "default"     | "default"       | "DDAgentWriter { api=DDApi { tracesEndpoint=http://localhost:8126/v0.3/traces } }"
-    "writer" | "writer.type" | "LoggingWriter" | "LoggingWriter { }"
-    "writer" | "agent.host"  | "somethingelse" | "DDAgentWriter { api=DDApi { tracesEndpoint=http://somethingelse:8126/v0.3/traces } }"
-    "writer" | "agent.port"  | "9999"          | "DDAgentWriter { api=DDApi { tracesEndpoint=http://localhost:9999/v0.3/traces } }"
+    source   | key                | value           | expected
+    "writer" | "default"          | "default"       | "DDAgentWriter { api=DDApi { tracesEndpoint=http://localhost:8126/v0.3/traces } }"
+    "writer" | "writer.type"      | "LoggingWriter" | "LoggingWriter { }"
+    "writer" | "agent.host"       | "somethingelse" | "DDAgentWriter { api=DDApi { tracesEndpoint=http://somethingelse:8126/v0.3/traces } }"
+    "writer" | "agent.port"       | "777"           | "DDAgentWriter { api=DDApi { tracesEndpoint=http://localhost:777/v0.3/traces } }"
+    "writer" | "trace.agent.port" | "9999"          | "DDAgentWriter { api=DDApi { tracesEndpoint=http://localhost:9999/v0.3/traces } }"
   }
 
   def "verify sampler/writer constructor"() {
diff --git a/dd-trace-ot/src/traceAgentTest/groovy/DDApiIntegrationTest.groovy b/dd-trace-ot/src/traceAgentTest/groovy/DDApiIntegrationTest.groovy
@@ -12,7 +12,7 @@ import java.util.concurrent.TimeUnit
 import java.util.concurrent.atomic.AtomicReference
 
 import static datadog.trace.api.Config.DEFAULT_AGENT_HOST
-import static datadog.trace.api.Config.DEFAULT_AGENT_PORT
+import static datadog.trace.api.Config.DEFAULT_TRACE_AGENT_PORT
 
 class DDApiIntegrationTest {
   static class DDApiIntegrationV4Test extends Specification {
@@ -33,7 +33,7 @@ class DDApiIntegrationTest {
       new PendingTrace(TRACER, "1", [:]),
       TRACER)
 
-    def api = new DDApi(DEFAULT_AGENT_HOST, DEFAULT_AGENT_PORT, v4())
+    def api = new DDApi(DEFAULT_AGENT_HOST, DEFAULT_TRACE_AGENT_PORT, v4())
 
     def endpoint = new AtomicReference<String>(null)
     def agentResponse = new AtomicReference<String>(null)
